Declunkify focal point picker dragging #28676
Conversation
Also: - fix focus and keyboard input on media container - fix onDragStart and onDragEnd to actually fire
Due to the need to forward a ref through a few components they are also updated. The HOC withNotices is refactored to a function component and made accept and return forwardRef components. ResizableBox is updated to a forwardRef.
gziolo
added
[Type] Enhancement
|
Wow, this is a night and day improvement. You keep making amazing PRs to the project, thank you so much! Big fan of this work — I'll trust the developers to help review the details, but in testing this, the experience is vastly improved. |
There was a problem hiding this comment.
🚀 from me! This is great @stokesman :).
I think offering an onDrag as an "in-between" of start/stop (onChange) works well.
Like you've demonstrated, it provides the flexibility to handle updates/rendering in alternate ways. In this case, manipulating the DOM directly, by-passing React's render cycle (which is always faster).
The trade-off is, we'd need to replicate this setup every time - which I believe is uncommon, but may happen.
It's a 👍 from me.
(Below are my thoughts on Gutenberg / UI / performance. Not directly related to this PR :D)
I feel like this touches upon a deeper problem - one that I've encountered often in my work exploring improving Design Tools for Gutenberg. That is... how do we effectively add rich interactions without degrading performance? @stokesman from what I've observed of your PRs, this is probably something you're mindful of as well.
Rich interactions often involve actions (usually drag) updating many times a second. These updates need to move up to the data store and back down, and ripple React's render cycles. From my research, the best pattern to resolve these issues (at scale) is to liberally use (data) selectors + (component) memoization to control re-renders. The trick is to establish the necessary patterns (perhaps helper functions?) to reduce the verbosity of this "rigging" work.
- interrupt dragging on blur instead of via withFocusOutside - avoid redundant calls to onDragEnd and onChange - have mousemove listener start and stop synchronously - pass focal point value as first argument to onDrag - some minor stylistic changes
|
Thanks for the quick test drive @jasmussen and review @ItsJonQ! I've pushed a bunch of mostly small commits to patch up a few things, add tests, and update docs + changelog. With that, I'll take this out of draft status. |
|
That's great @stokesman ! I'm 👍 for this to merge if other folks are |
|
@stokesman @ItsJonQ @aristath these changes appear to have introduced a breakage for mobile Gutenberg in the form of wordpress-mobile/gutenberg-mobile#3149. Specifically, it appears it may be related to the changes to the This breakage is causing an app crash and is blocking the next release of the mobile apps this week, which is a time-sensitive matter. 😞 Would you be able to help us identify a fix or evaluate the possibility of reverting these changes until a fix is identified please? Thank you! |
It is unknown why relying upon function declaration hoisting causes an error in React Native, but it does occur. Recent changes with WordPress#28676 refactored a class to a functional component, including leveraging function declaration hoisting. I.e. the function was referenced prior to its declaration in terms of the order of the lines of code. Re-ordering the code so that the function declaration occurs prior to referencing it appears to resolve the error in React Native. Related: https://git.io/JtXMm
|
To provide an update on #28676 (comment), I believe I have identified a fix and am awaiting review in #29013. |
It is unknown why relying upon function declaration hoisting causes an error in React Native, but it does occur. Recent changes with #28676 refactored a class to a functional component, including leveraging function declaration hoisting. I.e. the function was referenced prior to its declaration in terms of the order of the lines of code. Re-ordering the code so that the function declaration occurs prior to referencing it appears to resolve the error in React Native. Related: https://git.io/JtXMm
The refactor of withNotices in WordPress#28676 introduced a bug where the functions within `noticeOperations` were changing on every render. That meant that if they were present in the dependency array of a useEffect, then the effect would trigger causing an infinite loop. This was partially fixed with WordPress#29491, but there are other instances where the noticeOperations object itself is being listed as a dependency, and so the problem persists. As suggested by @stokesman WordPress#29491 (comment) this change memoizes the whole object.
The refactor of withNotices in #28676 introduced a bug where the functions within `noticeOperations` were changing on every render. That meant that if they were present in the dependency array of a useEffect, then the effect would trigger causing an infinite loop. This was partially fixed with #29491, but there are other instances where the noticeOperations object itself is being listed as a dependency, and so the problem persists. As suggested by @stokesman #29491 (comment) this change memoizes the whole object.
The refactor of withNotices in #28676 introduced a bug where the functions within `noticeOperations` were changing on every render. That meant that if they were present in the dependency array of a useEffect, then the effect would trigger causing an infinite loop. This was partially fixed with #29491, but there are other instances where the noticeOperations object itself is being listed as a dependency, and so the problem persists. As suggested by @stokesman #29491 (comment) this change memoizes the whole object.
This PR stemmed from realizing that maybe it wasn't just me and my modest computer experiencing clunky dragging in the focal point picker.
#28487 (comment)
Setting out to improve that, it didn't take much tinkering to figure out that the bulk of the poor performance comes from setting block attributes during the drag operation. It's super intensive because it's driven by
onMouseMoveand causes quite the render cascade.While I'd hoped to find the solution with changes only to
FocalPointPicker, the consuming blocks need some changes as well. The Cover and Media & Text blocks have been updated to set their attributes only at the end of a drag operation and to preview the focal point with an imperative (render-free) approach while dragging.A complication ensued in updating the Media & Text block. To create an imperative routine for focal point preview, a ref to a
ResizableBoxwas needed, and being that it was also wrapped bywithNotices, both components needed updates to support ref forwarding. To support that inwithNoticesit seemed best to refactor it to a function component. Touching that component looks fraught with peril since I see no unit tests for it, but I'm hoping there may be some E2E coverage.UPDATE: Unit tests have been added for
withNotices. A few new unit tests have also been added forFocalPointPicker.How has this been tested?
Making posts with Cover and Media & Text blocks and dragging their focal points like mad.
Screen recordings (as specific to my machine as they may be)
Before:
focal-point-drag-before.mp4
After
focal-point-drag-after.mp4
Types of changes
FocalPointPickercallsonChangeonly at the end of a drag operation, whereas before it was called for eachmousemoveevent. So it still works, just not as rapidly as it used to. (I wasn't totally sure it qualifies as breaking but it's how I've classified it in the changelog).onDragadded as an event forFocalPointPicker. This is now what's called eachmousemoveFocalPointPickerdraggable area and adjustment with arrow keys. (This was added Cover: Video - Add position controls #22531 but was no longer working)FocalPointPickerfiresonDragStartandonDragEnd(they were in the code already but did not actually fire).ResizeableBoxis now aforwardRef*withNoticesreturns aforwardRefif passed aforwardRef**React docs suggest to treat changing a component to a
forwardRefas breaking, but I believe that's for a context of changing a class component with an ad-hoc prop to support ref forwarding. In the cases ofResizableBoxandwithNoticesthey had no support for ref forwarding before this otherwise have no changes. So it seems only an unbreaking added feature.Checklist: