Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Block toolbar: account for scrollable blocks that affect the position of the block toolbar #66188

Merged
merged 3 commits into from
Oct 21, 2024
+22 −2
Merged

Block toolbar: account for scrollable blocks that affect the position of the block toolbar #66188

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cecad1
The while that gets the rect bounds of the child, including the x and…
ramonjd Oct 17, 2024
4403bb9
Check for parent overflow only once.
ramonjd Oct 18, 2024
218ab99
Try a mash up between code suggestions.
ramonjd Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/block-editor/src/utils/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,14 @@ export function getVisibleElementBounds( element ) {
for ( const child of currentElement.children ) {
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.height > bounds.height ) {
childBounds.y = bounds.y;
childBounds.height = bounds.height;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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) 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ramonjd ramonjd Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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.

bounds = rectUnion( bounds, childBounds );
stack.push( child );
}
Expand Down
Loading