Block Toolbar Regression - Toolbar obstructs selected blocks.

[Addison-Stavlo]

When a block is near the top of the canvas and selected, the block toolbar obstructs the block.

We previously resolved this in #29464 by moving the toolbar beneath the selected block IF there is no room in the editor canvas to position it above the block without selected block obstruction.

Some recent change has caused a regression in this behavior, trying to place the toolbar above the block regardless of available space. This causes the toolbar to float over the selected block, obstructing the user from interacting with that block or its children.

BEFORE REGRESSION:

AFTER REGRESSION (current trunk branch):

Either we need to:

• A: Re-add this conditional bottom-positioning behavior AND follow up to resolve the 1 edge case related to calculating the position when popover children are involved (i.e. navigation block submenus as reported here - 40382, 36335, 36977)
• B: add a background buffer space at the top of the editor canvas / above the template equal to the height needed for the block toolbar. (or do this until we can resolve the edge cases around the solution A)
• C: conditionally enable the "top toolbar" setting when a block at this position is selected. (or do this until we can resolve edge cases around solution A)
• D: other ideas?

The current state is extremely limiting and will not be acceptable to be in place for any extended period of time. Users are reporting inability to edit certain blocks near the top of the canvas, such as the main navigation block in their header.

cc @talldan @jasmussen @youknowriad @getdave @mtias

[Addison-Stavlo]

Any thoughts or work to resolve this issue?

This seems like it should be high priority. Every block in short height Headers in the site editor suffers from this toolbar overlapping it - logos, site titles, taglines, navigation, etc...

[getdave]

This is a regression. I will flag in the Core Editor chat to get it some more 👀

[talldan]

I'll see if I can work this one out

[talldan]

There are notably quite a few popover issues that may be related, or even the same bug - #42058, #41823, #41739.

After bisecting, this particular one does seem to be caused by #40740. I'm assuming there's supposed to be some logic to flip the toolbar to underneath the block that's missing.

edit: looks like it's this bit (thanks to whoever left the comments):

gutenberg/packages/components/src/popover/utils.js

Lines 197 to 229 in b499487

	if ( anchorRect.top <= stickyPositionTop ) {
	if ( editorWrapper ) {
	// If a popover cannot be positioned above the anchor, even after scrolling, we must
	// ensure we use the bottom position instead of the popover slot. This prevents the
	// popover from always restricting block content and interaction while selected if the
	// block is near the top of the site editor.
	const isRoomAboveInCanvas =
	height + HEIGHT_OFFSET <
	editorWrapper.scrollTop + anchorRect.top;
	if ( ! isRoomAboveInCanvas ) {
	return {
	yAxis: 'bottom',
	// If the bottom of the block is also below the bottom sticky position (ex -
	// block is also taller than the editor window), return the bottom sticky
	// position instead. We do this instead of the top sticky position both to
	// allow a smooth transition and more importantly to ensure every section of
	// the block can be free from popover obscuration at some point in the
	// scroll position.
	popoverTop: Math.min(
	anchorRect.bottom,
	stickyPositionBottom
	),
	};
	}
	}
	// Default sticky behavior.
	return {
	yAxis,
	popoverTop: Math.min( anchorRect.bottom, stickyPositionTop ),
	};
	}
	}

another edit: I'm guessing the above code will need to be converted to a floating-ui middleware. The flip middleware seems close to what is needed - https://floating-ui.com/docs/flip. I've tried a few things, but nothing has worked.

[talldan]

I tried a fix in #42086, but unfortunately that PR currently regresses another part of the toolbar positioning. I'll continue looking for a solution.

[ciampo]

After bisecting, this particular one does seem to be caused by #40740. I'm assuming there's supposed to be some logic to flip the toolbar to underneath the block that's missing.

I can also confirm that #40470 introduced the regression.

edit: looks like it's this bit (thanks to whoever left the comments):

gutenberg/packages/components/src/popover/utils.js

Lines 197 to 229 in b499487

	if ( anchorRect.top <= stickyPositionTop ) {
	if ( editorWrapper ) {
	// If a popover cannot be positioned above the anchor, even after scrolling, we must
	// ensure we use the bottom position instead of the popover slot. This prevents the
	// popover from always restricting block content and interaction while selected if the
	// block is near the top of the site editor.
	const isRoomAboveInCanvas =
	height + HEIGHT_OFFSET <
	editorWrapper.scrollTop + anchorRect.top;
	if ( ! isRoomAboveInCanvas ) {
	return {
	yAxis: 'bottom',
	// If the bottom of the block is also below the bottom sticky position (ex -
	// block is also taller than the editor window), return the bottom sticky
	// position instead. We do this instead of the top sticky position both to
	// allow a smooth transition and more importantly to ensure every section of
	// the block can be free from popover obscuration at some point in the
	// scroll position.
	popoverTop: Math.min(
	anchorRect.bottom,
	stickyPositionBottom
	),
	};
	}
	}
	// Default sticky behavior.
	return {
	yAxis,
	popoverTop: Math.min( anchorRect.bottom, stickyPositionTop ),
	};
	}
	}

On top of this logic, another interesting change is that #40470 removed the __unstableEditorCanvasWrapper prop from Popover, which was previously used to let the Popover know about the boundaries of the editor:

gutenberg/packages/block-editor/src/components/block-popover/index.js

Lines 76 to 78 in b99e668

	// Used to safeguard sticky position behavior against cases where it would permanently
	// obscure specific sections of a block.
	__unstableEditorCanvasWrapper={ __unstableContentRef?.current }

The way I see it, the Popover component should not know about the "editor canvas wrapper" and therefore it should not have any ad-hoc internal logic for it (as it was prior to #40470)

another edit: I'm guessing the above code will need to be converted to a floating-ui middleware. The flip middleware seems close to what is needed - https://floating-ui.com/docs/flip. I've tried a few things, but nothing has worked.

That seems like a good idea — in general, I'd love to implement a solution that works regardlessly of where the Popover is employed (and therefore, not tightly coupled to the "editor").

Looking at the code, the flip middleware is already used when the __unstableForcePosition prop is set:

gutenberg/packages/components/src/popover/index.js

Line 187 in fe4890f

__unstableForcePosition ? undefined : flip(),

I started tinkering and noticed another prop called __unstableShift which also triggers a floating UI middleware (shift):

gutenberg/packages/components/src/popover/index.js

Lines 201 to 207 in fe4890f

	__unstableShift
	? shift( {
	crossAxis: true,
	limiter: limitShift(),
	padding: 1, // Necessary to avoid flickering at the edge of the viewport.
	} )
	: undefined,

I ended up removing the two mentioned props:

diff --git a/packages/block-editor/src/components/block-popover/index.js b/packages/block-editor/src/components/block-popover/index.js
index 370959d1b1..74cefd54dd 100644
--- a/packages/block-editor/src/components/block-popover/index.js
+++ b/packages/block-editor/src/components/block-popover/index.js
@@ -61,8 +61,8 @@ export default function BlockPopover( {
 			__unstableSlotName={ __unstablePopoverSlot || null }
 			// Observe movement for block animations (especially horizontal).
 			__unstableObserveElement={ selectedElement }
-			__unstableForcePosition
-			__unstableShift
 			{ ...props }
 			className={ classnames(
 				'block-editor-block-popover',

and it looks like these changes may be going in the right direction (although I have no context on why these props existed in the first place (maybe they can be removed?), and I haven't really checked for other regressions):

Kapture.2022-07-12.at.14.37.50.mp4

[talldan]

@ciampo To restore the behavior that was in place before it should work a bit like this (using floating-ui terminology):

• Flip position when there's not enough space at the top of the editor canvas
• Shift during scrolling when there's not enough space at the top of the viewport (like it does right now in trunk)

On top of this logic, another interesting change is that #40470 removed the __unstableEditorCanvasWrapper prop from Popover, which was previously used to let the Popover know about the boundaries of the editor

What you mentioned here is one of the difficulties with implementing a fix. There doesn't seem to be a way to restore the previous functionality within the Popover component itself. I think writing custom middleware is the only way to fix the regression, and it would need to be external to the Popover component in BlockPopover.

[talldan]

I have something that I think works as expected now in #42086.

Turns out that I didn't need the __unstableEditorCanvasWrapper like I thought I would, I think floating ui automatically detects the canvas as the main scrolled element, and internally it seems to offset any coordinates against that element.

[ntsekouras]

I think floating ui automatically detects the canvas as the main scrolled element, and internally it seems to offset any coordinates against that element.

Noting that site editor(iframe) has different handling/nuances from the post editor we need to take into account. The scroll update is an example.

[talldan]

Noting that site editor(iframe) has different handling/nuances from the post editor we need to take into account. The scroll update is an example.

There is existing middleware that offsets the iframe position so that later middleware already has that offset:

gutenberg/packages/components/src/popover/index.js

Lines 161 to 182 in f01f981

	/**
	* Offsets the position of the popover when the anchor is inside an iframe.
	*/
	const frameOffset = useMemo( () => {
	const { defaultView } = ownerDocument;
	const { frameElement } = defaultView;
	if ( ! frameElement || ownerDocument === document ) {
	return undefined;
	}
	const iframeRect = frameElement.getBoundingClientRect();
	return {
	name: 'iframeOffset',
	fn( { x, y } ) {
	return {
	x: x + iframeRect.left,
	y: y + iframeRect.top,
	};
	},
	};
	}, [ ownerDocument ] );

I didn't notice any differences between the site editor and post editor, but let me know if you spot anything.

[ntsekouras]

There are still discrepancies between the editors that we need to figure out to work well in all cases. One of the things that still puzzles me is why in site editor the block toolbar is limited in the block's limits and in post editor(screenshot) is having the more space..

They both share the same code and the iframe offset is applied properly but still not identical behavior. In my mind figuring this out might unlock some of the problems we have, but.. 🤷

[ciampo]

I'm not understanding exactly what is the expected behavior — in particular this point here:

• Shift during scrolling when there's not enough space at the top of the viewport

Here's my reasoning: if, when there's not enough space at the top of the editor canvas, the popover flips (and therefore moves below the block), why does it also need to shift? The flip should already fix the "not enough space at the top" issue.

I checked out commit b499487 (the one immediately before the Popover refactor that caused the regression) and took a video in which I can identify the shifting behavior, but only there's not enough space at the bottom :

Kapture.2022-07-13.at.17.27.03.mp4

I don't have much context around the behavior of the toolbar's Popover, but I agree with @ntsekouras that we should aim for the same behavior across all editors (post, site...).

[talldan]

@ciampo It seems as though the shift behavior I described happens in the post editor. Try the Gutenberg Demo post as the easiest way to test:

Kapture.2022-07-14.at.10.05.10.mp4

That's the way it has worked for quite a while. The flip thing was only introduced for the site editor when it became possible to have blocks close to the top of the canvas, and maybe that has introduced a discrepancy.

[ciampo]

As stated before, I am not experience in working on the site editor or the post editor, and I'm not very experience with the Popover component either (especially after its refactor) — but here's some thoughts on how we could move forward:

• I think that, as the first step, we should debug and find out the reason why the component behaves differently in the site editor and in the post editor.
• once we understand that reason and fix it, we can work on implementing the desired Popover behavior:
  • I don't see why the Post Editor and the Site Editor would require two separate behaviors for the toolbar — we should agree on one behavior and aim at applying to both editors.
  • I think that we should try to identify the expected behavior, express it in floating-ui terms (e.g a mix of flipping and shifting), and expose it to consumers of Popover in a simple way.
  • my personal preference would be to implement a solution where we remove code from the Popover component instead of adding, making it more generic (ie. with less internal "special cases" tailored at the editor) and relying more on floating-ui
• if any specific behavior (or middleware) still had to be defined, they should be defined outside of the Popover component and of the @wordpress/components library (for example, in the block-editor and site-editor packages)

WDYT?

[talldan]

I think that, as the first step, we should debug and find out the reason why the component #42086 (comment) in the site editor and in the post editor.

I did some more delving. The root cause of the difference is the site editor using an iframe and a different scroll container for content. In the site editor, the blocks are inside the iframe and the toolbar outside. The blocks are also inside a scroll container element, and the toolbar popover outside of that scroll container.

When the content is scrolled, the toolbar popover isn't being scrolled with it, so its position has to be continuously updated on scroll:

gutenberg/packages/components/src/popover/index.js

Lines 320 to 328 in c8feb39

	// If we're using getAnchorRect, we need to update the position as we scroll the iframe.
	useLayoutEffect( () => {
	if ( ownerDocument === document ) {
	return;
	}
	ownerDocument.addEventListener( 'scroll', update );
	return () => ownerDocument.removeEventListener( 'scroll', update );
	}, [ ownerDocument ] );

This is different to the post editor where the toolbar popover is within the scroll container and scrolls with the content, no positional update on scroll is required.

I think Floating UI smoothes out some of these inconsistencies, but the code I wrote in #42086 doesn't.

I know the post editor will at some point also use an iframe for its content, so there will be more consistency. Though there's still the widget editors to consider.

Having said that, updating the toolbar position continuously on scroll like the site editor does seems like an expensive option, and it'll always prove to be a little laggy. It may be better to try changing the scroll container and make it more like the post editor—though I think this would mean dynamically sizing the iframe to its content, which is never easy.

[Greatdane]

@talldan This still happens with the navigation block, but perhaps #40382 is going to address this?

My submenu is blocked by the toolbar;

WordPress Version: 6.0.3
Gutenberg Version: 14.3.1

[talldan]

@Greatdane Yep, that one's a different issue/bug that happens because the submenus are overflowing the block container. #40382 is the correct issue that tracks it 👍

This one was a bug where any block at the top of the editor was overlapped.