Block toolbar: account for scrollable blocks that affect the position of the block toolbar #66188
Conversation
… y values, which change when the element is scrolled. This change attempts to correct the overflow so the scrollable children do not influence the x,y values of the popover.
|
Size Change: +49 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
aaronrobertshaw
added
[Type] Bug
There was a problem hiding this comment.
Thanks for spinning this up @ramonjd 🚀
I've taken the liberty to flesh out the PR description and test instructions with some of the testing I've done using this PR branch. I don't suspect it to be an exhaustive list though but here's to hoping it helps spark further ideas.
In the meantime:
✅ Code changes match the snippet discussed on the original issue
✅ Toolbar remains correctly positioned for both horizontally and vertically scrollable blocks
✅ Block popovers such as spacing visualizers or cover blocks resizable popover appear unaffected
✅ The block toolbar is positioned correctly when toggling display of a nav submenu, adding links to it etc.
The Navigation block appears to be the only core block where a child extends beyond the visual bounds of the block's wrapper. I happy to be corrected here but if it is. This PR works well for me.
Screen.Recording.2024-10-17.at.3.38.18.pm.mp4
In the video above you might notice the block selection outline not adapting to scrollable blocks as well. I don't think that needs to be covered by this PR and is best suited to a seperate follow-up.
I'll hold off on approving this to leave space for new ideas, further testing etc.
|
Thanks a lot for the great description and test steps. Helps a lot. So far it's working for me too - waiting for @t-hamano to find the bug |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| if ( childBounds.height > bounds.height ) { | ||
| childBounds.y = bounds.y; | ||
| childBounds.height = bounds.height; | ||
| } |
There was a problem hiding this comment.
Unfortunately I think the logic is imperfect — while it works for me when the links within a sub menu are the same height as the submenu item itself, if we add a link within the submenu that is taller (i.e. a link to a page that wraps to a second line) then the toolbar will overlap the submenu again:
If I'm following the logic correctly, in this case, the individual item within the submenu is taller than bounds.height so we use bounds.height instead, so the bounds doesn't grow to include this item.
Apologies that this isn't a very useful comment — unfortunately I can't think of a neat way to determine if the child item is taller because it's in a popover (and so we want to include it) versus it being because of scroll (in which case we want to exclude it) 🤔
There was a problem hiding this comment.
Having trouble replicating it, but I see what you mean based on the logic you've explained
2024-10-18.14.11.51.mp4
🤔 Do we only care about the parentRect?
Maybe we should rather be checking against that:
Example diff
diff --git a/packages/block-editor/src/utils/dom.js b/packages/block-editor/src/utils/dom.js
index 19dd24ce382..1983d37f723 100644
--- a/packages/block-editor/src/utils/dom.js
+++ b/packages/block-editor/src/utils/dom.js
@@ -140,7 +140,8 @@ export function getVisibleElementBounds( element ) {
return new window.DOMRectReadOnly();
}
- let bounds = element.getBoundingClientRect();
+ const parentRect = element.getBoundingClientRect();
+ let bounds = parentRect;
const stack = [ element ];
let currentElement;
@@ -150,12 +151,12 @@ export function getVisibleElementBounds( element ) {
if ( isElementVisible( child ) ) {
const childBounds = child.getBoundingClientRect();
// If the child is larger than the parent, adjust the x and y values to account for any scrolling.
- if ( childBounds.width > bounds.width ) {
- childBounds.x = bounds.x;
+ if ( childBounds.width > parentRect.width ) {
+ childBounds.x = parentRect.x;
}
- if ( childBounds.height > bounds.height ) {
- childBounds.y = bounds.y;
- childBounds.height = bounds.height;
+ if ( childBounds.height > parentRect.height ) {
+ childBounds.y = parentRect.y;
+ childBounds.height = parentRect.height;
}
bounds = rectUnion( bounds, childBounds );
stack.push( child );There was a problem hiding this comment.
Having trouble replicating it, but I see what you mean based on the logic you've explained
Now that you mention it, I can only replicate it when the tall menu item is the last in the list. If it isn't, then when the logic gets to the last child, the union will work against that item so the bug isn't apparent. So perhaps this is a bit more edge-casey than I thought! Ideally it'd be good to figure out, though 🤔
🤔 Do we only care about the parentRect?
Maybe we should rather be checking against that:
Comparing explicitly against the parentRect now more closely matches the intent (and the comment), but I can still replicate the issue, so I'm not sure it changes much, unfortunately.
There was a problem hiding this comment.
I see it now - is it right that it only happens onHover?
Selecting the element, I cannot replicate.
Seems as if the position, onHover, is the previous position. Only onHover where the currently-selected item's toolbar doesn't drop.
2024-10-18.14.46.15.mp4
Interesting!
There was a problem hiding this comment.
Or rather, I'm seeing it only when selecting a menu item with no submenu and the hovering over a menu item with a submenu.
There was a problem hiding this comment.
I managed to get something working by caching all the x/y values and using the max:
2024-10-18.15.25.44.mp4
It's pretty experimental, but I'll commit for consideration 😄
2024-10-18.15.25.19.mp4
There was a problem hiding this comment.
Thanks for digging in further here! That's definitely getting it closer for me. Now it looks like the y position of the toolbar is directly beneath the last item, but not the padding of the sub menu. Much better but still not quite right:
But it might just be close enough?
Or rather, I'm seeing it only when selecting a menu item with no submenu and the hovering over a menu item with a submenu.
Ah yep, that seems to be the case for me. That makes it feel even more edge-casey, so I feel like we could be nearly close to something workable.
| } | ||
| if ( childBounds.height > parentRect.height ) { | ||
| childBounds.y = Math.max( | ||
| parentRect.y, |
There was a problem hiding this comment.
Now, this all makes me suspect that the original logic might be over-engineered? I'm not sure, so don't quote me.
What do we care about here? Whether any children overflow their parents.
A flat list of element.querySelector(*) might do the same job.
I don't really know to be honest 😆
There was a problem hiding this comment.
It's a good point, this is pretty complex and I'm finding it hard to reason about making changes that feel stable.
What do we care about here? Whether any children overflow their parents.
A flat list of element.querySelector(*) might do the same job.
I don't really know to be honest 😆
The more I look at the logic the less I feel sure of how it should work, but that's also possibly because of being at the end of the week!
I like your thinking in general about simplification. Overall we're trying to see if any children extend beyond the bounds of the top-level container visually. It'd be great to be able to do that without having to walk the entire DOM hierarchy, which depending on the circumstances could be fairly complex.
A flat list of element.querySelector(*) might do the same job.
I suppose the question then is which is more performant: iterating over currentElement.children and adding to a stack, or using the querySelector. In my mind what we're doing is fairly similar in each case, though element.querySelector(*) might be slightly easier to read as it'd be a forEach instead of a nested for within a while 🤔
For now, I'd probably lean toward seeing if we can keep the current overall shape of the code in place while fixing the bug, but I do agree it's worth revisiting the overall complexity here.
There was a problem hiding this comment.
It's a good point, this is pretty complex and I'm finding it hard to reason about making changes that feel stable.
Ditto.
For now, I'd probably lean toward seeing if we can keep the current overall shape of the code in place while fixing the bug, but I do agree it's worth revisiting the overall complexity here.
In my testing it appears latest update in 4403bb9 has reintroduced the issue where a vertically scrolling block's toolbar isn't hidden until after the height of the scrollable content.
Here's what I'm now seeing
Screen.Recording.2024-10-18.at.4.35.41.pm.mp4
Personally, I'm starting to feel as though we should ensure we have clarity around what we have to achieve, then perhaps start from there, working to the simplest solution rather than persist with the complex approach and hope to work back to a simpler option. In other words, if we're struggling to reason about the changes etc it could be hard to reach the point where we're confident enough that it could be refactored.
There was a problem hiding this comment.
Personally, I'm starting to feel as though we should ensure we have clarity around what we have to achieve, then perhaps start from there, working to the simplest solution rather than persist with the complex approach and hope to work back to a simpler option. In other words, if we're struggling to reason about the changes etc it could be hard to reach the point where we're confident enough that it could be refactored.
Also a very good point! No objections from me if we think we can come up with a simpler solution 👍
There was a problem hiding this comment.
With the "Friday brain is mush" disclaimer established, take the following with a grain of salt.
Do we need to treat children that are scrollable within the parent differently to one rendered "outside" the parent. The former should be restricted by the parent's bounds, right? The latter shouldn't be?
If that makes any sense, I appreciate that it isn't making things simpler as we'd hoped.
|
Flaky tests detected in 4403bb9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11397547769
|
| if ( childBounds.width > parentRect.width ) { | ||
| childBounds.x = Math.max( | ||
| parentRect.x, | ||
| xValues.sort().at( -1 ) || 0 |
There was a problem hiding this comment.
Just a thought, but if we stick with the idea of caching values, do we need arrays? Could we set let maxX = 0 and only update maxX if childBounds.x is greater than it?
|
In case it helps explain my early thought bubble, I've pushed a branch based off trunk that might illustrate it in code. I'm probably missing something stupid, so I'm not intending that branch to supplant this one. If it does somehow get the job done, we can easily bring the change in here or convert that branch to a PR. Using the alternative approach that checks if a child is within a scrollable parent this is what I get: Navigation Submenu TestScreen.Recording.2024-10-18.at.5.33.19.pm.mp4Scrollable Blocks TestScreen.Recording.2024-10-18.at.5.32.37.pm.mp4It should be noted that this is a naive implementation, it doesn't take into account descendants of children that could be positioned in a way that would always make them visible despite being within the overall scrollable block. That's an edge case that's so contrived it could be addressed when/if it ever becomes an issue. |
|
Sorry for the late reply. I realized that this is a much more difficult problem than I had imagined 😅 I also wondered if there was a better approach. One approach I came up with is the following logic. I was inspired by the isScrollable function proposed by @aaronrobertshaw:
The code that actually tried this approach is below. The diff below is for the trunk branch, not for this PR: Diffdiff --git a/packages/block-editor/src/utils/dom.js b/packages/block-editor/src/utils/dom.js
index 9c2e813ef7..bd91fbc04e 100644
--- a/packages/block-editor/src/utils/dom.js
+++ b/packages/block-editor/src/utils/dom.js
@@ -74,6 +74,29 @@ export function rectUnion( rect1, rect2 ) {
return new window.DOMRectReadOnly( left, top, right - left, bottom - top );
}
+/**
+ * Checks if the element is horizontally scrollable.
+ *
+ * @param {Element} element Element.
+ * @return {boolean} True if the element is horizontally scrollable.
+ */
+function isHorizontallyScrollable( element ) {
+ const style = window.getComputedStyle( element );
+ return style.overflowX === 'auto' || style.overflowX === 'scroll';
+}
+
+/**
+ * Checks if the element is vettically scrollable.
+ *
+ * @param {Element} element Element.
+ * @return {boolean} True if the element is vettically scrollable.
+ */
+function isVerticallyScrollable( element ) {
+ const style = window.getComputedStyle( element );
+ console.log( style.overflowY );
+ return style.overflowY === 'auto' || style.overflowY === 'scroll';
+}
+
/**
* Returns whether an element is visible.
*
@@ -149,6 +172,16 @@ export function getVisibleElementBounds( element ) {
for ( const child of currentElement.children ) {
if ( isElementVisible( child ) ) {
const childBounds = child.getBoundingClientRect();
+ const currentElementBounds =
+ currentElement.getBoundingClientRect();
+ if ( isHorizontallyScrollable( currentElement ) ) {
+ childBounds.width = currentElement.clientWidth;
+ childBounds.x = currentElementBounds.x;
+ }
+ if ( isVerticallyScrollable( currentElement ) ) {
+ childBounds.height = currentElement.clientHeight;
+ childBounds.y = currentElementBounds.y;
+ }
bounds = rectUnion( bounds, childBounds );
stack.push( child );
}It seems to work fine as far as I tested it, but I'm probably missing something. 975c472d6a44561327dd23ad69f5f2ad.mp4 |
|
Thanks for helping out with this problem, everyone! ❤️ The |
|
@t-hamano @aaronrobertshaw Good call on the I tried both code snippets independently and they address all use cases as far as I can see. Also tested both using So, which do folks think? I think it's now between the fewer calls to DOM API methods. I think try/improve-block-toolbar-positioning-alternative might elbow in as it makes only one call to We might even get away with using the existing diff --git a/packages/block-editor/src/utils/dom.js b/packages/block-editor/src/utils/dom.js
index 8a7b21aaa7f..0603e9bbb1d 100644
--- a/packages/block-editor/src/utils/dom.js
+++ b/packages/block-editor/src/utils/dom.js
@@ -118,6 +118,22 @@ function isElementVisible( element ) {
return true;
}
+/**
+ * Checks if the element is scrollable.
+ *
+ * @param {Element} element Element.
+ * @return {boolean} True if the element is scrollable.
+ */
+function isScrollable( element ) {
+ const style = window.getComputedStyle( element );
+ return (
+ style.overflowX === 'auto' ||
+ style.overflowX === 'scroll' ||
+ style.overflowY === 'auto' ||
+ style.overflowY === 'scroll'
+ );
+}
+
/**
* Returns the rect of the element including all visible nested elements.
*
@@ -136,41 +152,23 @@ function isElementVisible( element ) {
*/
export function getVisibleElementBounds( element ) {
const viewport = element.ownerDocument.defaultView;
+
if ( ! viewport ) {
return new window.DOMRectReadOnly();
}
- const parentRect = element.getBoundingClientRect();
- let bounds = parentRect;
-
+ let bounds = element.getBoundingClientRect();
const stack = [ element ];
- const xValues = [];
- const yValues = [];
let currentElement;
while ( ( currentElement = stack.pop() ) ) {
for ( const child of currentElement.children ) {
if ( isElementVisible( child ) ) {
- const childBounds = child.getBoundingClientRect();
- yValues.push( childBounds.y );
- xValues.push( childBounds.x );
- /*
- * If the child is larger than the parent, adjust the x and y values to the greatest known
- * to account for any scrolling.
- */
- if ( childBounds.width > parentRect.width ) {
- childBounds.x = Math.max(
- parentRect.x,
- xValues.sort().at( -1 ) || 0
- );
- }
- if ( childBounds.height > parentRect.height ) {
- childBounds.y = Math.max(
- parentRect.y,
- yValues.sort().at( -1 ) || 0
- );
+ let childBounds = child.getBoundingClientRect();
+ // If the parent is scrollable, use parent's scrollable bounds.
+ if ( isScrollable( currentElement ) ) {
+ childBounds = currentElement.getBoundingClientRect();
}
-
bounds = rectUnion( bounds, childBounds );
stack.push( child );
}
@@ -187,12 +185,12 @@ export function getVisibleElementBounds( element ) {
*/
const left = Math.max( bounds.left, 0 );
const right = Math.min( bounds.right, viewport.innerWidth );
-
bounds = new window.DOMRectReadOnly(
left,
bounds.top,
right - left,
bounds.height
);
+
return bounds;
}At least, in testing, it works for me. Tell if I've missed something from your demo @aaronrobertshaw |
I'm on board with less DOM API method calls if we aren't reinventing the wheel elsewhere. I'm also fine with Aki's version if there is a benefit to splitting the horizontal vs vertical scrollable checks.
I've had a weekend to wipe my memory since I rushed that branch through Friday afternoon. I recall trying Which I'll do in a review shortly 🙂 |
There was a problem hiding this comment.
Thanks for all the iteration here @ramonjd 👍
I think we've narrowed in on something that works for the known uses cases.
✅ Block toolbar is correctly positioned for top most block
✅ Toolbar position is kept below content that expands outside the parent block e.g. nav submenu
✅ Scrolling content within a block does not affect the toolbar position. Works for horizontal, vertical, and dual scrolling content
Screen.Recording.2024-10-21.at.4.45.24.pm.mp4
I've tried to break the current approach but it seems pretty solid.
I could only come up with contrived examples like using fixed positioning on inner block content but even then the block toolbar would behave as it does for other scrollable content, which could be the expected behaviour.
Any unforeseen edge cases would probably warrant a deeper investigation. So in that spirit, I think we can move forward with this approach and land the fix as is for 6.7.
ramonjd
added
the
Backport to WP 6.7 Beta/RC
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: a3c0d3c |
github-actions
bot
added
the
Backported to WP Core
… of the block toolbar (#66188) The rect bounds of blocvk child elements, specifically the x and y values, change when the element is scrolled. This change attempts to correct the overflow so the scrollable children do not influence the x,y values of the block toolbar popover. Co-authored-by: ramonjd <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: kevin940726 <[email protected]>
| let childBounds = child.getBoundingClientRect(); | ||
| // If the parent is scrollable, use parent's scrollable bounds. | ||
| if ( isScrollable( currentElement ) ) { | ||
| childBounds = currentElement.getBoundingClientRect(); | ||
| } |
There was a problem hiding this comment.
Good work on the solution and nice collaboration here. While I was tinkering around on solutions for #66438, I noticed what seems to be a minor issue here so I’m leaving this late review comment.
I know scrollable elements aren’t expected to be a common case but from what I’ve tested this is wasteful. It loops over all their children yet gets and unions the bounds from the same element each time. I spun up #66546 to address it and included some instructions on testing my claim here. I haven’t requested reviews from anyone yet since I don’t think there’s any urgency to this but I thought it better to mention while still pretty fresh.
… of the block toolbar (WordPress#66188) The rect bounds of blocvk child elements, specifically the x and y values, change when the element is scrolled. This change attempts to correct the overflow so the scrollable children do not influence the x,y values of the block toolbar popover. Co-authored-by: ramonjd <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: kevin940726 <[email protected]>
Fixes: #66112
What?
The while that gets the rect bounds of the child, including the x and y values, which change when the element is scrolled. This change attempts to correct the overflow so the scrollable children do not influence the x,y values of the popover.
Why?
Hard to control what you can't see. This makes the block toolbar positioning more robust for extenders.
How?
Adjust calculated x/y values to account for any scrolling.
Testing Instructions
Navigation block with child extending beyond the nav block's bounds
This will confirm that the original fix from #62711 (comment) still works.
Scrollable block toolbars
Follow the nice and detailed instructions on #66112
TL;DR
Block Popovers and Visualizers
Screenshots or screencast
2024-10-17.15.38.45.mp4