Block toolbar position in 6.7 can shift unexpectedly

[ndiego]

Description

This issue is likely the same one displayed in #65872

Changes were made in 6.7 that adjusted the positioning of the block toolbar. I have not yet been able to identify the PR, but the following videos demonstrate the situation. I used a WIP branch of the Icon Block in the videos below, but you can experience the same issue using the public preview with 6.7 applied. The toolbar will bounce up and down when you click the rotate button in the toolbar.

Block toolbar in 6.6

The toolbar does not move.

block-toolbar-6.6.mp4

Block toolbar in 6.7

The toolbar bounces around.

block-toolbar-6.7.mp4

Step-by-step reproduction instructions

1. Preview the Icon Block using the public preview with 6.7
2. The preview will open the Editor with the WordPress icon added to the page. Click on the W icon and rotate it using the button in the block toolbar.
3. Notice the toolbar bounces up and down.
4. Switch to 6.6 and notice that the toolbar remains fixed.

Screenshots, screen recording, code snippet

No response

Environment info

• WordPress 6.7 RC1

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

Please confirm which theme type you used for testing.

• Block
• Classic
• Hybrid (e.g. classic with theme.json)
• Not sure

[t-hamano]

I was able to reproduce the problem too:

d6b6457dd762b130acfc20ab439e47dd.mp4

@ramonjd @andrewserong @aaronrobertshaw I think this issue is related to the one we addressed in #66188 😅 Is there a good approach to solving this? I suspect this is because a transition is applied to the icon, and the rectangle is recalculated while the element is rotating.

@ndiego This may be a temporary solution, but disabling transitions seems to prevent this issue from occurring:

06d5fc76dbc85db97accc640b8c9cc1e.mp4

[ramonjd]

I think this issue is related to the one we addressed in #66188 😅 Is there a good approach to solving this? I suspect this is because a transition is applied to the icon, and the rectangle is recalculated while the element is rotating.

Yeah, it looks like the toolbar is trying to sit on top of the block, using the block's rect values as a guide.

This is normally a good thing as it ensures the controls do not cover the block's contents.

2024-10-25.13.33.01.mp4

But it can be pronounced where the height and width change rapidly:

2024-10-25.13.29.02.mp4

I'm not quite sure what to do here. I don't believe there's a reliable way to detect transitions.

[t-hamano]

This is normally a good thing as it ensures the controls do not cover the block's contents.

Yes, the block toolbar shift is odd, but the behavior itself seems to follow the expected specifications.

Here's a video that shows the issue in a more clear way:

9c1277c11d6825adb72b771abed6b85b.mp4

[aaronrobertshaw]

Just a quick passing thought: could we check whether the block's overflow is hidden/clipped and adjust calculations around the children to suit? Similar to the approach to balance scrollable content with content positioned outside the parent block in #66188. It might only mitigate some of the issue but that could be enough in the short term 🤷

I haven't had a chance to play with the code yet, so I could be on the wrong track, feel free to disregard this.

[getdave]

Just noting that it's likely the potential benefits provided by the new positioning algorithm for the toolbar are significant. For example, it avoids the toolbar being obscured by the Navigation block submenus in the canvas which was a longstanding bug and frustration with a key Core block.

That said, I recognise when these edge cases do occur they can be quite pronounced as demonstrated by Nick here.

Given that there is an immediate fix here (don't use the transition) I don't currently see this as being a critical bug with 6.7. However I like the idea from @aaronrobertshaw above - might be worth exploring.

[ndiego]

Given that there is an immediate fix here (don't use the transition) I don't currently see this as being a critical bug with 6.7. However I like the idea from @aaronrobertshaw above - might be worth exploring.

Yes, I do think the benefits outweigh the edge cases like the one here. I do not think this is critical to fix in 6.7, but want to make sure this issue was logged should it come up in other custom blocks, like in #65872. If there is no easy solution, and I doubt there is, I am happy to punt.

[t-hamano]

My other thought is that maybe the new positioning algorithm should be an opt-in feature via block support.

From my understanding, the new positioning algorithm solves the issue for the Navigation block, but all other blocks don't seem to have the toolbar issue.

I think the ideal approach would be to add new block support for the new positioning algorithm, similar to __experimentalExposeControlsToChildren, and apply it only to the Navigation block. Alternatively, it might be possible to extend __experimentalExposeControlsToChildren itself to accept a non-boolean value to opt-in to the new placement algorithm.

[stokesman]

maybe the new positioning algorithm should be an opt-in feature via block support

That’s an interesting idea and could be a practical solution.

I had a couple ideas other ideas that wouldn’t require expanding the block API surface.

• Revise the new positioning algorithm to use offsetTop and offsetWidth as such properties report untransformed values so they won’t vary if a block is being transformed. I quickly tried it and it might be workable but not terribly simple.
• Make the block toolbar popover opt out of the animation loop that repositions it continuously. (The animationFrame parameter to autoUpdate). The tricky part though is opting back in when needed—e.g. the zoom out transition and maybe other times. I like this idea but haven’t had an idea for the tricky part.

[ramonjd]

My other thought is that maybe the new positioning algorithm should be an opt-in feature via block support.

From my understanding, the new positioning algorithm solves the issue for the Navigation block, but all other blocks don't seem to have the toolbar issue.

You raise a great point.

The positioning logic was rolled out to the general block population when the only use case (so far) was the navigation block.

Gating it to nav block and its children seems very reasonable.

[t-hamano]

I'm not sure how to address this issue in 6.7 if there is no easy solution other than introducing an opt-in API.

If the only cases where the issue occurs are #66438 and #65872, it might be considered an edge case, but there might be underlying issues that haven't been reported by block developers yet 🤔

[andrewserong]

Given where we are in the release cycle, and that it might be an edge case, I think I'd probably lean toward leaving as-is for 6.7 and if it's a problem for block developers, prioritise a fix for a 6.7.1 point release. Would that work?

[stokesman]

Just to note about the second idea I mentioned earlier "Make the block toolbar popover opt out of the animation loop that repositions it continuously". I gave it a shot and it mostly worked but I realized that anytime the popover rerenders it will still reposition based on the element transforms. Among the many things that can rerender the toolbar is hovering parts of it so there would still be potential for the unwanted repositions. Scratch that one!

[ndiego]

Thanks for looking into this everyone. I am going to punt it to 6.7.1.

[t-hamano]

I ran into a related issue when updating a client's site to WP 6.7.

I sometimes scale images to make it easier to see which images have links applied to them, for example using code like this:

function test_scale_image_style() {
	register_block_style(
		'core/image',
		array(
			'name'         => 'scale',
			'label'        => "Scale",
			'inline_style' => '
			.wp-block-image.is-style-scale {
				overflow: hidden;
			}
			.wp-block-image.is-style-scale:has(a) img {
				transition: transform 0.3s ease;
			}
			.wp-block-image.is-style-scale:has(a):hover img {
				transform: scale(1.2);
			}',
		)
	);
}
add_action( 'init', 'test_scale_image_style' );

The new positioning logic affects cases like this too.

9e8fe3609675de3227ff6ef74f24fe69.mp4

[ramonjd]

Gatekeep to core/navigation anyone? 😄

Seems to fix all bugs and retain the original intention.

Are there other blocks that have overflow like the navigation block?

diff --git a/packages/block-editor/src/utils/dom.js b/packages/block-editor/src/utils/dom.js
index e30f809e38..c8b9ae3ad8 100644
--- a/packages/block-editor/src/utils/dom.js
+++ b/packages/block-editor/src/utils/dom.js
@@ -158,17 +158,19 @@ export function getVisibleElementBounds( element ) {
 	}
 
 	let bounds = element.getBoundingClientRect();
-	const stack = [ element ];
-	let currentElement;
-
-	while ( ( currentElement = stack.pop() ) ) {
-		// Children won’t affect bounds unless the element is not scrollable.
-		if ( ! isScrollable( currentElement ) ) {
-			for ( const child of currentElement.children ) {
-				if ( isElementVisible( child ) ) {
-					const childBounds = child.getBoundingClientRect();
-					bounds = rectUnion( bounds, childBounds );
-					stack.push( child );
+	if ( element?.dataset?.type === 'core/navigation' ) {
+		const stack = [ element ];
+		let currentElement;
+
+		while ( ( currentElement = stack.pop() ) ) {
+			// Children won’t affect bounds unless the element is not scrollable.
+			if ( ! isScrollable( currentElement ) ) {
+				for ( const child of currentElement.children ) {
+					if ( isElementVisible( child ) ) {
+						const childBounds = child.getBoundingClientRect();
+						bounds = rectUnion( bounds, childBounds );
+						stack.push( child );
+					}
 				}
 			}
 		}

[t-hamano]

As you say, it might be ideal to apply this new positioning logic only to the Navigation block for now 😅

If we implement this approach, it might be better to align function comment and name to the internal logic:

const WITH_OVERFLOW_ELEMENT_BLOCKS = [ 'core/navigation' ];

/**
 * Returns the rect of the element.
 * For blocks that have elements that overflow their parent,
 * such as the Navigation block, the rect is generated that
 * takes into account the visible child elements.
 *
 * @param {Element} element Element.
 * @return {DOMRect} Bounding client rect of the element.
 */
export function getElementBounds( element ) {
	// ...
	const dataType = element.getAttribute( 'data-type' );

	if ( ! dataType || ! WITH_OVERFLOW_ELEMENT_BLOCKS.includes( dataType ) ) {
		return bounds;
	}
	// ...
}

[t-hamano]

This issue affects slider blocks for example too.

I can reproduce the same issue with the GutenSlider block with over 10,000 active installs.

https://playground.wordpress.net/?php=8.0&wp=beta&plugin=gutenslider

be0d0c05fd4b9c5342fed0a9984874da.mp4

It might be a good idea to apply the fix suggested in this comment to 6.7.1 after all. In that case, I think we will need to backport #66546 first.

[ramonjd]

@t-hamano I plagiarized your idea in #66702

Do you think that will work? Tests well for me so far.