Reach-ui: Dialog's focus lock steals focus across frame boundaries

[henningmu]

🐛 Bug report

Current Behavior

Check the CodeSandbox example linked below. When opening the Dialog by clicking Show Dialog the button in it gets focussed (so far so good). Now use the mouse to return focus to the CodeSandbox and try to type something. As soon as you begin, the focus is returned to the button in the dialog.

The problem in our setup is a bit more complex as we're opening a Google Driver Picker from a dialog but the underlying problem seems to be the same: The focus lock on the dialog is too aggressive.

Expected Behavior

The focus lock does not interfere with frame boundaries (ie. doesn't steal focus from iframes or the like).

Reproducible example

CodeSandbox

Suggested solution(s)

At first I wanted to propose to make this configurable through a prop like disableFocusLock. However, given the extreme example of the CodeSandbox being rendered unusable maybe there is a more holistic solution to this. Maybe this is even an issue in react-focus-lock?

Happy to help implement something after a pointer in the right direction 🙌

Your environment

| Software | Name(s) | Version |
| ---------------- | ------- | ------- |
| Reach Package | @reach/dialog | latest (0.10.0) |
| React | | 16.12.0 |
| Browser | | Chrome |

_Edit: This might be related to #83_

[chaance]

Interestingly this isn't an issue in Firefox 🤔 but this seems like a good use for FocusLock's whiteList prop. I'm not sure that simply exposing it directly via the DialogOverlay is the right API for us. See line 142 in Dialog.tsx: https://codesandbox.io/s/whitelist-focus-lock-for-reachdialog-8nmb1

@theKashey Have you any thoughts? I wasn't around back when Ryan implemented react-focus-lock so I'm still learning some of the nuances in its API.

[theKashey]

I've experienced exactly this codesandbox issue during focus-lock development - while it is active the rest of application is basically 💀dead💀.

And it is a bug.

There are a few moments:

• this is how FocusLock should work. You __asked to lock the focus__, so here you go.
• if you nest iframe inside a lock - it will understand that it is still within the boundaries and work as expected. So it have to work across frame boundaries
• when you move focus out of the page - it shall not prevent it. However there are special treatment for focus when it "goes back".

The sequence is the following:

• focus allow body to be an active element
• there is onBlur event on the document, which sets focusWasOutsideWindow=true
• if body is active, and focusWasOutsideWindow is true - this means what we reactivated current tab, and __focus has to be restored__.
• we are here.

If focusWasOutsideWindow would be false - no focus stealing would occur, and the problem here is that the page is still the same... and React elements receives blur as well and triggers the check.

And here is the problem - document.onBlur sets focusWasOutsideWindow synchronously, while trap.onBlur works asynchronously(Promise + setImmediate). If only the would be in the same conditions - focusWasOutsideWindow would not be set by the time onBlur handler is called.

Should be easy to fix the problem, as well as create a test for it. Thanks for calling me 👍

[chaance]

Even better, and thanks for the detailed explanation @theKashey!

[theKashey]

I am adding new crossFrame option, right now defaults to true, as long as this behaviour might introduce a breaking change for someone.

• setting crossFrame=true will work "as today". Focus will __try to keep focus__
• setting crossFrame=false will work "as proposed". Iframes would be treated as "pages", and let you move focus outside.

I am not sure which behaviour is actually correct - iframes are usually a part of a page, a part of your product, and the only exemptions are non-production examples like code-sandbox.

[theKashey]

Said option was added in [email protected]

[chaance]

@henningmu See https://github.com/reach/reach-ui/commit/de0655b2de8698f5e7add2e4507dee5062750b36#diff-ca1b0a6f0c85b5110ece085b9f18ca0bR304

[henningmu]

Nice one @chancestrickland (and @theKashey) that fixes our problem, thank you very much 🎉 🙌

[chaance]

Added the unstable_lockFocusAcrossFrames prop in 0.10.1