Modal: Fix loss of focus when clicking outside

[stokesman]

What?

• Ensures that return of focus from the Modal component works when it’s dismissed by clicking outside.
• Adds a unit test to guard regressions.

Why?

Focus loss is troublesome in various ways. This fixes #51722.

How?

Uses a pointer event handler to preempt the onFocusOutside handling for dismissing the modal when clicking outside.

Testing Instructions

1. Start in the Post Editor
2. Open the Options menu
3. Open the Preferences modal
4. Click outside the Preferences modal to dismiss it
5. Observe the Options menu is still open and focus returned to the “Preferences” button.

This can be generalized to any modal/confirmation dialog in any of the block editors.

Testing Instructions for Keyboard

After step 4 of the above testing instructions you can try using editor keyboard shortcuts and observe they do work.

Unit Test Testing Instructions

1. Check out the first commit on this branch git checkout 1aaa194
2. npm run test:unit -- modal/test/index.tsx
3. Observe the new test fails
4. git switch -
5. Repeat step 2.
6. Observe the new test passes

[github-actions]

Flaky tests detected in 2750830.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5660909745
📝 Reported issues:

• [Flaky Test] should toggle "lightbox" in saved attributes #52350 in /test/e2e/specs/editor/blocks/image.spec.js
• [Flaky Test] zoom animation #52381 in /test/e2e/specs/editor/blocks/image.spec.js

[mirka]

This works as described, but it leaves me wondering why useFocusOutside() was necessary in the first place. Do you know? Since tabbing is constrained to the modal, wouldn't a click on the overlay be the only way that focus would move outside the modal?

I've even noticed that only relying on useFocusOutside to detect clicks on the overlay doesn't even work reliably when iframes are involved. For example in the Storybook in Canvas mode, if you open the modal, click outside the story iframe, then click on the overlay, it doesn't close.

It seems like a click handler on the overlay like in this PR is better and sufficient.

[stokesman]

Thanks for having a look Lena 🙇

it leaves me wondering why useFocusOutside() was necessary in the first place. Do you know?

🤓 As best I can tell it never was technically necessary. The component had an overlay element and constrained tabbing from the start #6261 so you'd think it might have used a simple click handler on the overlay. Instead it used a more generalized approach provided by an HOC of an external library. Later that was replaced by the withFocusOutside HOC #16878 and of course later the hook equivalent.

Since tabbing is constrained to the modal, wouldn't a click on the overlay be the only way that focus would move outside the modal?

Practically speaking yes. Focus can be moved programmatically but I don't think that was intentionally supported or relied upon.

It seems like a click handler on the overlay like in this PR is better and sufficient.

Well, yeah that’s #51602 which removes the hook from the Modal. That stalled mostly because there’s one case of unintentional reliance on closing the modal when focus moves outside programmatically. Also, there is some desire to update useFocusOutside so it could support iframes so that useDialog and its consumers would not have the iframe issue.

[mirka]

Thanks for explaining how this relates to the other PRs 🙏 I think it makes sense to merge this to fix the immediate problem with the focus returning then!

[mirka]

Can we add a TODO comment here? Something to the effect of "Reconcile with useFocusOutside" and "Ideally this should be an onClick".

[stokesman]

I gave it my best and hopefully it captures what you had in mind. At least regarding the first part. I left out the bit about preferring onClick. That could provide some nuance to the interaction but I think handling pointerdown will still be necessary. That’s because if the default isn't prevented from pointerdown focus will move and be left unreturned by useFocusReturn.

[mirka]

It does 👍 I'm mostly concerned that we're treating a mere pointerdown as a click intent, which isn't technically true. For example I sometimes mousedown on a button but change my mind and drag away so it doesn't go through as a click.

But I guess we could even resolve that right now, if we just event.preventDefault() on the pointerdown, and wait until the click event to actually call onRequestClose().

[stokesman]

Yep, that's just what I meant about nuance in the interaction. I do the same sometimes. To keep current behavior and this PR focused, I'd left it as is.

Since you agree it should be resolved I went ahead and tried it. It's not quite as simple as we likely both thought. It's also an opportunity to try a bit of further nuance as suggested by Aki #51602 (comment). So I've got a branch for it and will make it a separate PR.

[stokesman]

…and that PR #52994