Popover: improve iframe offset computation #42417
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
talldan
added
[Type] Bug
talldan
changed the title
|
Size Change: +219 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
talldan
commented
Jul 14, 2022
talldan
force-pushed
the
fix/popover-offset
branch
from
df90569
to
8140c3b
Compare
ciampo
added
[Feature] UI Components
ciampo
reviewed
Jul 22, 2022
There was a problem hiding this comment.
The shifting behavior could still be improved (as discussed in #42531 (comment)), as currently the popover's shift is limited in such a way that makes it stop right on top of a block when scrolling up:
Kapture.2022-07-22.at.12.33.16.mp4
We could either look into applying a fix to the "shift limiting" behavior separately (#42531), or we could try to add it to this PR. Any preferences?
ciampo
changed the title
I think it's ok to ship this separately as it wasn't intended as a fix for that, more a fix for the Link popovers that were all incorrectly positioned. |
talldan
force-pushed
the
fix/popover-offset
branch
from
37b11df
to
7967da9
Compare
ntsekouras
reviewed
Jul 25, 2022
ciampo
reviewed
Jul 25, 2022
67 tasks
|
I gave this PR another round of testing and found out another issue: the popover's offset doesn't seem to update after the block/pattern inserter is closed Kapture.2022-07-28.at.13.58.35.mp4I can't reproduce this issue in the post editor. |
This seems to be on trunk too. I'll test this now again, but I'd expect it's okay to merge and handle the other issues in follow ups. |
ntsekouras
approved these changes
Jul 28, 2022
|
Thanks for the reviews and guidance, Nik and Marco! |
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?
#42389 fixed an issue with Link Control popovers going offscreen, but as @ntsekouras pointed out, in the site editor the popover isn't taking the iframe offset into account and while now onscreen is incorrectly positioned.
Why?
The issue seems to be that the custom
iframeOffsetmiddleware in the Popover component doesn't work well with floating-ui'sshiftmiddleware.shiftexpects to receive any offsets via middleware data, butiframeOffsetdoesn't do this:https://github.com/floating-ui/floating-ui/blob/77c4b27a8096cafe06e58b2c7592317d6d3a3aea/packages/core/src/middleware/shift.ts#L203-L212
The easiest fix seems to be use floating-ui's own
offsetmiddelware instead of our custom middleware.How?
In practice it was a bit harder than I thought to fix this. The main difficulty is that when using floating ui's
offsetmiddleware, themainAxisandcrossAxisvalues need to be specified instead ofxandy, and floating-ui doesn't seem to offer any way to determine what those are.It also seems that when position is
toporleftthe offset needs to be subtracted, and whenbottomorrightadded. As soon as I fixed the link popover issue, the toolbar was offset incorrectly until I multiplied the offset by the correct sign.Testing Instructions
Expected: The toolbar and the link popover should be positioned correctly.
Screenshots or screencast
Before
After
Other screenshots
To make sure this was right, I tried setting the button block's popover positioning to 'top', 'bottom', 'left' and 'right' and checked the results. I also added some left margin to the site editor's iframe to ensure that x axis offsetting works correctly, and the results were all correct:
Top placement
Right placement
Bottom placement
Left placement