Fix popover glitch that results in incorrect toolbar positioning in site editor #43267
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Bug
talldan
changed the title
talldan
commented
Aug 16, 2022
ciampo
reviewed
Aug 16, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
(This PR relates to #41612 (cc @chad1008 ))
While the main issue flagged here seems resolved, I can still find a couple of problems in the behavior of the popover (see the video below):
- When the anchor's
ownerDocumentis inside the iframe (e.g site editor), the toolbar is quite slow to update — this can be noted while selecting a block and scrolling the document up and down: the block toolbar is quite sluggish in following the selected block - While the block toolbar is showing for the selected block, opening the "W" menu causes the toolbar to get out of sync with its anchor. The toolbar repositions itsef correctly only after "forcing" an update (e.g by scrolling)
Kapture.2022-08-16.at.13.49.42.mp4
When reading the "Updating" section of the React docs, I started thinking:
- since calling
autoUpdateis apparently expensive, do we need to callautoUpdateat all in that hook? Could we callupdateinstead? - alternatively, is there a better way to call
autoUpdate(i.e. as an option ofuseFloating) that may allow is to callupdateless often?
| update | ||
| ); | ||
| }, [ anchorRef, anchorRect, getAnchorRect ] ); | ||
| }, [ anchorRef, anchorRect, getAnchorRect, update ] ); |
Contributor
There was a problem hiding this comment.
For good measure, I ran npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/popover/.
While on trunk it flagged the missing update dependency here (and on the useLayoutEffect below), it also flagged reference and refs.floating as missing dependencies. Since reference is a callback ref, and refs.floating is a "modern" React ref, I suggest that we add a comment disabling the ESLint rule and explaining the reason why:
Suggested change
| }, [ anchorRef, anchorRect, getAnchorRect, update ] ); | |
| // 'reference' and 'refs.floating' are refs and don't need to be listed | |
| // as dependencies (see https://github.com/WordPress/gutenberg/pull/41612) | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [ anchorRef, anchorRect, getAnchorRect, update ] ); |
67 tasks
|
Size Change: +71 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
fix/popover-using-outdated-update
branch
from
a5e38b7 to
2896766
Compare
Contributor
|
I've just rebased this PR in order to incorporate the update to floating ui's |
Contributor
|
Update:
Maybe it's the rebase (including the update to
This is fixed by @ntsekouras 's commit (see #43267 (comment) for details)
I'd be curios to see if folks have ideas on this — I get the feeling that we could potentially be able to simplify the component's code in some way |
Contributor
Author
|
I've just pushed another commit that ensures I am still seeing that occasionally the toolbar is incorrectly positioned when selecting a block while the site editor is still loading content. I expect that's because there's nothing in place to update the popover when the reference element resizes. I tested and it happened even before #43172.
It seems better to me too. I could be sure I saw some side to side movement of the toolbar when scrolling before (perhaps some subpixel issues) but that isn't happening now. There's probably still going to be more that can be done. It might be worth tinkering to see whether passing stable references to
I tried switching to just |
ntsekouras
reviewed
Aug 17, 2022
…e function by specifying it in the deps
ntsekouras
approved these changes
Aug 17, 2022
Contributor
ntsekouras
left a comment
ntsekouras
left a comment
There was a problem hiding this comment.
This tests well for me and I haven't noticed any sluggishness - we should keep an eye though on all these Popover stuff for performance regressions..
Thanks Dan!
talldan
force-pushed
the
fix/popover-using-outdated-update
branch
from
bf6c803 to
433c550
Compare
Contributor
|
I didn't manage to test this PR on time before it got merged, but I just noticed that this PR seems to have introduced a regression where the Kapture.2022-08-17.at.14.48.16.mp4I will look more into this |
Contributor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #43172 (comment)
When scrolling in the site editor, the block toolbar jumps to the incorrect position.
The issue seems to be that
Popoverwasn't calling a fresh version of theupdatefunction when scrolling, it was calling an outdated version becauseupdatewasn't specified in the hook deps. The bug has been there since the initial refactor to floating-ui, but it has only become apparent since #43172.Deeper explanation:
Since #43172, on the very first render of a
Popover,frameOffsetis undefined. Theupdatefunction seems to be closing over or caching thisundefinedversion offrameOffset, which results in theframeOffsetbecomingundefinedagain wheneverupdateis triggered by the scroll event.How?
This fixes things by specifying
updatein the hook deps wherever it's needed.After this change it mostly works. I still see a little glitchiness, and I think the solution would be to make sure the
offsetmiddleware has the correctframeOffsetvalue on the very first render.Testing Instructions
Screenshots or screencast