Block Editor: Consider events only from DOM descendents (WritingFlow, ObserveTyping)

[aduth]

Alternative to: #19804

This pull request seeks to improve upon how events are interpreted by WritingFlow and ObserveTyping. Historically, we have often resorted to Event#stopPropagation within many components for the sole purpose of avoiding this top-level event handling, but this is problematic for a number of reasons:

• It is not obvious to implement this, and is usually added later only once the undesirable behavior has been identified.
  • Example: Fix navigation in inserter menu triggering writing flow #11088
• Event#stopPropagation is an extreme solution and often introduces a lot of unintended incompatibilities and interoperability issues. It should be avoided whenever possible.
  • Related reading: https://css-tricks.com/dangers-stopping-event-propagation/
• It introduces an undesirable implicit dependency between the component and its ancestors
  • Example: In stopping propagation, URLInput (@wordpress/components) establishes a dependency on both its editor ancestry (@wordpress/block-editor WritingFlow, ObserveTyping) and an assumption that it is shown in Popover, despite the fact it is intended to be a general-purpose component.

In some of the discussion from #19804 (comment) , it is considered that it might be possible to consider the various dialog components (Popover, Modal) as entirely separate contexts and to not allow events to bubble through to its ancestor components. In some ways, this aligns with its implementation using Slot/Fill, where the intention of rendering the dialogs elsewhere in the DOM is largely to treat it as being positioned independently from its ancestors (in practical terms, to avoid CSS cascading styles).

The implementation here achieves this by considering whether the events handled by ObserveTyping and WritingFlow originate from within its own DOM hierarchy. This is meant to be distinguished from event bubbling which occurs through the React hierarchy, and provides an opportunity for any components which do not want to be subject to this handling to avoid detection by rendering elsewhere in the DOM. In the case of popovers, this happens by virtue of the fact that they are rendered using Slot and Fill components.

Testing Instructions:

Verify that affected components continue to work as expected, notably that:

• When typing in a paragraph, the block contextual toolbar is dismissed until the mouse is moved, Escape is pressed, or a non-collapsed selection is made (ObserveTyping behaviors)
• Arrow keys traverse across blocks (WritingFlow behaviors)
• These same interactions are not affected when typing occurs within a popover
  • Example: When adding a link to a paragraph, the block contextual toolbar should remain visible while typing within the link editor.

[aduth]

To simplify review here, I extracted the function-component refactoring of ObserveTyping (3e0159a) to its own pull request at #19881.

[ellatrix]

This is great. I’ll review tomorrow if I find some time

[ellatrix]

Huh

[aduth]

🤷‍♂ #19458 (comment)

[getdave]

I noticed this and removed it in #19775.

Not sure how it happened. Sorry!

[aduth]

The failing end-to-end tests could be legitimate, based on the error message, since the new code checks DOM descendent by Element#contains:

● autosave › should save to sessionStorage
    TypeError: Cannot read property 'contains' of undefined

Not sure how the ref.current could be undefined at the point that it's handling onKeyDown, onFocus, onKeyPress. At worst, we could just add an extra check there to ensure it's truthy, but it's worth investigating why it's happening in the first place.

[aduth]

I've rebased this after merging #19881, so the ObserveTyping-specific changes should be fewer.

The test failure was quite bizarre. From debugging, the node.current.contains() condition was being triggered in response to a focus event on the initial render, caused by the auto-focus of the post title. But it's strange to me that this event handler could ever be called without the ref also being set, considering that both should take effect in the same render. It could be a timing issue where event handlers are added before the ref is assigned, and if those event handlers happen to be triggered in the meantime, there's a rare chance the ref may still be unassigned.

In any case, it occurred to me there's a better implementation here anyways, avoiding the ref altogether and simply using event.currentTarget, which should be effectively the same as what we're wanting, but more closely associated with the event and the DOM, and is slightly simpler code.

See changes in 46a8e28.

[aduth]

This isn't viable without changing how we approach navigation mode tabbing:

gutenberg/packages/block-editor/src/components/writing-flow/index.js

Lines 325 to 354 in 9e3c713

	// In navigation mode, tab and arrows navigate from block to block.
	if ( isNavigationMode ) {
	const navigateUp = ( isTab && isShift ) || isUp;
	const navigateDown = ( isTab && ! isShift ) || isDown;
	const focusedBlockUid = navigateUp ? selectionBeforeEndClientId : selectionAfterEndClientId;
	if ( navigateDown || navigateUp ) {
	if ( focusedBlockUid ) {
	event.preventDefault();
	selectBlock( focusedBlockUid );
	} else if ( isTab && selectedBlockClientId ) {
	const wrapper = getBlockDOMNode( selectedBlockClientId );
	let nextTabbable;
	if ( navigateDown ) {
	nextTabbable = focus.tabbable.findNext( wrapper );
	} else {
	nextTabbable = focus.tabbable.findPrevious( wrapper );
	}
	if ( nextTabbable ) {
	event.preventDefault();
	nextTabbable.focus();
	clearSelectedBlock();
	}
	}
	}
	return;
	}

The problem is: The Tab event originates from the toolbar button:

...which is now rendered in a Popover as of #19564. And these are the exact sorts of events we're otherwise wanting to ignore.

The issue can be reproduced following this test case:

gutenberg/packages/e2e-tests/specs/editor/various/keyboard-navigable-blocks.test.js

Lines 107 to 134 in 9e3c713

	it( 'allows tabbing in navigation mode if no block is selected', async () => {
	const paragraphBlocks = [ '0', '1' ];
	// Create 2 paragraphs blocks with some content.
	for ( const paragraphBlock of paragraphBlocks ) {
	await insertBlock( 'Paragraph' );
	await page.keyboard.type( paragraphBlock );
	}
	// Clear the selected block and put focus in front of the block list.
	await page.evaluate( () => {
	document.querySelector( '.edit-post-visual-editor' ).focus();
	} );
	await page.keyboard.press( 'Tab' );
	await expect( await page.evaluate( () => {
	return document.activeElement.placeholder;
	} ) ).toBe( 'Add title' );
	await page.keyboard.press( 'Tab' );
	await expect( await getActiveLabel() ).toBe( 'Paragraph Block. Row 1. 0' );
	await page.keyboard.press( 'Tab' );
	await expect( await getActiveLabel() ).toBe( 'Paragraph Block. Row 2. 1' );
	await page.keyboard.press( 'Tab' );
	await expect( await getActiveLabel() ).toBe( 'Document (selected)' );
	} );

Essentially, the Tab might skip over the logic to focus the next block's label, and instead circle back to the title.

A few thoughts on how to approach this:

• Move out navigation mode handling to a separate component which will handle React bubbled events.
• Render the toolbar-specific Slot within WritingFlow so it can be considered as part of the same DOM.
  • On first glance, it seems this could work, but in truth I'm not really sure what the original purpose of this custom slot is meant to be, and if there are other similar issues we might encounter with other non-toolbar Popover renderings.
• Find a way to allow native tab behavior to handle block navigation.
  • e.g. Assuming that the next block could receive focus for the tab event, maybe it can be the responsibility of the block to show/focus the navigation label when it receives focus.
• Revert Block: split out toolbar rendering #19564.
  • I assume want to avoid this, but for exhaustiveness' sake, it is technically an option.

[ellatrix]

@aduth Trying to understand the issue here: is the problem that the breadcrumb button is not part of the DOM tree or that it is not part of the React tree?

[aduth]

@aduth Trying to understand the issue here: is the problem that the breadcrumb button is not part of the DOM tree or that it is not part of the React tree?

It's a problem because it's not part of the DOM tree (it is not a DOM descendent of WritingFlow wrapper).

[ellatrix]

I see two options.

• Either we move the navigation mode logic to the breadcrumb button component (focus stays on the button during navigation, only selection changes) and we skip the content when you reach the end (the breadcrumb is located above the content).
• Or we do not set focus on button but rather on the blocks.

I find the latter more intuitive since when you're navigating content, focus also moves from block to block. This also ensures that all logic is kept within the WritingFlow component. With option one I fear that all the logic will become fragmented.

I don't really understand why focus is set on a button in navigation mode right now. @youknowriad @MarcoZehe do you remember the reasons behind that?

[youknowriad]

Or we do not set focus on button but rather on the blocks.

This is not an option for navigation mode because it breaks to have keyboard event handlers on non-button elements (div) in some windows screen readers (NVDA I think). That's the main reason the navigation mode was added a bit late.

[ellatrix]

This is not an option for navigation mode because it breaks to have keyboard event handlers on non-button elements (div) in some windows screen readers (NVDA I think).

That's strange. We currently already have keyboard event handlers on div elements (enter and delete/backspace on the block wrapper in edit mode). Does that not work then?

[youknowriad]

Does that not work then?

Probably not unless that element has a specific role. The specificity here is that this feature was specifically dedicated to a11y users so it didn't make sense for these handlers to not work on all screen readers.

[ellatrix]

In #27489, I wrote the missing piece for this PR: moving navigation logic from writing flow to the block selection button (the focussed element in navigation mode). Previously I also rewrote the typing observer as a hook in #27043 (which now only listens for events of DOM descendants). @aduth, thanks for creating this PR. :)