Group placeholder: broken keyboard interaction and duplicate props

[afercia]

Description

Seems to me this bug originates from #43496.

When inserting a Group block, the related placeholder is initially shown. Initial focus, keyboard interaction, and tooltips displaying appear to be completely broken. For example:

Initial focus is on the block wrapper. But the first button within the placeholder shows its tooltip, even if the button is not focused:

When pressing the Right arrow key, focus should go to the first button. Actually, the wrapper isn't focused any longer but the first button doesn't receive focus. Instead, focus appears to go somewhere in the middle, on a non-visible element. The first button tooltip is still shown, even if the button isn't focused:

Navigating with the arrow keys and the Tab key surfaces more unexpected / broken behaviiors. I made a GIF to better illustrate:

Root cause:

To my understanding, in #43496

• a wrapper was added around the Placeholder component
• the blockProps are passed 'as is' to the Placeholder wrapper, the only change is an additional class name
• thus, all the block props are actually set both on the block wrapper and on the Placeholder wrapper.
• as a result, the two wrappers have duplicated HTML attributes in the DOM:
  • duplicated role=document
  • duplicated aria-label
  • duplicated IDs
  • most of the data-* attributes are duplicated
  • most of the class names are duplicated
  • etc.

Other props that aren't rendered in the DOM are duplicated as well. For example the ref prop, which seems far from ideal. Not sure what may happen with the same ref passed to twoneted components.
This ref is actually a st of merged refs, including some refs that are used for hooks like useFocusFirstElement. Thus, some behaviors depending on these hooks is inevitably brokend eg. focus management.

Screenshot of the two wrappers DOM, wee all the duplicated attributes:

To my understanding, theblockProps that come from useBlockProps are meant to be exclusively used for the Block wrapper. Using these props on an extra wrapper seems less than ideal. That breaks expected interaction and semantics.

If the only goal of the changes from #43496 wa to set the class names on the extra wrapper, then only the class names should be passed.

I'd also recommend to double check if there are other cases where theblockProps are passed to extraneous components / elements.

Worth noting that in WordPress 5.9 the Group placeholder was very similar to the current one and it worked fine, simply because there was no extra wrapper:

Pinging a few people who may know more about this implementation:
@ramonjd, @tellthemachines, @youknowriad

Lastly: when a placeholder contains focusable elements, initial focus should to the first focusable element. See for example the Columns block.

Step-by-step reproduction instructions

• Add a Group block
• See initial focus is on the block wrapper (see the blue outline)
• Observe the first button within the Placeholder isn't focused but it shows its tooltip anyways.
• Press the Right arrow key.
• Observe that, visually, nothing seems to happen.
• Press the Right arrow key.
• Observe the first button is now focused.
• Play with the Right and Left arrow keys and observe buggy behaviors as illustrated in the attached GIF above.

Screenshots, screen recording, code snippet

No response

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]

Thanks for the ping.

As far as I can tell, the duplicate props/element comes from the recent addition of innerProps

@andrewserong I tested this out by removing the inner props from <View />

			{ showPlaceholder && (
				<View>
					{ innerBlocksProps.children }
					<GroupPlaceHolder
						clientId={ clientId }
						name={ name }
						onSelect={ selectVariation }
					/>
				</View>
			) }

Keyboard navigation seems to work as before and it still satisfies the dragging bug in #49256

What do you reckon?

I haven't thoroughly tested though.

Here's a test link https://gutenberg.run/49783 from a PR I threw up to try it out.

[andrewserong]

Thanks for digging in @ramonjd and for the detailed issue report @afercia!

From testing, Ramon's PR in #49783 appears to fix the issue while retaining the earlier bug fix, by only outputting innerBlocksProps.children in the placeholder state. innerBlocksProps.children was the only thing required to fix the issue in #49256, as InnerBlocks needed to be rendered in order for the blockListSettings to be updated so that the placeholder state can be treated as a container block.

That sounds like a good path forward to me!