Navigation block: requires double clicking to select/edit when using page list

[annezazu]

Description

The "click to edit" overlay #35079 seems to be broken causing one to have to double click in quick succession in order to edit a navigation block. This seems specific to a post theme switch from a classic theme when the page list fallback is in place as I can't replicate when a "regular" navigation block is in place. cc @getdave

Step-by-step reproduction instructions

1. Use 6.1 RC2 on a site.
2. Convert from a classic theme to block with one menu in place.
3. Try to select and edit the navigation block.

If you need a test site to make it easier, you can spin up a site with https://app.instawp.io/launch?t=fse-call-for-testing-17 from the last call for testing for the FSE Outreach Program. This will give a classic site. Uninstall Gutenberg, update to 6.1 RC2 via beta tester, then switch to TT3.

Screenshots, screen recording, code snippet

Here's a video showing the problem just after switching from a classic to block theme and needing to convert the page list links to blocks:

click.to.edit.needing.double.click.mov

I can't replicate after converting to links:

Uploading click to edit post conversion.mov…

Environment info

• WordPress 6.1 RC2
• TT2
• No Gutenberg
• Chrome

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

[getdave]

Tested and confirmed this is a bug on RC2. You cannot select the Page List at all without double click.

[getdave]

I can confirm that turning off Overlay causes this issue to disappear. Therefore I believe this is the overlay interfering.

[getdave]

Ok here I am testing on Gutenberg trunk.

• initially I cannot click on the Page List in the canvas to select it
• then I toggle the overlay to off.
• now I can click in the canvas to select.
• now I toggle the overlay back to Mobile again.
• now I can click in the canvas to select (this is in contrast to the initial experience).

Why is this happening? What changes? Diving deeper....

Screen.Capture.on.2022-10-19.at.12-08-02.mp4

[getdave]

I can observe the same behaviour using the following markup:

<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:page-list /--></div>
<!-- /wp:group -->

Therefore I don't think this is specific to the Navigation block but rather the Page List block.

[getdave]

Narrowing this down some more. It seems that the Page List block as an inner block is not receiving the focusin event under certain conditions.

This is causing the onFocus callback of useRefEffect not to run for the Page List block. Instead it just runs for the wrapper block.

However, some changes in the editor seem to "trigger" the focus to work as you'd expect for the Page List. One example is simply going in and then out of Code Editor view. After doing that the focus works to allow you to select the inner Page List directly. See video:

Screen.Capture.on.2022-10-19.at.15-58-57.mp4

My current thinking is that something is blocking focusin from firing. Is it some piece of DOM overlay or a quirk of the JS... 🤔

[getdave]

Going deeper. The focusin handler is not registered on the Page List <ul> element. It appears is is removed by the cleanup function returned by useFocusHandler.

Before toggling Code Editor mode

Notice that the only event handler is that on the parent Group block.

After toggling Code Editor mode

Notice that the new handle on the <ul>.

[getdave]

I now believe the following

• the clean up function of useFocusHandler is running for the Page List node.
• useRefEffect will run it's cleanup if the ref changes.
• the "ref" is passed via useMergeRefs here (notice that useFocusHandler is one of the hooks):
  

  gutenberg/packages/block-editor/src/components/block-list/use-block-props/index.js

  Lines 124 to 140 in 88a1f7a

  	const mergedRefs = useMergeRefs( [
  	props.ref,
  	useFocusFirstElement( clientId ),
  	useBlockRefProvider( clientId ),
  	useFocusHandler( clientId ),
  	useEventHandlers( clientId ),
  	useNavModeExit( clientId ),
  	useIsHovered(),
  	useIntersectionObserver(),
  	useMovingAnimation( {
  	isSelected: isPartOfSelection,
  	adjustScrolling,
  	enableAnimation,
  	triggerAnimationOnChange: index,
  	} ),
  	useDisabled( { isDisabled: ! hasOverlay } ),
  	] );

• by process of elimination if I removed useIntersectionObserver from the merged refs the ref remains stable and the thus the callback of useFocusHandler does not fire. This means the focus functionality works for Page List.

Therefore I believe the issue may lie in useIntersectionObserver or some quirk of how Page List interacts with that hook.

EOD for me here but can pick up again tomorrow.

[annezazu]

Amazing job continuing to sleuth. Thank you.

[getdave]

I think I now have a fix at #45143. Full explanation in that PR.