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: selectionChange: bind on focus, unbind on blur #12480

Merged
merged 2 commits into from Nov 30, 2018

Conversation

Projects
None yet
3 participants
@iseulde
Copy link
Member

iseulde commented Nov 30, 2018

Description

This PR is an attempt to decrease the work done on a key press. Every key press is eventually followed by (several) selection change events. Every RichText instance has a listener on this global event, all of which will be called multiple times after a key press. This is not significant for a few instances, but it becomes quite noticeable if there are a huge amount of instances.

The solution is to only add the listener on focus, then remove again on blur.

Testing on with this post I notice one selection change event takes on average 9ms to complete. With this PR it takes on average 0.3ms.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@iseulde iseulde requested review from aduth and WordPress/gutenberg-core Nov 30, 2018

@iseulde iseulde added this to the 4.7 milestone Nov 30, 2018

@iseulde

This comment has been minimized.

Copy link
Member

iseulde commented Nov 30, 2018

screen shot 2018-11-30 at 20 48 30

In this graph you can see all the little callbacks on every selection change.

@@ -354,6 +347,12 @@ export class RichText extends Component {
if ( unstableOnFocus ) {
unstableOnFocus();
}

document.addEventListener( 'selectionchange', this.onSelectionChange );

This comment has been minimized.

@aduth

aduth Nov 30, 2018

Member

What of these lines then?

// Ensure it's the active element. This is a global event.
if ( ! this.isActive() ) {
return;
}

This comment has been minimized.

@iseulde

iseulde Nov 30, 2018

Member

Yeah, we should be able to remove this.

}

onBlur() {
document.removeEventListener( 'selectionchange', this.onSelectionChange );

This comment has been minimized.

@aduth

aduth Nov 30, 2018

Member

I guess blur, despite bubbling, doesn't fire when moving between elements in the same contenteditable ?

https://codepen.io/aduth/pen/VVqOmq?editors=1111

(In other words, I suspected there could be an issue, but it doesn't appear so)

This comment has been minimized.

@iseulde

iseulde Nov 30, 2018

Member

No, it shouldn't, it starts at the contenteditable element.

@@ -392,11 +386,6 @@ export class RichText extends Component {
* Handles the `selectionchange` event: sync the selection to local state.
*/
onSelectionChange() {
// Ensure it's the active element. This is a global event.
if ( ! this.isActive() ) {

This comment has been minimized.

@aduth

aduth Nov 30, 2018

Member

I think I'd noted it elsewhere, but there's a small difference between isActive and onFocus in that the prior was testing the div wrapper as the active element, the latter exclusively applied to the TinyMCE element (i.e. not considering toolbars and such). When last I tested this, I recall it being a reasonable difference in that (a) all other elements are non-visual or slot-rendered and (b) are not relevant to the selection anyways.

Does that sound accurate to you?

This comment has been minimized.

@iseulde

iseulde Nov 30, 2018

Member

Hm, I think both are checking the contenteditable element? this.editableRef is the contenteditable element, and onFocus is a listener on that element. Maybe you're thinking about setFocusedElement?

This comment has been minimized.

@aduth

aduth Nov 30, 2018

Member

Hm, I think both are checking the contenteditable element? this.editableRef is the contenteditable element, and onFocus is a listener on that element. Maybe you're thinking about setFocusedElement?

Ah! I am!

I still think we should consolidate these, as having multiple concepts of focus here is particularly confusing for me, but it's not relevant to the change proposed here.

This comment has been minimized.

@iseulde

iseulde Nov 30, 2018

Member

Yeah, tbh I'm not sure why there's something like setFocusedElement on the wrapper...

This comment has been minimized.

@aduth

aduth Nov 30, 2018

Member

My hope was that at some point, we could get rid of the wrapper, at least in the sense of having it be a <Fragment> instead of an actual DOM div node.

@aduth

aduth approved these changes Nov 30, 2018

Copy link
Member

aduth left a comment

👍

@iseulde iseulde merged commit 2094200 into master Nov 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iseulde iseulde deleted the try/selectionchange-bind branch Nov 30, 2018

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 2, 2018

@iseulde, out of curiosity, do you have an idea if there would be a way to detect that you are still editing a given RichText when you use block's toolbar? Use case would be as follows:

  1. I type something in the RichText area (focus is on RichText)
  2. I use the keyboard shortcut to start navigating in the toolbar (blur is triggered on RichText)
  3. I pick the formatting option
  4. I get back to editing in RichText (focus gets back to RichText)
  5. I select another text in `RichText
  6. I use the keyboard shortcut to add link (focus would stay in RichText because popover is in the same virtual dom tree)
  7. I save the link
  8. I get back to editing in RichText

In other words, whatever you do related to editing, we would need to be able to identify a currently active RichText field. As far as I was trying to tackle it, I had the issue with the focus/blur dance when using block's toolbar. I think the same issue would apply in case you want to use InsepectorControls like in your plugin which adds colors formatting. Inline tokens like images, might be another use case where this is getting tricky.

@iseulde

This comment has been minimized.

Copy link
Member

iseulde commented Dec 3, 2018

Sounds like this is what setFocusedElement in the discussion above?

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 3, 2018

Sounds like this is what setFocusedElement in the discussion above?

This is where we want it to be. It's not yet there :( The issue is mostly when there are other components like PlainText which should take over focus and reset formatters from RichText. It isn't the case. We have an issue which tracks it: #7463.

@iseulde

This comment has been minimized.

Copy link
Member

iseulde commented Dec 3, 2018

But this PR doesn't cause any issues right?

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 3, 2018

But this PR doesn't cause any issues right?

I don't think so. I just wanted to discuss further improvements :)

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 12, 2018

There's an issue with the changes here, in that if the component unmounts while it's focused and before it's blurred, an error can be logged. This can be seen when merging two blocks, for example.

  1. Navigate to Posts > Add New
  2. Add a paragraph with some text
  3. Add a second paragraph with some text
  4. Place caret at the beginning of second paragraph and press Backspace

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.

I'll plan to submit a fix soon, which would be to call to remove the event handler still in componentWillUnmount (is fine to call even if not actually added).

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 13, 2018

I'll plan to submit a fix soon

See: #12817 (now merged)

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Jan 4, 2019

RichText: selectionChange: bind on focus, unbind on blur (WordPress#1…
…2480)

* RichText: selectionChange: bind on focus, unbind on blur

* Remove isActive check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment