Popover: remove custom frame scroll/resize listeners #54286
Merged
Conversation
This file contains 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
jsnajdr
added
[Type] Code Quality
|
Flaky tests detected in af40356. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6119114278
|
tyxla
reviewed
Sep 8, 2023
jsnajdr
force-pushed
the
remove/popover-frame-listeners
branch
from
af40356
to
b5f24e2
Compare
jsnajdr
force-pushed
the
remove/popover-frame-listeners
branch
from
b5f24e2
to
88e3895
Compare
|
Size Change: -218 B (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
|
The How to test:
|
tyxla
reviewed
Sep 11, 2023
jsnajdr
force-pushed
the
remove/popover-frame-listeners
branch
from
88e3895
to
02b5e9f
Compare
Floating UI has "visual" tests, and for my patch I added only a unit test. A visual test where you can test that it really works in a real browser could be a useful follow-up. In Gutenberg's storybook? I'm not sure, we currently don't have any stories for iframes explicitly, do we? |
We seem to have at least one: https://wordpress.github.io/gutenberg/?path=/story/components-popover--with-slot-outside-iframe |
ciampo
approved these changes
Sep 11, 2023
There was a problem hiding this comment.
Code is nice and clean, and the changes test well as per instructions.
I also tested briefly in Storybook (the story that @tyxla linked above) and things seems to work well as on trunk — ie. the popover is placed correctly only when the toolbar and the sidebar are hidden. I'd be curious to know why that is still the case and if we could make further improvements in floating UI (maybe look recursively for ancestors that are more than one level up the hierarchy?), although definitely not a blocker.
popover.iframe.test.mp4
Thanks for pointing this out, the popover is really quite broken in the storybook. I'll try to find out why. |
|
It's happening because the iframe offset is taken into account only when calculating the client rect for the reference (anchor) element, but not for the floating (popover) element. In Gutenberg only the anchor element is ever in an iframe, the popover is always in the top document. So the bug is not visible. But Storybook renders the entire story, with all elements, inside a "preview iframe". That iframe has an offset that is not applied to the popover element, only the anchor. I'm now going to merge this PR because it's not really related to this situation. We should expand the iframe story to also include the case when the iframe's parent element is scrolling, but let's do it in a different PR, after the positioning gets fixed. If we did it now, we'd be testing something that's already broken. |
|
Agreed 100% with your opinion and your suggested next steps. Thank you! |
|
Also noting that Tooltips (using ariakit, and therefore floating UI) are currently facing the same issue in Storybook
https://wordpress.github.io/gutenberg/?path=/story/components-tooltip--default |
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.
After Floating UI ships a fix for iframe support in
getOverflowAncestors(my WIP PR is here: floating-ui/floating-ui#2535), we'll be able to remove a lot of custom code fromPopover, code that finds the iframe of the reference and floating elements and attachesresizeandscrolllisteners to the iframe parent.This is a draft PR that works together with the Floating UI changes, and is useful for testing that the changes really work.
It will remain a draft until we can really upgrade to a fixed version of Floating UI.