[Modal] Better focus management

[theKashey]

Current(master) focus-management is broken for shift+tab, while forward tapping is also working not as expected, as long as you may leave a modal, and got into browser address line, which is good, but should be impossible.

A new TrapFocus implementation also does not seems to be correct, especially due to the absence of RootRef, used to discover nested portals (like here - https://nk7zmp3900.codesandbox.io/)

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

• loop then tabbing forward
• loop then tabbing backwards

Current Behavior 😯

• loop then tabbing forward, with focus on body and modal after leaving browser address bar - causing 1 or 2 tabs without visible response.
• does not loop backwards. A new TrapFocus would probably fix it, but might introduce other issues.

Steps to Reproduce 🕹

See an example with 3 buttons inside a modal - https://codesandbox.io/s/j1ll74o3v9

Context 🔦

Sorry, I have a professional deformation :) I've invested too much time in focus management, and all focus-related problems are visible to me.

Proposed solution

Replace TrapFocus by react-focus-lock, which would handle all edge cases including Portals. There is only one downside of it - +4kb.

[joshwooding]

TrapFocus was released in @material-ui/core@v4.0.0-alpha.1 (next) do you still have the same problems with newer versions? It would be good to hear about your experience with Material-UI and focus accessibility :)

[theKashey]

next fixed experience, but now I could not tab into address bar, which was useful before (except for Material-UI only React-bootstrap supported it). Removing last sentinel might keep the old experience.

There is still a problem with portaled content, which should be also wrapped with TrapFocus, like Selects, or be inaccessible.

[oliviertassinari]

Current(master) focus-management is broken for shift+tab, while forward tapping is also working not as expected, as long as you may leave a modal, and got into browser address line, which is good, but should be impossible.

@theKashey Thanks for caring about the problem! I have benchmarked a lot of different UI libraries regarding focus handling. Very little are handling the focus correctly. I hope that #14545 solves it. I have also benchmarked a lot of correct implementations, a few of them are listed in #13702 (comment).

next fixed experience

Thank you for the confirmation. I was planning on making TrapFocus public post v4, but I wanted to stress test the implementation first. Let's keep track of the bundle size: #15453.

now I could not tab into address bar

I have implemented the logic with this constraint in mind. I trust these implementations for caring about the details, they don't allow it, I don't think that we should either:

1. https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html
2. https://jqueryui.com/dialog/
3. https://ui.reach.tech/dialog

There is still a problem with portaled content, which should be also wrapped with TrapFocus, like Selects, or be inaccessible.

Do you have an example? I have taken your codesandbox and updated it with v4.0.0-alpha.8. It's good: https://codesandbox.io/s/2zm2qzl6jp.

There is only one downside of it - +4kb.

+4kb is a problem for us. Material-UI has these points of interests:

• Provide many ready to use components.
• Have an overhead as small as possible so it can be used to build any design.
• Provide world-class accessible components.

[theKashey]

I have implemented the logic with this constraint in mind. I trust these implementations for caring about the details, they don't allow it, I don't think that we should either:

It's more about technical constraints - old behaviour is only possible if the modal is the last element on the page. That's usually just (was) not possible.

Do you have an example? I have taken your codesandbox and updated it with v4.0.0-alpha.8. It's good: https://codesandbox.io/s/2zm2qzl6jp.

Let's imagine you will use something non-MUI based. Anyway - that's easy to fix - just add "react" onFocus handled to a top node, and listen to events React will send to you. Once you have an event with a target not seen via react handlers - activate the lock. Ie just replace current .contains logic. Absolutely the same logic would work for scroll management.

To mitigate another problem with tabbing from address bar - #13702 (comment) - just add one more sentinel with tabIndex=1 (the "first" tabbable element on a page). Plus - I recommend style them a bit, to make them hidden and never affect scroll by any reason.

export const hiddenGuard = {
  width: '1px',
  height: '0px',
  padding: 0,
  overflow: 'hidden',
  position: 'fixed', // <-- that's the trick
  top: '1px',
  left: '1px',
};

[oliviertassinari]

It's more about technical constraints - old behaviour is only possible if the modal is the last element on the page. That's usually just (was) not possible.

@theKashey The focus shouldn't move to the address bar. It goes against the accessibility guide: https://www.w3.org/TR/wai-aria-practices-1.1/#dialog_modal. But it seems people disagree on this point: Accessible Modal Dialogs -- A11ycasts #19. It could be an opt-in prop.

Tab: Moves focus to the next tabbable element inside the dialog. If focus is on the last tabbable element inside the dialog, moves focus to the first tabbable element inside the dialog.

Our modal has an intermediary step where the focus moves to the Modal container. It was a tradeoff to win 1kB gzipped. I would be happy to revisit it.

Let's imagine you will use something non-MUI based.

I'm sorry. Could you provide a more detailed issue reproduction? I don't understand the problem you are trying to solve. Let's go back to the codesandbox provided in

See an example with 3 buttons inside a modal

I have updated it to use v4.0.0-beta.8: https://codesandbox.io/s/w7wmvnlxp7. Do you see an issue?
I'm aware of two problems:

• address bar, tab => scroll change: [Modal] Scroll changes when focus from browser URL bar #14895. Most modal implementations has this behavior. It's probably fine to ignore. It helps to reduce the bundle size and might actually be a good thing, if you start from the address bar, you might want to reset the scroll too. 🤔
• modal tab loop => focus the container each time. It was a tradeoff to save the usage of tabbable. When you look at preact size, 1kB starts to matter. But I'm eager to simplify it.

[theKashey]

address bar, tab => scroll change: #14895. Most modal implementations has this behavior. It's probably fine to ignore. It helps to reduce the bundle size and might actually be a good thing, if you start from the address bar, you might want to reset the scroll too. 🤔

As I mentioned in the Modal PR - just add one more sentinel withposition:fixed + tabIndex=1 to handle this situation.

modal tab loop => focus the container each time.

Yeah, that's a tricky part. You might list all nodes between start and end sentinels and .focus() them until activeElement changes. That would solve an issue and requires just a few bytes of code.

I have updated it to use v4.0.0-beta.8: https://codesandbox.io/s/w7wmvnlxp7. Do you see an issue?

The issue is possible only if you are using non-MUI components. I've added reach-ui/menu-button, which would be accessible outside modal, but completely inaccessible within a modal.

[eps1lon]

You might list all nodes between start and end sentinels and .focus() them until activeElement changes.

What happens with focus(in|out) listeners on those nodes? Or would a dispatch of a focus event imply that activeElement has changed i.e. it wouldn't fire to many focus events?

[theKashey]

No event would be dispatched if they are not focusable.

[oliviertassinari]

I have lost the flow of this issue. Do we still have a specific problem so solve for this one?

[theKashey]

AFAIK - waiting for React Flare.

[eps1lon]

AFAIK - waiting for React Flare.

Same

[geoffhuntsgood]

Apologies for commenting on a dead thread, but this is the only place I've really found a reference to this issue. Is there going to be further discussion about the modal itself being focused when tabbing? I'm working on a site, and this issue was raised during 508 testing. If it's impossible to "skip" the modal itself when tabbing, is there some way to visually indicate that the modal itself is in focus? I've tried adding outlines to every generated class with no luck. Would you happen to have any recommendations for implementing the "tabbable" library?

I'm implementing a Dialog component, and here's the element that's active when the modal is in focus.

[oliviertassinari]

@geoffhuntoon see #19651 and #23827.