reactjs / react-modal

Accessible modal dialog component for React
http://reactcommunity.org/react-modal
MIT License
7.34k stars 808 forks source link

Stacked/nested modals have focus lost in Safari #801

Open sufian-slack opened 4 years ago

sufian-slack commented 4 years ago

Summary:

When nested Modal elements are open, focus behaves strangely in Safari. I believe that the behavior is that any tab or shift-tab will cause focus to elements in the outermost modal, not in the inner (top) modal.

Steps to reproduce:

Using Safari 13.1.

Here's a minimal repro case: https://codesandbox.io/s/distracted-napier-s7b57

  1. Tab to the button labeled "Click to toggle outer modal"
  2. Tab to the button labeled "Click to toggle inner modal"
  3. Press tab or shift-tab, notice that the focus is not moving
  4. Click on the input element to move focus to the inner modal's input element.
  5. Press tab or shift-tab, notice that focus has been lost (and moved to the outer modal)

Expected behavior:

I'd expect the focus lock to prevent focus from leaving the innermost modal. This is how it works in Chrome and Firefox (I have not verified behavior in IE/Edge).

Link to example of issue:

Minimal repro case: https://codesandbox.io/s/distracted-napier-s7b57

Additional notes:

pleunv commented 2 years ago

I ran into similar issues the past few days. As far as I can tell this happens when your modals are nested in the React tree, i.e.

<Modal>
  {showInnerModal && <InnerModal />}
</Modal>

A tab event on the inner modal will be bubbled up by react to the outer modal as well, and cause a scopeTab() to be triggered twice, first inner modal, then outer modal. Subsequently, the "safari radio button workaround" code (here) then causes the outer modal to be focused in Safari, but not in other browsers.

If you can structure your modals to not be nested I think this issue does not occur. However, this is not always feasible (nor is it for us):

<>
  <Modal />
  {showInnerModal && <InnerModal />}
</>

I think putting an event.stopPropagation() in the handleKeyDown here should fix it, but even then focus in Safari does not seem to be working reliably, but I still have to dig into it a little further (debugging focus code in Safari is a bit... erhh...). One thing worth noting is that the focusManager's modalElement is always undefined due to being passed an undefined this.node in the open() handler, here. Not sure if this has anything to do with the other broken focus behavior I'm noticing. Perhaps someone more familiar with the codebase can chime in.

s-robertson commented 2 years ago

The fix mentioned by @pleunv worked for me and I've opened a PR to prevent propagation on that event.

I was noticing other focus issues in Safari such as the nested modal's content div losing focus after the modal opens, but it seems to be a separate issue that I haven't managed to track down yet.