-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
RichText: remove selection change listener during composition #14449
Conversation
c43764b
to
9c8585a
Compare
@@ -373,6 +373,8 @@ export class RichText extends Component { | |||
// Browsers setting `isComposing` to `true` will usually emit a final | |||
// `input` event when the characters are composed. | |||
if ( event && event.nativeEvent.isComposing ) { | |||
// Also don't update any selection. | |||
document.removeEventListener( 'selectionchange', this.onSelectionChange ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think there's a simpler way togo instead of this selection enabling/disabling? maybe a flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this as the simplest way. There's no need to run a listener if it's not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, I think it's "cleanliest" if you remove listeners if you don't need them, add them back if you do need them again. I made the mistake before not to do that, and it had a negative performance impact (#12480), so I make a habit of taking this "cleaner" approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the issue for me. I'm wondering, is it possible that the event is not rebound properly in a scenario like this:
- Trigger a composition
^
for instance - Leave the RichText by clicking outside or something else?
@youknowriad Thanks for the review! To answer your question: the browser always triggers a |
Description
This should be included in WP 5.2. It worked fine in WP 5.1.
Fixes #14434.
This bug can also be reproduced with other keyboards, including an American keyboard.
option+`
. You started composing a character.a
.Expected:
à
. Actual:`à
.Cause: the selection change event listener is called after input, which overwrites the DOM if the selection state is different (and it is different, but shouldn't be recorded). The overwrite happens for the format boundaries.
Solution: remove the selection change listener during composition.
Includes a minor clean up, as I was experimenting with different events bound to
Editable
.How has this been tested?
See above.
Screenshots
Types of changes
Checklist: