Zoom out mode: scale iframe instead of contents #47004
Conversation
ellatrix
force-pushed
the
try/zoom-out-iframe-scale
branch
from
8100f10 to
a03799d
Compare
|
Size Change: +226 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
| } | ||
| } | ||
|
|
||
| return new window.DOMRect( left, top, 0, 0 ); |
There was a problem hiding this comment.
The style calculation above is now absorbed in the popover ref calculation.
|
Flaky tests detected in c4c11d6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4075849331
|
| * | ||
| * @default false | ||
| */ | ||
| overlay?: boolean; |
There was a problem hiding this comment.
Isn't that the same as the "placement" prop. In other words, should this be another "placement" instead of a separate prop?
There was a problem hiding this comment.
It could be, if it's fine to add an extra option to the floating UI options.
There was a problem hiding this comment.
I agree that having overlay as a separate prop would basically "override" whatever behavior was set through placement.
Probably adding an extra option to placement feels like the correct choice here, even if that means deviating from @floating-ui. The best thing here would be to make these changes directly in @floating-ui, so that we can keep using placement without any changes in @wordpress/components
There was a problem hiding this comment.
If I understand it correctly, overlay is a type of placement where the popover completely covers the anchor element: it's placed at the anchor's top-left corner, and resized to exactly match the size of the anchor.
@floating-ui's job is then to maintain the placement when things move, resize and scroll.
It could be a nice contribution to @floating-ui, it makes sense to me.
There was a problem hiding this comment.
I agree with @jsnajdr , the best solution here is to keep overlay as an additonal value for the placement prop and remove the separate overlay prop.
What's the specific blocker to making the Inbetween inserter component do the scaling? (as an alternative to adding the new prop to Popover) |
The in between inserter then needs to be aware of the scaling, which is not great. It means some duplicate logic in checking the iframe element for the scale transform. I preferred this approach. |
There was a problem hiding this comment.
Thank you for working on this, Ella!
Since Popover is already quite a complex component which has had (and still has) a few quirks especially when dealing with cross-document references (and more specifically around scaling), I had the same initial thoughts as @talldan
What's the specific blocker to making the Inbetween inserter component do the scaling? (as an alternative to adding the new prop to Popover)
The scaling seems to be a problem quite specific to the inbetween inserter at the moment, and therefore it seemed fitting to me that the complexity would stay in that component. But I can also see why you'd add the logic directly to Popover.
A few more points:
- it would be great if we could add some tests, although I'm not sure there's an easy way to do so (for example,
floating-uiuses e2e + visual regression on the screenshots) - the best solution, in my opinion, would be to add this feature directly to
floating-ui - finally, would it be possible to move the changes to the
Popovercomponent to a separate
cc'ing also @jsnajdr who's been recently taking a look at Popover and @floating-ui
| * | ||
| * @default false | ||
| */ | ||
| overlay?: boolean; |
There was a problem hiding this comment.
I agree that having overlay as a separate prop would basically "override" whatever behavior was set through placement.
Probably adding an extra option to placement feels like the correct choice here, even if that means deviating from @floating-ui. The best thing here would be to make these changes directly in @floating-ui, so that we can keep using placement without any changes in @wordpress/components
| * | ||
| * @default false | ||
| */ | ||
| overlay?: boolean; |
There was a problem hiding this comment.
If I understand it correctly, overlay is a type of placement where the popover completely covers the anchor element: it's placed at the anchor's top-left corner, and resized to exactly match the size of the anchor.
@floating-ui's job is then to maintain the placement when things move, resize and scroll.
It could be a nice contribution to @floating-ui, it makes sense to me.
| return rects.reference; | ||
| }, | ||
| } | ||
| : undefined, |
There was a problem hiding this comment.
Seems to me that the overlay option makes all the other middlewares pointless, except maybe frameOffset.
offset, flip, size, shift, arrow: they all make sense only for popovers attached to one side of the anchor. For a popover that fully covers the anchor, none of them is useful.
Therefore, the middleware should be more like:
const middleware = overlay
? [ /* array of overlay middlewares */ ]
: [ /* array of classic middlewares */ ];There was a problem hiding this comment.
I didn't want to make any assumptions here. The iframe middleware is still needed though. Theoretically you could also still shift the position I guess.
Happy to try that once we merge it here. I think I addressed all feedback now. |
ellatrix
force-pushed
the
try/zoom-out-iframe-scale
branch
from
0192210 to
fc91518
Compare
There was a problem hiding this comment.
Hey @ellatrix , sorry for the delay in my reply — still catching up with a long queue after my last AFK.
Happy to try that once we merge it here.
Sounds good to me — I'd love to see us contributing back to @floating-ui as a follow-up to this PR.
Could you please also add an entry to the components package's CHANGELOG?
Thank you! 🙏
| placement: | ||
| ( normalizedPlacementFromProps === 'overlay' | ||
| ? undefined | ||
| : normalizedPlacementFromProps ) || 'bottom', |
There was a problem hiding this comment.
The <Popover />'s placement prop already has a default value of bottom-start (see props destructuring at the top of the component), and otherwise floating-ui defaults to bottom for its placement option — I think we can safely delete the bottom fallback here
| * | ||
| * @default false | ||
| */ | ||
| overlay?: boolean; |
There was a problem hiding this comment.
I agree with @jsnajdr , the best solution here is to keep overlay as an additonal value for the placement prop and remove the separate overlay prop.
ellatrix
force-pushed
the
try/zoom-out-iframe-scale
branch
from
4b89a9c to
402ec7c
Compare
|
@ciampo Thanks! I adjusted the change log and addressed your feedback. |
There was a problem hiding this comment.
Thank you Ella! I had a good look at the component in Storybook too, and couldn't find any major issues.
One change that I made was to enable the Storybook examples to showcase the new `overlay` value for `placement` — would you be able to apply this change as well?
diff --git a/packages/components/src/popover/stories/index.tsx b/packages/components/src/popover/stories/index.tsx
index f5a8fab45d..15723becf1 100644
--- a/packages/components/src/popover/stories/index.tsx
+++ b/packages/components/src/popover/stories/index.tsx
@@ -31,6 +31,7 @@ const AVAILABLE_PLACEMENTS: PopoverProps[ 'placement' ][] = [
'left',
'left-start',
'left-end',
+ 'overlay',
];
const meta: ComponentMeta< typeof Popover > = {
@@ -170,7 +171,12 @@ export const AllPlacements: ComponentStory< typeof Popover > = ( {
</h2>
<div>
{ AVAILABLE_PLACEMENTS.map( ( p ) => (
- <PopoverWithAnchor key={ p } placement={ p } { ...args }>
+ <PopoverWithAnchor
+ key={ p }
+ placement={ p }
+ { ...args }
+ resize={ p === 'overlay' ? true : args.resize }
+ >
{ children }
<div>
<small>(placement: { p })</small>Another aspect that came to mind while playing with the component in Storybook, is that when the placement is set to 'overlay' a few props may behave in a weird / unexpected way, or become ineffective: offset, noArrow, flip....
Is the behaviour of the component when tweaking those props respecting the consumer's expectations? Should we change anything? Should we at least document those quirks?
ellatrix
force-pushed
the
try/zoom-out-iframe-scale
branch
from
98551ba to
f233d6d
Compare
If there are two sets of props that are mutually incompatible (I also pointed it out in #47004 (comment)), maybe that's a hint we should have two components, not just one? One of them would be the old |
ellatrix
force-pushed
the
try/zoom-out-iframe-scale
branch
from
f233d6d to
c4c11d6
Compare
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Marco Ciampini <[email protected]>
ellatrix
force-pushed
the
try/zoom-out-iframe-scale
branch
from
c4c11d6 to
29fe4a9
Compare
I don't think these props are incompatible. Some props may just not have any effect in some circumstances. I would leave things as they are now and see how it evolves. |
They both make sense for popovers that are outside the anchor element, and they move the popover if there's not enough outer space in the preferred location. I don't see what they could possibly do for an overlay popover. Other than these API details, the PR seems OK and mergeable to me. |
I guess an overlay position could still overflow the anchor if it isn't resized. Anyway, we can figure out these details as use cases pop up. Thanks for the reviews! |
|
Opened floating-ui/floating-ui#2171 to get the conversation started about implementing the overlay functionality directly in |
|
Update: the |
|
Interesting. It's pretty much like their |
|
IMO, if we want to keep the current Otherwise, if we wanted to expose a Also — this conversation around gutenberg/packages/components/src/popover/utils.ts Lines 63 to 70 in 8dee3e6 We could also consider exposing a |
|
The changes in this PR introduced the following warning when switching the device types in the Site Editor. Screenshot
|
@ellatrix , would you be able to take a look at that? |
|
What?
This PR moves the scaling from zoom out mode from the iframe body to the frame element.
Why?
We should avoid adding styles such as margin, height and positioning inside the iframe as it might interfere with theme styles.
See also #46929.
How?
One important detail: I refactored the in between inserter by moving the width/height calculation inside the popover ref and adding a new popover prop "overlay" to tell it to overlay this reference.
The problem with the current approach is that when the width is calculated in the in between inserted, it isn't scaled.
In any case, I find this new approach a bit clearer, it clears up some in between inserter logic.
Testing Instructions
Enable the zoomed out mode experiment. Go to the site editor and toggle it. Ensure popovers are positioned correctly and scrolling works as expected.
Testing Instructions for Keyboard
Screenshots or screencast