Avoid triggering WritingFlow when the containing element is aria-hidden

[talldan]

Description

Extracted from PR #18014

There's an issue that occurs in live/master with the Block Navigator in the Navigation Block. When this modal is open, if you use arrow-keys, the modal closes instantly.

What's happening is that the WritingFlow component's onKeyDown event is being triggered. It thinks the user is navigating around or between blocks and causes a change of focus. That change of focus closes the modal.

One solution would be for the modal to preventDefault on arrow key navigation. But this is a very specific problem that only occurs when a modal is launched from a block. It'd mean sprinkling this kind of code on every modal implemented as part of a block.

Instead, I think this is really a bug with WritingFlow—it shouldn't be handling key events when it's in the background/aria-hidden. This PR adds an if statement that returns early from the onKeyDown event if one of WritingFlow's parents is aria-hidden, which is what happens when a modal is opened.

How has this been tested?

Test this using PR #18014

1. Add a navigation block
2. Open the Block Navigator from the navigation block's toolbar
3. Press an arrow key
4. Observe that the Block Navigator modal should stay open
5. Close the modal
6. Add a few more blocks
7. Navigate between them using arrow keys
8. Observe that navigation continues to work as before
9. Press escape to enter navigation mode
10. Use arrow keys to navigate up and down between blocks
11. Observe that this continues to work.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[youknowriad]

aria-hidden can be false right?

[youknowriad]

For me ideally this is not the responsibility of the WritingFlow component because imagine other similar components handling keyboard interactions but more of the component applying aria-hidden. I don't know if that's possiible but we have a "Disabled" component to prevent interactions like this, why not use it instead of just adding aria-hidden.

[aduth]

I'd tend to agree. I don't really see the connection between aria-hidden and whether events are handled by this component. Part of this is that I don't really understand why there would be typing happening inside an aria-hidden element (why is it aria-hidden? is it not an accessibility concern that we have interactive content which is hidden from users of assistive technology?).

Related: https://www.w3.org/TR/using-aria/#fourth

Applying aria-hidden to a parent/ancestor of a visible interactive element will also result in the interactive element being hidden, so don't do this either:

[ellatrix]

Should there be focus is an element that's hidden? To me it sounds like the problem is that there's still focus in this element, while probably there shouldn't be?

[talldan]

Part of this is that I don't really understand why there would be typing happening inside an aria-hidden element (why is it aria-hidden? is it not an accessibility concern that we have interactive content which is hidden from users of assistive technology?).

The modal adds aria-hidden to other content when it's opened using this ariaHelper.hideApp.

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

Lines 84 to 87 in c079120

	openFirstModal() {
	ariaHelper.hideApp( parentElement );
	document.body.classList.add( this.props.bodyOpenClassName );
	}

To me, it doesn't seem right that WritingFlow is focusing elements that are aria-hidden.

Should there be focus is an element that's hidden? To me it sounds like the problem is that there's still focus in this element, while probably there shouldn't be?

It's not that there's focus on a hidden element. Instead the modal bubbles keyboard events up to WritingFlow. The modal is rendered in a portal, but events still bubble up through portals. Perhaps the solution is to stop that happening, it just seems like we're adding stopPropagation pretty liberally at the moment on block UI that uses popovers and modals.

This could be where I'd add stopPropagation:
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/modal/frame.js#L69-L73

[aduth]

while for the two others, you're still expected to be able to interact with other parts of the UI.

I'm a bit confused on this point. When would we expect interactions elsewhere when a Popover is opened? I mean, I grant that it makes sense that those things are still reachable, but the dialog remains open only so long as focus remains within it. It ceases to remain relevant as soon as interactions (focus, clicks) occur elsewhere. As far as that impacts the implementation, we close it.

I think it's an important point to clarify though, since it's especially relevant for what I'm proposing.

The whole idea with stopPropagation at the Popover is under the assumption that, when you're typing within a popover (like in a link input), you are explicitly not interacting with the rest of the page. And that's the primary reason I think it's acceptable to stop the propagation.

[aduth]

Alternatively, we could do something more like what was originally proposed in this pull request, where we reverse how the event is considered to be handled by ObserveTyping, WritingFlow, etc:

Rather than stop propagation of the event from the popover...
...we consider from those wrappers whether the event occurred within its own DOM hierarchy.

This would optimize for an assumption that, if events aren't meant to be handled by these wrappers, then they should occur elsewhere in the DOM.

It's maybe a strange requirement, since the default behavior of Popover will render its contents in-place (source), and this is intended to be a valid usage. But even then, it still seems like we would not want events to be handled outside the context of the Popover. There are interesting parallels in considering that the fact we use Slot/Fill in the first place is largely to avoid conflicts in the real-world implications of in-place rendering (where CSS positioning is difficult taking into account ancestor positioning). Event bubbling behavior is not too much unlike this, so considering "rendering outside the DOM hierarchy" as the solution to "I don't want events to be handled by ancestors" fits under the reasons we use Slot/Fill already.

[aduth]

Rather than stop propagation of the event from the popover...
...we consider from those wrappers whether the event occurred within its own DOM hierarchy.

See proposed implementation: #19879

[youknowriad]

I'm a bit confused on this point. When would we expect interactions elsewhere when a Popover is opened? I mean, I grant that it makes sense that those things are still reachable, but the dialog remains open only so long as focus remains within it.

When a popover is opened, you can click on a button outside the popover to trigger it. it's not the case for modals.

[youknowriad]

I think your PR is good regardless because these two components should work on the DOM tree not the React Tree (I attempted the same for WritingFlow at some point but failed).

That said, this doesn't mean the "Modal" shouldn't stop propagation. I don't see these two as mutually exclusive.

[ellatrix]

I don't quite follow. Pressing arrow keys in a modal triggers writing flow? Where is this component rendered? In the block itself? Maybe a solution would be to render it outside the block?

[talldan]

Closing in favour of #19879