Popover: update @floating-ui to latest version, remove custom fix for iframe positioning and scaling
#46845
Conversation
|
Size Change: -104 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
ciampo
added
[Type] Enhancement
|
It would be great to take it to the finish line as it's causing issues when handling WordPress core releases. @tellthemachines highlighted some aspects of it in the Proposal: improve the editor tech workflow for major releases in the "Package ecosystem problems" section. |
gziolo
added
the
[Priority] High
|
👋 @ciampo @jsnajdr @tyxla @noahtallen Any chance y'all could maybe prioritize this one? 😊 Seems like the pinned dependency is adding some extra friction to the WordPress release process 😅 |
|
I'm going to have a look 👍 |
jsnajdr
force-pushed
the
fix/popover-iframe
branch
from
59856b7
to
961758f
Compare
|
I just rebased the PR onto latest There is a breaking change in version 2.0 where the But currently, after the upgrade, the positioning doesn't work. A block toolbar is not aligned to the block, but to the entire document canvas. We need to figure out what's wrong. @mirka have you been looking at this issue, too? Another thing that's very likely to be broken now is the "Zoom out mode" stuff that @ellatrix added in #47004. We'll need to test the behavior when the iframe is zoomed. |
This is fixed now. To take iframes into account, the "virtual elements" passed to Floating UI, i.e., the objects with custom What remains is to fix the "zoom out mode". I suspect we're accounting for the iframe's scale twice now. |
jsnajdr
force-pushed
the
fix/popover-iframe
branch
from
7e43290
to
2962306
Compare
jsnajdr
changed the title
Popover: Update @floating-ui to latest version, remove custom fix for iframe positioning@floating-ui to latest version, remove custom fix for iframe positioning and scaling
jsnajdr
requested review from
ellatrix,
fluiddot,
dcalhoun and
ajitbohra
as code owners
jsnajdr
force-pushed
the
fix/popover-iframe
branch
from
a1015d6
to
26b3e1a
Compare
|
I believe this upgrade is ready for review, testing, and merging. The last issue I fixed today was the experimental "zoom out" mode in Site Editor. I needed to:
How to test: |
jsnajdr
force-pushed
the
fix/popover-iframe
branch
from
26b3e1a
to
f42b785
Compare
There was a problem hiding this comment.
Doing some testing, I spotted some clunkiness that I see only with this branch. When opening the post editor more menu in a small window, and scrolling aggressively to the top, you can see some of that weird jumping around. Previously, that wasn't there.
This branch:
Screen.Recording.2023-08-24.at.16.56.48.mov
trunk:
Screen.Recording.2023-08-24.at.17.01.06.mov
Is this something we should be worried about?
That's indeed an interesting observation, I'll try to figure out why the jumping is happening. Maybe it has something to do with the |
|
Not sure if it helps, but I wonder if these lines could be related: gutenberg/packages/components/src/popover/index.tsx Lines 331 to 334 in f42b785 Notice, in particular, the Related docs: |
There are some news about
By the way, maybe the Post Editor More menu could benefit from using the |
jsnajdr
force-pushed
the
fix/popover-iframe
branch
from
f42b785
to
adb9a2e
Compare
jsnajdr
force-pushed
the
fix/popover-iframe
branch
from
adb9a2e
to
3d62c20
Compare
|
After spending some more time with the autoUpdate( r, f, u, {
layoutShift: false,
animationFrame: true,
} );I'm explicitly disabling the new It seems that it was this combination of two update checks that caused the menu jumping. Using only one check fixes that. Using |
|
|
||
| updateFrameOffset(); | ||
| defaultView.addEventListener( 'resize', update ); | ||
| scrollContainer?.addEventListener( 'scroll', update ); |
There was a problem hiding this comment.
The reason why we need to attach our own listeners to the iframe's scroll container is described in floating-ui/floating-ui#2518. Once that's fixed, Floating UI will attach them automatically for us, will watch for scroll position changes for us, and our workaround code can be removed.
|
After addressing feedback, I once again believe this is ready for final review 🙂 @ciampo can you have an expert look at this? There surely are popover configurations I didn't test. |
|
Your findings about the I spent some time testing popovers across the editor and couldn't see any regression (although I admit that it's been 2 months since I last worked in the editor). I say we merge this PR, and keep our eyes peeled for any report of regression, since we have a couple of months before the next WP release. Feel free to approve this PR on my behalf (I can't, since I'm the original author) |
|
Thank you all so much for fixing this ❤️ |
What?
With floating-ui/floating-ui#2043 merged, this PR updates
@floating-uito its latest version and removes any custom fixes that aroundPopoverpositioning across different documents (ie. anchor inside iframe, reference in root document). (also see floating-ui/floating-ui#1869 for a description of the original issue)Why?
The custom fix was supposed to be temporary, and hopefully
@floating-ui's latest updates will allow us to remove it.How?
@floating-ui's versionTesting Instructions
In the editor
Open the site editor, try editing a template (or the whole site). Select a block, make sure that the toolbar is anchored correctly to the selected block. Try scrolling, make sure that the toolbar scrolls as expected and then sticks at the edges of the viewport.
On storybook
Open the
Popover's "With Slot outside IFrame" Storybook example, and make sure that thePopoverfollow its anchor's position correctlyTesting Instructions for Keyboard
N/A
Screenshots or screencast
N/A