Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't apply author annotations when in composition #3350

Merged
merged 2 commits into from
Nov 9, 2022

Conversation

mejo-
Copy link
Member

@mejo- mejo- commented Nov 2, 2022

When typing dead keys (like diacritics), the browser is in composition mode[1] until the accompanioning character is typed. This breaks author annotations.

We have to hold back the transaction when in composition and only apply it afterwards, as suggested at [2]. So check for view.composing before applying the transaction.

Fixes: #2871

[1] https://w3c.github.io/uievents/#events-compositionevents
[2] https://discuss.prosemirror.net/t/plugins-and-characters-with-a-diacritic/2674/3

Signed-off-by: Jonas jonas@freesources.org

  • Resolves: #
  • Target version: master

Summary

@mejo-
Copy link
Member Author

mejo- commented Nov 2, 2022

Maybe it would be good to have tests for this to prevent regressions. Probably needs an e2e cypress test. Would be curious if you think it's testable in jest.

@juliusknorr juliusknorr added the bug Something isn't working label Nov 3, 2022
@juliusknorr
Copy link
Member

Might not even be testable in cypress if using a chromium based browser. Iirc firefox introduced a slightly different handling of compose keys.

@max-nextcloud
Copy link
Collaborator

I checked and the issue only seems to occure in firefox. So yes... will be hard to test in chrome.

@max-nextcloud
Copy link
Collaborator

/rebase

When typing dead keys (like diacritics), the browser is in composition
mode[1] until the accompanioning character is typed. This breaks author
annotations.

We have to hold back the transaction when in composition and only apply
it afterwards, as suggested at [2]. So check for `view.composing` before
applying the transaction.

Fixes: #2871

[1] https://w3c.github.io/uievents/#events-compositionevents
[2] https://discuss.prosemirror.net/t/plugins-and-characters-with-a-diacritic/2674/3

Signed-off-by: Jonas <jonas@freesources.org>
@nextcloud-command nextcloud-command force-pushed the fix/author_colors_composing branch from b14542c to d1b10e0 Compare November 9, 2022 06:55
@max-nextcloud
Copy link
Collaborator

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@max-nextcloud
Copy link
Collaborator

Also had a brief look at tiptap tests and prosemirror tests.

Tiptap runs all tests in cypress - but sometimes they are just plain js tests.
You could create an editor with the user color extension and then try to test that directly. But i don't know how to get the editor into composition mode.
You could also try and get a hold of the apply function of the Prosemirror plugin and somehow inject a stub for the editorview. But that will only unit test the function itself. If the composition behavior of the browsers change we will not notice.

Prosemirror has an explicit test for composition: https://github.com/ProseMirror/prosemirror-view/blob/master/test/webtest-composition.ts
However that seems pretty hard to mimic as it uses it's own testing toolset etc.

So i don't see a way of testing this right now that would be somewhat reasonable to achieve.

Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tried it in my local firefox and it fixes the issue.

@cypress
Copy link

cypress bot commented Nov 9, 2022



Test summary

104 0 0 0Flakiness 1


Run details

Project Text
Status Passed
Commit 55826da ℹ️
Started Nov 9, 2022 7:06 AM
Ended Nov 9, 2022 7:13 AM
Duration 06:55 💡
OS Linux Ubuntu -
Browser Electron 106

View run in Cypress Dashboard ➡️


Flakiness

cypress/e2e/links.spec.js Flakiness
1 test link marks > link preview > shows a link preview

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@max-nextcloud
Copy link
Collaborator

/backport to stable25

@max-nextcloud
Copy link
Collaborator

/backport to stable24

@max-nextcloud max-nextcloud merged commit bb360c3 into master Nov 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix/author_colors_composing branch November 9, 2022 07:14
@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

@max-nextcloud
Copy link
Collaborator

/backport d1b10e0 to stable25

@max-nextcloud
Copy link
Collaborator

/backport d1b10e0 to stable24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Author color is cleared when accent is added
4 participants