[TrapFocus] Restore focus issue

[ljones-i-nexus]

• [x] The issue is present in the latest release.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior

Modal component when pressing escape, focus is not being correctly restored to clicked element. Clicked element is an IconButton wrapped in a Tooltip.

Focus does not trigger expected highlighting of button and tooltip popup. Moving to a different tab and moving back to the page restores focus as expected.

Expected Behavior

Focus should be restored with a highlighted button and open Tooltip.

Steps to Reproduce

https://codesandbox.io/s/modal-focus-issue-ge4pr?fontsize=14&hidenavigation=1&theme=dark

1. Open either modal
2. Press escape

Alternate: 1.Open either modal

2. Press escape
3. Change tab and change back

Your Environment

Using Typescript and Hooks but clearly exists with vanillajs

[oliviertassinari]

Confirmed, I remember looking at this issue in the past. I couldn't understand exactly what was going on. From what I recall, the focus event wasn't firing (🙈), leading to a wrong focus visible style. Introducing a setTimeout in the .focus() call was solving the problem (🙈). In any case, the DOM focus is handled correctly, the UI feedback is not.

[ljones-i-nexus]

Thanks for looking into the issue quickly. Currently switched to using the Dialog component which restores focus correctly.

[eps1lon]

We need to entangle what you mean by "focus not being returned". There are two levels of focus that are relevant here: visible focus and (standard) focus.

The standard focus is behaving as expected (otherwise please include the operating system you are using i.e. do you use a mac or not).

What I see is not behaving as expected is visible focus. Even when only operating the codesandbox with a keyboard the the visible focus is not returned to the button (it is focused though). If I switch tabs it does get visibly focused though.

It seems like an issue with the tooltip integration. Should be possible to write an integration test using Button, Modal and Tooltip and debug what's happening.

[ljones-i-nexus]

I agree this is an issue with "visible focus" rather than focus being returned to the element. However it doesn't seem to be a tooltip issue.

I have duplicated a button on the sandbox linked above and removed the Tooltip wrapper and the issue still exists.

[fyodore82]

@ljones-i-nexus , please look at screenshot below and confirm, that I understand correctly

[ljones-i-nexus]

@fyodore82 Correct, the actual element is in focus after pressing ESC, but the focus is not visible.

[fyodore82]

Seems this issue is related to this line of code

if (nodeToRestore.current && nodeToRestore.current.focus) {
    nodeToRestore.current.focus();
}

TrapFocus component tries to restore focus when a modal is still opened. But a browser will not put focus on invisible elements. (somewhat related to this)

Adding setTimeout fixes this issue, but I don't think it is a good solution, so I'll continue investigation.

if (nodeToRestore.current && nodeToRestore.current.focus) {
    const btn = nodeToRestore.current;
    setTimeout(() => { btn.focus(); }, 100);
}

[oliviertassinari]

@fyodore82 It sounds like you have reached the point where I have stopped my investigation. It would be awesome if you could crack the case :). For instance, why does it work with the Dialog and not with the Modal? Why calling .focus() doesn't trigger a focus event?

[embeddedt]

Perhaps it's related to the fact that the Dialog has an animation built-in. If the function with these lines of code is delayed by the animation, that would be similar in timing to calling the setTimeout.

[fyodore82]

@oliviertassinari Thank you for mentioning Dialog component. @embeddedt , yes Fade component makes it work. Just adding Fade as child to Modal makes Modal return focus.

<Modal open={showLocalModal} onClose={() => setShowLocalModal(false)} >
  <Fade in={showLocalModal}>
    <div/>
  </Fade>
</Modal>

[fyodore82]

Seems to be a bug in React. Filed it

[oliviertassinari]

This comment https://github.com/facebook/react/issues/17894#issuecomment-656094405 suggests that the observed bug here will be fixed in React 17.

[oliviertassinari]

This bug is fixed in React 17.0.0 https://codesandbox.io/s/o6isx?file=/demo.tsx. It's definitely broken in React 16.14.0 https://codesandbox.io/s/basicmodal-material-demo-forked-g3y2p.

The documentation will no longer showcase the problem once we migrate the version of React we use. For now, our docs still have the issue: https://next--material-ui.netlify.app/components/modal/#basic-modal.

@eps1lon Should we close it now, or wait for the docs to upgrade to use React 17?

[eps1lon]

@eps1lon Should we close it now, or wait for the docs to upgrade to use React 17?

We first have to revisit what the plan is regarding supported React versions. I've been pushing for a single version from the beginning but you've always wanted to support multiple versions.

Is this still your opinion or have you reconsidered after realizing that supporting older versions also means supporting buggier React versions?

[oliviertassinari]

We first have to revisit what the plan is regarding supported React versions.

@eps1lon What do you think about upgrading the dev dependencies to React 17, and keep the current peer dependency scope for React?

I've been pushing for a single version from the beginning but you've always wanted to support multiple versions.

I personally think that:

• we should mostly only care about the latest version (17.0.1 now)
• accepting React 16.8 in the peer dependency comes with almost no cost (we can systematically tell developers to upgrade if the bug is outside of our control). No need to run tests with it.
• running a nightly test suite with React@next is helpful

[eps1lon]

accepting React 16.8 in the peer dependency comes with almost no cost (we can systematically tell developers to upgrade if the bug is outside of our control). No need to run tests with it.

It is a huge cost right now.

[oliviertassinari]

It is a huge cost right now.

@eps1lon In this case, I'm missing context. What cost do you have in mind? I can only think of one:

Developers raising something not working with 16.8 and us telling them: it's fixed with React 17.0, upgrade. This can create some support overhead for us. But developers would likely raise it anyway regardless of what or peer dependencies say (I'm pretty sure they most ignore peer dependencies). It could also harm the trust in the project, depending on what level of expectations developers have for when we say we support older versions of React. My counter argument is that saying it's outside of our control is fair and can be understood by developers.

I honestly don't feel strongly about this, I think that supporting React 17 only could be good enough. It's easy to upgrade. We can even use my previous argument against me: developers often ignore peer dependencies.

[mnajdova]

Now that we have react@^17.0.0 as a peer dependency, I believe we can close this. I've checked the original sandbox, and I can no longer reproduce the bug, as the latest version of react is this one.