Position BlockToolbar below all of the selected block's descendants #62711
Conversation
noisysocks
added
[Type] Bug
|
Changing anything to do with the popover usually carries a pretty high risk of unexpected positioning regressions so I wouldn't backport this to 6.6. |
|
Size Change: +262 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
noisysocks
force-pushed
the
fix/block-toolbar-positioning
branch
from
a009ab1 to
69e3b3c
Compare
packages/block-editor/src/components/block-tools/block-toolbar-popover.js
Outdated
Show resolved
Hide resolved
Good catch! My |
|
Hmm, I'm still seeing the toolbar moving along the page when scrolling: moving-toolbar.mov |
|
Forgot to ping @jasmussen 🙂 this all came from his suggestion in #40382 (comment). Oops, thanks @tellthemachines. I think I misread your initial comment and fixed something different. Will look into this later. |
|
Thanks for working on this! At a glance this seems a solid improvement, with no side-effects I can see. Can you think of any potential side-effects/downsides? |
packages/block-editor/src/components/block-tools/block-toolbar-popover.js
Show resolved
Hide resolved
noisysocks
force-pushed
the
fix/block-toolbar-positioning
branch
from
bceca61 to
e05a055
Compare
|
Flaky tests detected in e05a055. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10446855219
|
|
I've rebased this and (I think) fixed the bug @tellthemachines noticed. @ramonjd reckon you could test? ❤️ |
|
Scroll issue looks to be resolved. Good one! Kapture.2024-08-19.at.18.43.25.mp4On thing I noticed is that the toolbar align itself to the right - from what I can tell it's the inline Kapture.2024-08-19.at.18.41.34.mp4It appears to affect
|
There was a problem hiding this comment.
This PR is definitely an improvement on the current status. Good stuff.
The positioning would be a good follow up if it can't be done neatly here. I think it's expected, and easier to access the controls, when the toolbar positions itself near the selected block.
| const left = Math.min( rect1.left, rect2.left ); | ||
| const top = Math.min( rect1.top, rect2.top ); |
There was a problem hiding this comment.
On thing I noticed is that the toolbar align itself to the right - from what I can tell it's the inline translateX value of the block-editor-block-list__block-popover element.
Could be due to negative values being misinterpreted by the Popover component.
If I pass 0 in the case of negative values — left = left < 0 ? 0 : left — things look okay 🤔
Kapture.2024-08-19.at.19.04.36.mp4
I'm not 100% on this. Smells like a Popover thing.
There was a problem hiding this comment.
Hmm. Can you share the markup you have in the template?
There was a problem hiding this comment.
Nothing out of the ordinary I think. Here's the template:
<!-- wp:template-part {"slug":"header","theme":"emptytheme","tagName":"header"} /-->
<!-- wp:query {"queryId":1,"query":{"offset":0,"postType":"post","order":"desc","orderBy":"date","author":"","search":"","sticky":"","perPage":10}} -->
<div class="wp-block-query"><!-- wp:post-template -->
<!-- wp:post-title {"isLink":true} /-->
<!-- wp:post-excerpt /-->
<!-- /wp:post-template --></div>
<!-- /wp:query -->And the header template part:
<!-- wp:navigation {"ref":6} /-->
<!-- wp:site-title /-->
<!-- wp:site-tagline /-->Does that help? 😄
There was a problem hiding this comment.
Hmm there is probably some hidden element that makes getVisibleElementBounds return a bounds that's larger than it should be, but I'm having a hard time reproducing. Could you export your theme? Or come up with steps to reproduce that begin with installing Emptytheme?
There was a problem hiding this comment.
Try the latest commit I just pushed @ramonjd. I updated our check for hidden elements to explicitly check for <VisuallyHidden> which I think is causing problems because it hides its content using clip which is hard to detect.
There was a problem hiding this comment.
Thanks @aaronrobertshaw
If I get time, I might play around with disallowing negative values. I'm not super confident about that approach but hacking never hurt anyone, except maybe some major corporations, millions of consumers and certain small democracies.
There was a problem hiding this comment.
I'm not sure where the calculations are happening, but a console.trace confirms that that the -1 value is running through these new functions
{left: -1, top: 0, width: 1781, height: 63}
rectUnion @ dom.js:73
getVisibleElementBounds @ dom.js:142
getBoundingClientRect @ index.js:94
But after looking in the wrong place for ages, from what I can tell, it's because the selected submenu has a left value of -1px in its editor styles
Remove these, and it's kosher.
2024-08-22.12.27.22.mp4
So, I suppose there are two choices as far as I can tell:
-
Remove the -1px rules on the submenu (the quickest fix). I tested the bug it was trying to fix and I can't reproduce with the values removed, but it doesn't address the primary cause.
-
"Normalize" negative values in
rectUnion(e.g.,round up to zero in the case ofx) to calculate the visible element bounds. On the surface this makes the most sense since we're only interested in what's visible in the viewport.
There was a problem hiding this comment.
"Normalize" negative values in rectUnion (e.g.,round up to zero in the case of x) to calculate the visible element bounds. On the surface this makes the most sense since we're only interested in what's visible in the viewport.
This is what I was thinking
diff --git a/packages/block-editor/src/utils/dom.js b/packages/block-editor/src/utils/dom.js
index eef322959a..2504c7824b 100644
--- a/packages/block-editor/src/utils/dom.js
+++ b/packages/block-editor/src/utils/dom.js
@@ -65,12 +65,25 @@ export function getBlockClientId( node ) {
* @param {DOMRect} rect2 Second rectangle.
* @return {DOMRect} Union of the two rectangles.
*/
-export function rectUnion( rect1, rect2 ) {
- const left = Math.min( rect1.left, rect2.left );
- const top = Math.min( rect1.top, rect2.top );
- const right = Math.max( rect1.right, rect2.right );
- const bottom = Math.max( rect1.bottom, rect2.bottom );
- return new window.DOMRect( left, top, right - left, bottom - top );
+export function rectUnion( rect1, rect2, viewportRect ) {
+ let left = Math.min( rect1.left, rect2.left );
+ let top = Math.min( rect1.top, rect2.top );
+ let right = Math.max( rect1.right, rect2.right );
+ let bottom = Math.max( rect1.bottom, rect2.bottom );
+
+ if ( viewportRect ) {
+ left = Math.max( left, viewportRect.left );
+ top = Math.max( top, viewportRect.top );
+ right = Math.min( right, viewportRect.right );
+ bottom = Math.min( bottom, viewportRect.bottom );
+ }
+
+ return new window.DOMRect(
+ left,
+ top,
+ right - left,
+ bottom - top
+ );
}
/**
@@ -130,7 +143,12 @@ export function getVisibleElementBounds( element ) {
}
let bounds = element.getBoundingClientRect();
-
+ const viewportRect = {
+ top: 0,
+ right: viewport.innerWidth,
+ bottom: viewport.innerHeight,
+ left: 0,
+ };
const stack = [ element ];
let currentElement;
@@ -138,7 +156,7 @@ export function getVisibleElementBounds( element ) {
for ( const child of currentElement.children ) {
if ( isElementVisible( child ) ) {
const childBounds = child.getBoundingClientRect();
- bounds = rectUnion( bounds, childBounds );
+ bounds = rectUnion( bounds, childBounds, viewportRect );
stack.push( child );
}
}
Thoughts?
There was a problem hiding this comment.
Remove the -1px rules on the submenu
Without looking, I assume this is just to prevent the appearance of a double border along the window edge. That sounds like something we'd visually want to keep.
"Normalize" negative values in rectUnion
In the absence of the first option. This seems like the winner by default 🤷
Thoughts?
Looks viable to me. There's a lot of Math.min/max going on, would a quick inline comment help our future selves grok why the viewport rect is used etc?
There was a problem hiding this comment.
would a quick inline comment help our future selves grok why the viewport rect is used etc?
Definitely. Thanks for the sanity check. I'll commit this change. Appears to work as intended.
There was a problem hiding this comment.
I've taken this for a quick spin and in general it's testing pretty well for me, with the exception of the misaligned toolbar already under discussion.
I also did some smoke testing of other components based off the BlockPopover such as spacing visualizers and resizable cover popover. Didn't spot any issues using this PR, other than gremlins in the visualizers that already exist on trunk.
ramonjd
force-pushed
the
fix/block-toolbar-positioning
branch
from
ae2d23d to
d712323
Compare
| * this calculation function is called | ||
| * multiple times with the same values. | ||
| */ | ||
| const rectUnionCache = new Map(); |
There was a problem hiding this comment.
This is necessary (for now) because, if not cached, rectUnion() is called a few times every second.
On a page left untouched with a popover open, the call count can potentially reach n.
This cache reduces the times the internal logic of rectUnion runs dramatically.
Most of the functions up the chain that use this function are written with useCallback and useMemo, but it indicates that the Popover component is updating all these values using requestAnimationFrame.
Presumably indirectly, via useFloating and/or autoUpdate from floating-ui/@floating-ui/react-dom
There was a problem hiding this comment.
One possible problem would be that this cache is never cleared. So a long-running page with multiple popovers might cause memory leaks.
What if we use memoization rather than a global cache? memize might be helpful here. We can also create and clear the memoization for each instance. This should also help us avoid the need to generate a cache key string using JSON.stringify.
There was a problem hiding this comment.
Great idea, thanks for the feedback
There was a problem hiding this comment.
I tried a few rounds with memoizing the function, and it made things a little janky when scrolling the page, even with a maxSize to limit the cache size.
So I've taken out the optimization for now. It wasn't in response to any perceived performance issue, only a defensive tactic.
There was a problem hiding this comment.
Is it possible that you're using the DOMRect as the input parameter which cannot be memoized? I think the simplest way is to just flatten the input parameters and use primitives like so:
rectUnion(leftX, leftY, leftWidth, leftHeight, rightX, rightY, rightWidth, rightHeight)It gets a little verbose but then they're all primitives which can be memoized accordingly.
That said, we don't need to do this at all if this is not the bottleneck until proven otherwise! 👍
There was a problem hiding this comment.
we don't need to do this at all if this is not the bottleneck until proven otherwise
yeah, there's no demonstrated bottleneck, I was just surprised that this function was running 100,000s of times in a short period of time
There was a problem hiding this comment.
Caching comes with its own performance concerns 😀 So long as isElementVisible is fast it should be okay. I ordered the checks in order of most performant to least performant.
|
I've added a commit that trims the return left/right values of This logic applies only in Anyway, it works it seems. Without it, the popover might not position itself correctly if the selected element's right or left values are offscreen. See #62711 (comment) LTRKapture.2024-08-22.at.14.44.50.mp4Kapture.2024-08-22.at.14.45.40.mp4RTLKapture.2024-08-22.at.15.05.00.mp4Kapture.2024-08-22.at.14.46.44.mp4 |
| * this calculation function is called | ||
| * multiple times with the same values. | ||
| */ | ||
| const rectUnionCache = new Map(); |
There was a problem hiding this comment.
One possible problem would be that this cache is never cleared. So a long-running page with multiple popovers might cause memory leaks.
What if we use memoization rather than a global cache? memize might be helpful here. We can also create and clear the memoization for each instance. This should also help us avoid the need to generate a cache key string using JSON.stringify.
Very good point. I'll look into memoization. 👍🏻 |
There was a problem hiding this comment.
I've retested this one and it is working as advertised. I can no longer replicate the misaligned toolbar.
I'm happy to give this an approval now but it would be great if @kevin940726 can give the code changes a final ✅
| return false; | ||
| } | ||
|
|
||
| const bounds = element.getBoundingClientRect(); |
There was a problem hiding this comment.
Do we still need this if we're using checkVisibility?
There was a problem hiding this comment.
Good question.
I believe so because this tests for elements that would otherwise be visible if not for the 0 dimensions.
E.g., for <div id="test" style="width:0;height:0;">Test</div>, checkVisibility( ...options ) returns true.
https://drafts.csswg.org/cssom-view-1/#dom-element-checkvisibility
There was a problem hiding this comment.
I'm not sure this is supported in all the browsers that we support. I keep getting errors when using Safari.
There was a problem hiding this comment.
Same problem here
There was a problem hiding this comment.
Based on the browserslist config we support, we support the last 2 versions of Safari (17.5, 17.4). Both versions support checkVisibility according to caniuse.
Maybe the config we use is too loose if this is unexpected?
There was a problem hiding this comment.
Safari only updates with MacOS updates for me. (stable MacOS) and I'm still on 17.3 for this reason. I think we might want to update our Safari browser list config to 3 or 4 versions because of this.
There was a problem hiding this comment.
Good catch — yeah, I think we should handle calling this optionally. Even if we officially support the latest two Safari versions, we shouldn't be in a situation where we're breaking the editor in older Safari versions. Here's what I'm seeing when I select a block in the post editor in Safari 16.4.1:
I've put up a quick fix PR here: #65069
…ter limits of the container in which an element is supposed to be "visible" For example, if an element is positioned -10px to the left of the window x value (0), we should discount the negative overhang because it's not visible and therefore to be counted in the visible calculations. Updated comments
ramonjd
force-pushed
the
fix/block-toolbar-positioning
branch
from
591c2e7 to
675b3a6
Compare
| /* | ||
| * To calculate visible bounds using rectUnion, take into account the outer | ||
| * horizontal limits of the container in which an element is supposed to be "visible". | ||
| * For example, if an element is positioned -10px to the left of the window x value (0), | ||
| * this function discounts the negative overhang because it's not visible and | ||
| * therefore not to be counted in the visibility calculations. | ||
| * Top and bottom values are not accounted for to accommodate vertical scroll. | ||
| */ | ||
| if ( containerRect ) { | ||
| left = Math.max( left, containerRect.left ); | ||
| right = Math.min( right, containerRect.right ); | ||
| } | ||
|
|
||
| return new window.DOMRect( left, top, right - left, bottom - top ); | ||
| } |
There was a problem hiding this comment.
The approach makes sense but I don't really think this belongs in rectUnion. In a previous commit we had rectIntersect which we could bring back.
|
Thanks for landing this @ramonjd! ❤️ |
|
Absolutely love to see this. Huge usability improvement :) |
…ordPress#62711) * Position BlockToolbar below all of the selected block's descendants * Fix scrolling * Don't use window global * Explain what capturingClientId is * No need to clip bounds to viewport * Use explicit check for VisuallyHidden * To calculate visible bounds using rectUnion, take into account the outer limits of the container in which an element is supposed to be "visible" For example, if an element is positioned -10px to the left of the window x value (0), we should discount the negative overhang because it's not visible and therefore to be counted in the visible calculations. * switch to checkVisibility DOM method --------- Co-authored-by: noisysocks <[email protected]> Co-authored-by: ramonopoly <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]>
|
Due to this PR, it seems that when a block is scrollable, the block toolbar position moves with it. Could you help me find out the cause of this? |
What, why, how
Fixes #40382.
Currently
BlockToolbarpositions itself around a block's bounds usingelement.getBoundingClientRect().The problem with this is that it doesn't take into account nested elements that overflow the block. For example, Submenu blocks within a Navigation block will overflow the Navigation block when the user hovers over a menu item. This causes the toolbar to be positioned on top of the Submenu which results in a very unusable experience.
This PR is basically a refresh of @getdave's approach in #40625, though I've tried to make things a bit more generic.
Instead of using
element.getBoundingClientRect()we recurse throughelement's descendants and compute the block's bounds by taking a union of the root rect and all descendent rects. Hidden elements are ignored. This results in a rect that corresponds to what the user sees of the block.This logic is applied in two places: 1)
BlockPopoverwhere thePopovercomponentsanchoris computed; and 2)useBlockToolbarPopoverwhich decides whether or not to setflipandshifton thePopoverdepending on whether the block is too close to the header.Testing Instructions
Also need to test that the block popover appears correctly for other blocks in other instances.
Screenshots or screencast
Before:
Kapture.2024-06-21.at.11.27.44.mp4
After:
Kapture.2024-06-21.at.11.26.02.mp4