[WP 6.7] Scrollable blocks affect the position of the block toolbar

[t-hamano]

Description

If elements within a block are vertically or horizontally scrollable, scrolling will also affect the position of the block toolbar.

This appears to be an issue that first appeared in WordPress 6.7.

There are no scrollable blocks in the core, but it may affect third-party blocks.

Step-by-step reproduction instructions

Run the following code in your browser console to register a horizontally scrollable block and a vertically scrollable block:

wp.blocks.registerBlockType( 'test/horizontally-scrollable', {
	apiVersion: 3,
	title: 'Horizontally scrollable block',
	edit: () => {
		return wp.element.createElement(
			'div',
			wp.blockEditor.useBlockProps( {
				style: {
					overflowX: 'scroll',
					background: 'lightgray',
				},
			} ),
			wp.element.createElement( 'div', {
				style: {
					width: '2000px',
					height: '100px',
				},
			} )
		);
	},
} );

wp.blocks.registerBlockType( 'test/vertically-scrollable', {
	apiVersion: 3,
	title: 'Vertically scrollable block',
	edit: () => {
		return wp.element.createElement(
			'div',
			wp.blockEditor.useBlockProps( {
				style: {
					overflowY: 'scroll',
					background: 'lightgray',
					height: '200px',
				},
			} ),
			wp.element.createElement( 'div', {
				style: {
					height: '1000px',
				},
			} )
		);
	},
} );

• Insert the two blocks.
• Scroll the elements inside the blocks.

Screenshots, screen recording, code snippet

WordPress 6.7 or the latest Gutenberg

trunk.mp4

WordPress 6.6.2

This issue does not occur.

6.6.2.mp4

Environment info

No response

Please confirm that you have searched existing issues in the repo.

• Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

• Yes

[ramonjd]

From #62711 (comment)

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?

Thanks for the great testing instructions @t-hamano

From what I can gather it's this while loop that gets the rect bounds of the child, including the x and y values, which change when the element is scrolled.

I think @kevin940726 might have been right in this comment - we might need a better way to calculate overflow and also optimize the loop.

I did a quick test of the following, and seems to fix it, but it's probably not a holistic solution:

diff --git a/packages/block-editor/src/utils/dom.js b/packages/block-editor/src/utils/dom.js
index 9c2e813ef74..48157f6af2a 100644
--- a/packages/block-editor/src/utils/dom.js
+++ b/packages/block-editor/src/utils/dom.js
@@ -149,6 +149,12 @@ export function getVisibleElementBounds( element ) {
 		for ( const child of currentElement.children ) {
 			if ( isElementVisible( child ) ) {
 				const childBounds = child.getBoundingClientRect();
+				if ( childBounds.width > bounds.width ) {
+					childBounds.x = bounds.x;
+				}
+				if ( childBounds.height > bounds.height ) {
+					childBounds.y = bounds.y;
+				}
 				bounds = rectUnion( bounds, childBounds );
 				stack.push( child );
 			}
@@ -171,6 +177,6 @@ export function getVisibleElementBounds( element ) {
 		right - left,
 		bounds.height
 	);
-
+console.log( { bounds } );
 	return bounds;
 }

[andrewserong]

+1 great issue description @t-hamano, and good catch looking into overriding childBounds @ramonjd! In testing that change locally, I think if we go with that approach we'd likely also need to set childBounds.height to bounds.height where the child bounds height is larger than the bounds height. Otherwise, the block toolbar will continue to display when scrolling down the page, past the apparent end of the block:

2024-10-16.12.44.44.mp4

If we set childBounds.height = bounds.height then I think it fixes it, but also not sure it's a holistic solution as it potentially defeats part of the purpose of the function (to grab the largest possible visible element bounds, and so account for children that are taller than their parents). However in testing locally, it seems to be mostly working, so just thought I'd share in case it helps the debugging here:

2024-10-16.12.45.53.mp4

Here's what I'm using locally (the same as Ramon's snippet, but with one line added):

diff --git a/packages/block-editor/src/utils/dom.js b/packages/block-editor/src/utils/dom.js
index 9c2e813ef74..005472df6af 100644
--- a/packages/block-editor/src/utils/dom.js
+++ b/packages/block-editor/src/utils/dom.js
@@ -145,14 +145,21 @@ export function getVisibleElementBounds( element ) {
 	const stack = [ element ];
 	let currentElement;
 
 	while ( ( currentElement = stack.pop() ) ) {
 		for ( const child of currentElement.children ) {
 			if ( isElementVisible( child ) ) {
 				const childBounds = child.getBoundingClientRect();
+				if ( childBounds.width > bounds.width ) {
+					childBounds.x = bounds.x;
+				}
+				if ( childBounds.height > bounds.height ) {
+					childBounds.y = bounds.y;
+					childBounds.height = bounds.height;
+				}
 				bounds = rectUnion( bounds, childBounds );
 				stack.push( child );
 			}
 		}
 	}
 
 	/*
@@ -167,10 +174,10 @@ export function getVisibleElementBounds( element ) {
 	const right = Math.min( bounds.right, viewport.innerWidth );
 	bounds = new window.DOMRectReadOnly(
 		left,
 		bounds.top,
 		right - left,
 		bounds.height
 	);
-
+console.log( { bounds } );
 	return bounds;
 }

[kevin940726]

My comment in https://github.com/WordPress/gutenberg/pull/62711/files#r1727161279 mostly focused on the potential performance rather than correctness. I think it's a good opportunity though to also try out different options if possible. I was mostly concerned that traversing through every child might be slow for large blocks but that's not proven yet. Feel free to take it for a spin if you're feeling adventurous!

[aaronrobertshaw]

Nice collaboration all around here 👍

So I didn't miss out on the party entirely, I took the latest snippet for a spin as well.

However in testing locally, it seems to be mostly working,

While I'm not sure I have the full history behind this code, the suggested fix does work well in my testing as well. At least following the test instructions on the old PR (#62711) and on this issue.

Blocks with child elements outside the normal block bounds like a navigation's submenu still have the block toolbar appear below the submenu:

The scrollable blocks detailed in this issue appear to have their toolbar behave correctly as Andrew noted in his reply above.

Screen.Recording.2024-10-16.at.4.17.09.pm.mp4

I think I'm missing the edge cases that the proposal is supposed to not work for. I couldn't find them.

[ramonjd]

Thanks for confirming that approach, folks.

I threw the above snippet into a PR #66188 to test. Seems to be okay 🤔 but yeah, haven't tested it many scenarios.