Popover: enable auto-updating every animation frame#43617
Merged
Conversation
|
Size Change: -67 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
67 tasks
ciampo
added
[Feature] UI Components
talldan
approved these changes
Aug 29, 2022
Contributor
talldan
left a comment
talldan
left a comment
There was a problem hiding this comment.
LGTM! Thanks for exploring an alternative fix.
A note about performance, in testing the toolbar/popover position is laggier when scrolling if a toolbar dropdown (like the block settings menu) is open, but I don't notice any difference if toolbar dropdowns are closed.
I think it's more important to fix the bug right now, and then look at performance separately, so it'd be good to move ahead with merging this.
ciampo
force-pushed
the
fix/popover-autoupdate-raf
branch
from
6701a22 to
d164ae8
Compare
Contributor
Author
|
Just rebased this PR, will merge once CI is ✅ Regarding performance, I didn't really spot increased lagginess, but I really didn't do much testing on that, as I focused on fixing the bugs. Once this PR gets merged, I should be able to work on #43335, which hopefully may bring some perf improvements 🤞 although the "real" fix will be to improve cross-document support directly in |
This was referenced Aug 29, 2022
Contributor
Author
|
This PR seems to have broken the draggable block e2e tests, I'll see if I can make the test work — otherwise I will open a PR that will partially undo the changes from this PR |
Contributor
Author
ciampo
added
the
Needs Dev Note
This was referenced Sep 15, 2022
youknowriad
reviewed
Oct 3, 2022
| update | ||
| update, | ||
| { | ||
| animationFrame: true, |
Contributor
There was a problem hiding this comment.
I noticed that you did some follow-up work to this, so I was wondering if this is still necessary. The reason I'm asking is because this line is a source of a large impact on performance degradation potentially.
It now happens in the "select" and "inserter open" metrics after this PR #42722 but it can also happen I guess for other interactions when there's a big list of popovers.
So can we remove it or is it still necessary? Are there alternatives?
Member
There was a problem hiding this comment.
I guess this can be opt-in (or opt-out). However, I think it's generally a bad practice to render a large list of popovers at the same time. The performance regression in #42722 is a bit extreme that we render 1000 empty paragraphs in the test, but I do think we can refactor it in a follow-up to improve this.
From the floating-ui doc:
This is optimized for performance but can still be costly.
This option should be used sparingly...
I guess it makes sense to make it an opt-in in the next major release?
Contributor
Author
There was a problem hiding this comment.
This was introduced mainly to fix #42725, I expect that bug (and any other nested popover) to happen again if we undo this change — maybe it could be just enabled for the block toolbar?
While I agree that calling the getBoundingClientRect function every animation frame is not ideal, I would also argue that the main reason while this can be a noticeable regression is that we're rendering hundreds of popovers at the same time.
Contributor
There was a problem hiding this comment.
It turned out that the main issue for the performance regression is not this, so I'm ok not doing anything here and keeping it as but we should keep an eye on this any way as mentioned on their docs.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Dev note
See the dev note in #44195
What?
Fixes #42725
This PR changes the way
floating-uiauto-updates the popover's position by enabling theanimationFrameoption. This means that the popover position is recalculated every animation frame (see the docs for more details).This has a few benefits:
__unstableObserveElementprop (and the associatedMutationObserver) are not necessary anymore, and can be removedscrollevent listener added to theiframeis not necessary anymore, and can be removedWhy?
Currently, the
Popoverelement adds some custom event listeners to the anchor and itsonwerDocumentwhen the anchor is in a different document from the popover (i.e. in an iframe, which is the case for the block toolbar in the site editor).These events don't cover all edge cases, and are therefore prone to causing regressions when other parts of the
Popoverare updated.How?
By setting the
animationFrameoption totruewhen calling theautoUpdatefunction fromfloating-uiWhat is not included in this PR
The changes in this PR were inspired by the work done in #43335, but I've deliberately decided to keep this PR as small as possible to make it easier to review.
I plan on iterate on
Popoverimprovements in #43335, where I'm exploring / going to explore the following ideas:autoUpdateonly as part of theuseFloatingfunction call (and not in theuseLayoutEffecthook, like it is at the moment)Popovercomponent update correctly and more frequently in more scenarios (e.g. prop change, ref changes...)referencewithin the sameuseLayoutEffectwhere the reference itself is computedResizeObserverinstead of listening to theresizeeventownerDocumentTesting Instructions
Check that the remaining popovers behave as expected, without any regressions..
Screenshots or screencast
Kapture.2022-08-25.at.16.18.14.mp4