Popover: fix limitShift logic by adding iframe offset correctly (and a custom shift limiter) #42950
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
ciampo
changed the title
ciampo
added
[Type] Bug
ciampo
commented
Aug 3, 2022
Comment on lines
121
to
122
| const isTopBottomPlacement = ( placement ) => /top|bottom/i.test( placement ); | ||
| const hasBeforePlacement = ( placement ) => /top|left/i.test( placement ); |
There was a problem hiding this comment.
We may even change this regex to force the top/bottom/left to be at the start of the string?
There was a problem hiding this comment.
Yes, a startsWith could be enough I think..
There was a problem hiding this comment.
I had initially used a RegEx for its compact form, but I switched to startsWith as you suggested in 43c1d8d
|
Size Change: +77 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
fix/popover-limit-shift
branch
from
8ba9cfa to
c6bf3bd
Compare
67 tasks
ciampo
force-pushed
the
fix/popover-limit-shift
branch
from
c6bf3bd to
15b67a6
Compare
I couldn't replicate this.. 🤔 |
ntsekouras
reviewed
Aug 9, 2022
ntsekouras
reviewed
Aug 9, 2022
| : 1; | ||
|
|
||
| return { | ||
| mainAxis: -frameOffset[ mainAxis ], |
There was a problem hiding this comment.
Why do we always return the negative here?
There was a problem hiding this comment.
In my tests, setting value to a positive number never didn't lead to the behavior that we were looking for 🤷 Although it would be great if @talldan (who also has experience working and debugging these middlewares) could confirm that this is the correct "math"
There was a problem hiding this comment.
Instinctively, I would say that the we change sign of the crossAxis to take into account the same change of sign that the offset middleware applies to its mainAxis. Keeping in mind that the two middlewares have opposite definitions for cross and main axis, this means that the change of sign always applied to the axis (always x or always y)
There was a problem hiding this comment.
Yes, it seems the offset plays its part here and the left behavior issue is exactly 2 times the iframe offset.. I'm not sure yet if shift uses the offset value from the middleware and for other calculation takes into account the iframe so its doubled..
There was a problem hiding this comment.
Looking at the source code, it looks like the offset from the offset middleware doesn't get taken into account for the mainAxis (while it does get taken into account for the crossAxis).
This discrepancy may be the root cause for the unexpected behavior that we've been experiencing on this PR. I've opened floating-ui/floating-ui#1859, let's see how that goes
ciampo
force-pushed
the
fix/popover-limit-shift
branch
from
15b67a6 to
43c1d8d
Compare
Here is what I see on my branch after applying this change diff --git a/packages/block-editor/src/components/block-popover/index.js b/packages/block-editor/src/components/block-popover/index.js
index 370959d1b1..8aad54a2d8 100644
--- a/packages/block-editor/src/components/block-popover/index.js
+++ b/packages/block-editor/src/components/block-popover/index.js
@@ -53,7 +53,7 @@ export default function BlockPopover( {
<Popover
ref={ popoverScrollRef }
animate={ false }
- position="top right left"
+ placement="left"
focusOnMount={ false }
anchorRef={ anchorRef }
// Render in the old slot if needed for backward compatibility,In the site editor (iframed), you can notice that the popover keeps shifting when scrolling UP Kapture.2022-08-09.at.19.33.19.mp4Here you can see the behavior of the same popover in the post editor (without iframe): Kapture.2022-08-09.at.19.37.21.mp4Interestingly, this behavior is probably related to the |
|
Update: I believe the unexpected behavior that I flagged above could be related to a limitation of the |
ciampo
force-pushed
the
fix/popover-limit-shift
branch
from
43c1d8d to
7bb7088
Compare
|
I did some more testing, and I believe that the only way to get the to the behaviour that we want (while generalizing it enough to work for different I tried that in 5a1bc5e, and it seems promising — even if I'd really want to avoid implementing custom middleware like that. That's why I opened a PR on the |
ciampo
changed the title
|
@ntsekouras and @talldan , I believe this is ready for another round of review. I've updated the PR description too, for more details on the solution i'm proposing. |
|
Update: I've started working on cross-document support directly in My suggestion is that we merge this PR as a temporary fix, since it solves a few issues related to shift limiting. We will be able to remove the temporary fix once the What do you think, @ntsekouras and @talldan ? |
ntsekouras
reviewed
Sep 21, 2022
ciampo
force-pushed
the
fix/popover-limit-shift
branch
from
88339aa to
c292508
Compare
I just rebased and pushed the updated changes — The PR should be ready for a final review 🙏 |
ciampo
changed the title
ciampo
added
Backport to WP 6.7 Beta/RC
ciampo
force-pushed
the
fix/popover-limit-shift
branch
from
c292508 to
8d01dbf
Compare
|
@c4rl0sbr4v0 @ockham could you please include this PR in WP 6.1 beta 2? Thank you 🙏 |
ockham
pushed a commit
that referenced
this pull request
Sep 26, 2022
…a custom shift limiter) (#42950) * Extract placement utilities * Add offset to limitShift functionality to compensate iframe s offset * CHANGELOG * Custom limitShift implementation * Add offset to the iframe via a custom middleware * Remove unused utilities * Remove unnecessary offset ref * Update CHANGELOG * Add reference to floating-ui s MIT license
|
I just cherry-picked this PR to the wp/6.1 branch to get it included in the next release: 896fc77 |
ockham
removed
the
Backport to WP 6.7 Beta/RC
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Sep 27, 2022
Package updates for bug and regression fixes: * @wordpress/block-directory: 3.15.3 * @wordpress/block-editor: 10.0.3 * @wordpress/block-library: 7.14.3 * @wordpress/block-serialization-default-parser: 4.17.1 * @wordpress/blocks: 11.16.3 * @wordpress/components: 21.0.3 * @wordpress/compose: 5.15.2 * @wordpress/core-data: 5.0.3 * @wordpress/customize-widgets: 3.14.3 * @wordpress/edit-post: 6.14.3 * @wordpress/edit-site: 4.14.4 * @wordpress/edit-widgets: 4.14.3 * @wordpress/editor: 12.16.3 * @wordpress/format-library: 3.15.3 * @wordpress/interface: 4.16.3 * @wordpress/list-reusable-blocks: 3.15.3 * @wordpress/nux: 5.15.3 * @wordpress/preferences: 2.9.3 * @wordpress/reusable-blocks: 3.15.3 * @wordpress/server-side-render: 3.15.3 * @wordpress/style-engine: 1.0.2 * @wordpress/widgets: 2.15.3 References: * [WordPress/gutenberg#44233 Gutenberg PR 44233] – Blocks: Fix searching of blocks when description is non-string * [WordPress/gutenberg#44301 Gutenberg PR 44301] – Block Toolbar: update position when moving blocks * [WordPress/gutenberg#44334 Gutenberg PR 44334] – Global Styles: Re-add styles that were removed, for classic themes * [WordPress/gutenberg#44351 Gutenberg PR 44351] – Comments block: Support nested comments settings in the comments blocks * [WordPress/gutenberg#44448 Gutenberg PR 44448] – Add a correct TS signature for useEntityRecords * [WordPress/gutenberg#44315 Gutenberg PR 44315] – Pullquote: fix transform to quote crash * [WordPress/gutenberg#44446 Gutenberg PR 44446] – Fix spacing property generation in flow layout type. * [WordPress/gutenberg#44408 Gutenberg PR 44408] – Upgrade react-easy-crop to bring in fix for site editor iframe * [WordPress/gutenberg#44406 Gutenberg PR 44406] – Style engine: kebab case preset slugs in the editor * [WordPress/gutenberg#44209 Gutenberg PR 44209] – Fixing padding on the post editor when RootPaddingAwareAlignments setting is enabled * [WordPress/gutenberg#42950 Gutenberg PR 42950] – Popover: fix limitShift logic by adding iframe offset correctly (and a custom shift limiter) * [WordPress/gutenberg#44337 Gutenberg PR 44337] – Submenu block href only if url is not empty * [WordPress/gutenberg#44291 Gutenberg PR 44291] – Add role=application to list view to prevent browse mode triggering in NVDA * [WordPress/gutenberg#44283 Gutenberg PR 44283] – Navigation block: Fix submenu colors for imported classic menus * [WordPress/gutenberg#44282 Gutenberg PR 44282] – Fix popover stacking in the customize widgets editor * [WordPress/gutenberg#44247 Gutenberg PR 44247] – Spacing presets: switch to using numbers instead of t-shirt sizes for labels * [WordPress/gutenberg#44299 Gutenberg PR 44299] – Backport template creation changes from core * [WordPress/gutenberg#44294 Gutenberg PR 44294] – [Block Library - Query Loop]: Fix broken preview in specific category template * [WordPress/gutenberg#44287 Gutenberg PR 44287] – [Block Library]: Rename Comments pagination inner blocks * [WordPress/gutenberg#44256 Gutenberg PR 44256] – Avoid showing the recursion warning in previews when replacing template parts * [WordPress/gutenberg#44265 Gutenberg PR 44265] – Blocks: officially deprecated the children and node matchers * [WordPress/gutenberg#44251 Gutenberg PR 44251] – Global styles: Remove the beta label from global styles header Props bernhard-reiter, cbravobernal. See #56467. Built from https://develop.svn.wordpress.org/trunk@54335 git-svn-id: http://core.svn.wordpress.org/trunk@53894 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot
pushed a commit
to platformsh/wordpress-performance
that referenced
this pull request
Sep 27, 2022
Package updates for bug and regression fixes: * @wordpress/block-directory: 3.15.3 * @wordpress/block-editor: 10.0.3 * @wordpress/block-library: 7.14.3 * @wordpress/block-serialization-default-parser: 4.17.1 * @wordpress/blocks: 11.16.3 * @wordpress/components: 21.0.3 * @wordpress/compose: 5.15.2 * @wordpress/core-data: 5.0.3 * @wordpress/customize-widgets: 3.14.3 * @wordpress/edit-post: 6.14.3 * @wordpress/edit-site: 4.14.4 * @wordpress/edit-widgets: 4.14.3 * @wordpress/editor: 12.16.3 * @wordpress/format-library: 3.15.3 * @wordpress/interface: 4.16.3 * @wordpress/list-reusable-blocks: 3.15.3 * @wordpress/nux: 5.15.3 * @wordpress/preferences: 2.9.3 * @wordpress/reusable-blocks: 3.15.3 * @wordpress/server-side-render: 3.15.3 * @wordpress/style-engine: 1.0.2 * @wordpress/widgets: 2.15.3 References: * [WordPress/gutenberg#44233 Gutenberg PR 44233] – Blocks: Fix searching of blocks when description is non-string * [WordPress/gutenberg#44301 Gutenberg PR 44301] – Block Toolbar: update position when moving blocks * [WordPress/gutenberg#44334 Gutenberg PR 44334] – Global Styles: Re-add styles that were removed, for classic themes * [WordPress/gutenberg#44351 Gutenberg PR 44351] – Comments block: Support nested comments settings in the comments blocks * [WordPress/gutenberg#44448 Gutenberg PR 44448] – Add a correct TS signature for useEntityRecords * [WordPress/gutenberg#44315 Gutenberg PR 44315] – Pullquote: fix transform to quote crash * [WordPress/gutenberg#44446 Gutenberg PR 44446] – Fix spacing property generation in flow layout type. * [WordPress/gutenberg#44408 Gutenberg PR 44408] – Upgrade react-easy-crop to bring in fix for site editor iframe * [WordPress/gutenberg#44406 Gutenberg PR 44406] – Style engine: kebab case preset slugs in the editor * [WordPress/gutenberg#44209 Gutenberg PR 44209] – Fixing padding on the post editor when RootPaddingAwareAlignments setting is enabled * [WordPress/gutenberg#42950 Gutenberg PR 42950] – Popover: fix limitShift logic by adding iframe offset correctly (and a custom shift limiter) * [WordPress/gutenberg#44337 Gutenberg PR 44337] – Submenu block href only if url is not empty * [WordPress/gutenberg#44291 Gutenberg PR 44291] – Add role=application to list view to prevent browse mode triggering in NVDA * [WordPress/gutenberg#44283 Gutenberg PR 44283] – Navigation block: Fix submenu colors for imported classic menus * [WordPress/gutenberg#44282 Gutenberg PR 44282] – Fix popover stacking in the customize widgets editor * [WordPress/gutenberg#44247 Gutenberg PR 44247] – Spacing presets: switch to using numbers instead of t-shirt sizes for labels * [WordPress/gutenberg#44299 Gutenberg PR 44299] – Backport template creation changes from core * [WordPress/gutenberg#44294 Gutenberg PR 44294] – [Block Library - Query Loop]: Fix broken preview in specific category template * [WordPress/gutenberg#44287 Gutenberg PR 44287] – [Block Library]: Rename Comments pagination inner blocks * [WordPress/gutenberg#44256 Gutenberg PR 44256] – Avoid showing the recursion warning in previews when replacing template parts * [WordPress/gutenberg#44265 Gutenberg PR 44265] – Blocks: officially deprecated the children and node matchers * [WordPress/gutenberg#44251 Gutenberg PR 44251] – Global styles: Remove the beta label from global styles header Props bernhard-reiter, cbravobernal. See #56467. Built from https://develop.svn.wordpress.org/trunk@54335 git-svn-id: https://core.svn.wordpress.org/trunk@53894 1a063a9b-81f0-0310-95a4-ce76da25c4cd
ootwch
pushed a commit
to ootwch/wordpress-develop
that referenced
this pull request
Nov 4, 2022
Package updates for bug and regression fixes: * @wordpress/block-directory: 3.15.3 * @wordpress/block-editor: 10.0.3 * @wordpress/block-library: 7.14.3 * @wordpress/block-serialization-default-parser: 4.17.1 * @wordpress/blocks: 11.16.3 * @wordpress/components: 21.0.3 * @wordpress/compose: 5.15.2 * @wordpress/core-data: 5.0.3 * @wordpress/customize-widgets: 3.14.3 * @wordpress/edit-post: 6.14.3 * @wordpress/edit-site: 4.14.4 * @wordpress/edit-widgets: 4.14.3 * @wordpress/editor: 12.16.3 * @wordpress/format-library: 3.15.3 * @wordpress/interface: 4.16.3 * @wordpress/list-reusable-blocks: 3.15.3 * @wordpress/nux: 5.15.3 * @wordpress/preferences: 2.9.3 * @wordpress/reusable-blocks: 3.15.3 * @wordpress/server-side-render: 3.15.3 * @wordpress/style-engine: 1.0.2 * @wordpress/widgets: 2.15.3 References: * [WordPress/gutenberg#44233 Gutenberg PR 44233] – Blocks: Fix searching of blocks when description is non-string * [WordPress/gutenberg#44301 Gutenberg PR 44301] – Block Toolbar: update position when moving blocks * [WordPress/gutenberg#44334 Gutenberg PR 44334] – Global Styles: Re-add styles that were removed, for classic themes * [WordPress/gutenberg#44351 Gutenberg PR 44351] – Comments block: Support nested comments settings in the comments blocks * [WordPress/gutenberg#44448 Gutenberg PR 44448] – Add a correct TS signature for useEntityRecords * [WordPress/gutenberg#44315 Gutenberg PR 44315] – Pullquote: fix transform to quote crash * [WordPress/gutenberg#44446 Gutenberg PR 44446] – Fix spacing property generation in flow layout type. * [WordPress/gutenberg#44408 Gutenberg PR 44408] – Upgrade react-easy-crop to bring in fix for site editor iframe * [WordPress/gutenberg#44406 Gutenberg PR 44406] – Style engine: kebab case preset slugs in the editor * [WordPress/gutenberg#44209 Gutenberg PR 44209] – Fixing padding on the post editor when RootPaddingAwareAlignments setting is enabled * [WordPress/gutenberg#42950 Gutenberg PR 42950] – Popover: fix limitShift logic by adding iframe offset correctly (and a custom shift limiter) * [WordPress/gutenberg#44337 Gutenberg PR 44337] – Submenu block href only if url is not empty * [WordPress/gutenberg#44291 Gutenberg PR 44291] – Add role=application to list view to prevent browse mode triggering in NVDA * [WordPress/gutenberg#44283 Gutenberg PR 44283] – Navigation block: Fix submenu colors for imported classic menus * [WordPress/gutenberg#44282 Gutenberg PR 44282] – Fix popover stacking in the customize widgets editor * [WordPress/gutenberg#44247 Gutenberg PR 44247] – Spacing presets: switch to using numbers instead of t-shirt sizes for labels * [WordPress/gutenberg#44299 Gutenberg PR 44299] – Backport template creation changes from core * [WordPress/gutenberg#44294 Gutenberg PR 44294] – [Block Library - Query Loop]: Fix broken preview in specific category template * [WordPress/gutenberg#44287 Gutenberg PR 44287] – [Block Library]: Rename Comments pagination inner blocks * [WordPress/gutenberg#44256 Gutenberg PR 44256] – Avoid showing the recursion warning in previews when replacing template parts * [WordPress/gutenberg#44265 Gutenberg PR 44265] – Blocks: officially deprecated the children and node matchers * [WordPress/gutenberg#44251 Gutenberg PR 44251] – Global styles: Remove the beta label from global styles header Props bernhard-reiter, cbravobernal. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54335 602fd350-edb4-49c9-b593-d223f7449a82
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.
Tracked in #42770
Follows up to the experimentation done in #42531
What?
This PR:
offsetmiddleware back into its own custom middleware, andlimitShiftfunction in order that the iframe offset is taken into account correctly when shifting at the edges of the viewport.Why?
Currently, when the anchor is inside an
iframeand thePopoverhas shifting enabled, the popover stops its shifting on top of its anchor instead of shifting past it.The main issue is that the default
limitShiftfunction has some custom logic when computing the min/max limits for the shifting popover. This logic presents some "limitations" (pun not intended) for our case:offsetmiddleware only on thecrossAxiscrossAxis, it adds that offset conditionally, based on the popover'splacement(min, max)I initially tried compensating for the extra iframe offset via the
offsetoption for the defaultlimitShiftfunction, but that also doesn't work because that offset is added when computing the max limits, but is subtracted when computing the min limits — while, in our case, we'd always need to add the iframe offset.How?
Based on the findings written above, the only solution is to add a custom
limiterfunction for theshiftmiddleware.The new function is a modified version of the default
limitShiftfunction. The only change is that we're always adding the extra offset for theiframewhen computing min/max limits.Another necessary change was to move the addition of the
iframeoffset to its own custom middleware. This has 2 advantages:offsetmiddlewarelimiterfunction as a separate value from the regularoffsetTesting Instructions
No change should be introduced for Popovers where the anchor is inside the same
documentas the popoverThe easiest way to test for these changes is to observe the behavior of the block toolbar in the site editor.
Here's what I've tweaking during my tests:
offsetnumber to the popover in the block toolbarplacementoptions for the popover in the block toolbar (i.e top, bottom, left, right)To make sure the behavior is the correct / expected one, compare the behavior of the block toolbar in the site editor (with iframe) against the behavior of the block toolbar in the post editor (no iframe).
Screenshots or screencast
trunk:before.mp4
This PR:
after.mp4