[FocusTrap] Trap to only focus on tabbable elements

[eps1lon]

I reverted #23364 which is harmful and was merged without having a full picture.

Leaving this issue for visibility until the issue is transparently discussed.
Quick summary why #23364 does not work (or any DOM API approach for that matter):
What is click or keyboard focusable is up to the user agent by specification:

or the element is determined by the user agent to be focusable;

-- https://html.spec.whatwg.org/multipage/interaction.html#focusable-area

This means that any browser is free to break the behavior #23364 implemented. Specifically, #23364 does not support scrollable containers that are keyboard focusable with Chrome 87 on Ubuntu 20.04

Current implementation: https://d7r30.csb.app/
PR (broken) implementation: https://y8e9h.csb.app/

Forward tabbing loops between the two buttons. With the previous implementation all elements were accessible.

The PR being better than nothing is flawed since it replaces (potential) confusion with not being able to access elements at all. I don't think that a UI with excess elements is less accessible than a UI with completely inaccessible elements.

Further reading:

1. https://blog.whatwg.org/focusing-on-focus

[gregnb]

This means that any browser is free to break the behavior #23364 implemented. Specifically, #23364 does not support scrollable containers that are keyboard focusable with Chrome 87 on Ubuntu 20.04

Current implementation: https://d7r30.csb.app/
PR (broken) implementation: https://y8e9h.csb.app/

Forward tabbing loops between the two buttons. With the previous implementation all elements were accessible.

@eps1lon The only difference I see between the current implementation and the PR implementation is when testing is if hit tab again after the 'Cancel' button I land on the sentential. Define 'all elements' please. Because I am not seeing any other element that was accessible being no longer accessible in the PR other than the sentential

[oliviertassinari]

@eps1lon Thanks for providing more details on the limitations of #23364. This helps evaluate the tradeoff.

So we have two downsides with the proposed approach:

1. false-negative. We can miss tabbable scroll containers. I could find Clarify focusing behavior of scrollable area whatwg/html#2851 and https://www.chromestatus.com/feature/5231964663578624 on this topic. It seems that it's something Firefox has been doing for a long time (at least since 2017). I can't reproduce on Chrome on Windows and macOS (Linux has <2% of the desktop usage). I have asked about it on Scroll containers focus-trap/tabbable#167.
   From what I understand: 1. developers can hedge it by manually setting a tabIndex, 2. end-users can manually overcome it by tabbing in the opposite direction. 3. wai-aria discourage focusing on large scrollable areas. 4. this behavior affects <10% of the end-users on desktop (~8% Firefox + ~2% Linux, so far). 5. the TrapFocus could inspect the DOM to find them (once more user agents do it).
2. false-positive. On Safari macOS buttons aren't tabbable. With the current approach on YouTube's modals, the code focuses the x icon button when it should have skipped it.
   From what I understand, it's OK-ish, we get from time to time questions about why MUI's Button isn't tabbable.

On the other hand, we have two downsides with the current approach:

1. It loses track of where the focus is. It's hard to make the modal container focus indicator visible. The modal usually have a border on their own. This needs to be done with a :focus-visible logic. If you don't, you either get this crappy UX: [DatePicker] Strange black border in the calendar #23532 or the focus visible is not visible at all. The focus-visible logic is hard to get right and has to be repeated every single time you need it. It's easy to give up on it (unless we decide to create a proper hook abstraction to make it easier).
2. It floods screen readers. Modal containers usually contain a lot of content that we don't want screen readers to read.

Focusing the container is discouraged by WAI-ARIA:

---

@gregnb do you have more details on the negative aspects of the current logic that were reported by the FAAMNG? I don't understand the point about the sentinels, it seems fine. Both approaches use them. What problem did these guys catch? Are they aware of the downsides of the approach we implemented?

From my perspective #23364 is a net positive, and that's coming from the one who didn't want to implement it initially in #14545 to save bundle size. It makes the Modal +12% bigger. My imagination fails to see the alternatives available. Thoughts?

[gregnb]

@gregnb do you have more details on the negative aspects of the current logic that were reported by the FAAMNG? I don't understand the point about the sentinels, it seems fine. Both approaches use them. What problem did these guys catch? Are they aware of the downsides of the approach we implemented?

@oliviertassinari the issue is landing on the sentential and then having to hit tab again. Both implementations are fine using two sentential to create a zone, but the current implementation causes a user to land on the sentential and then nothing happens. They have to hit tab again vs the PR which then simply forwards a user to the first tabbable element.

Let me share a sample of what these defects read like:

"Users who rely on Keyboard for Navigation are getting impacted as the focus is moving on to the Hidden Element after the while navigating using TAB key. User will get confused as the focus is moving on to the hidden element and focus indicator is not visible on any element. "

Reference: https://www.w3.org/WAI/WCAG21/Understanding/focus-order.html"

[oliviertassinari]

the current implementation causes a user to land on the sentential and then nothing happens.

@gregnb Wait, what, How can it be reproduced? It was never designed to behave like this (nor I have ever seen it behave like this). Maybe the observation is wrong and assumed because there is no focus ring on the Modal container (which resonates with the pain of #23532)

[gregnb]

the current implementation causes a user to land on the sentential and then nothing happens.

@gregnb Wait, what, How can it be reproduced? It was never designed to behave like this (nor I have ever seen it behave like this). Maybe the observation is wrong and assumed because there is no focus ring on the Modal container (which resonates with the pain of #23532)

I believe you are right and I am wrong. Observation is wrong in a way but the details in the ticket are what we are suffering from

#23532 (comment)

I see you have the following suggestions from that thread:

1. ✅ We remove the container from the focus ring #19651, if it gets focused, we move it to the first focusable element inside the popup. It will help with a11y at the same time.
2. We only trigger the outline when the modality is keyboard, we have a useFocusVisible.js helper in the core.

[oliviertassinari]

@gregnb Ok, so if we ignore the technical aspect and focus on the end-user issue reported by the FAAMNG, it's about end-users losing track of where the focus is, downside 1/2 of the current approach:

User will get confused as the focus is moving on to the hidden element and focus indicator is not visible on any element.

[oliviertassinari]

@eps1lon What do you think about this middle ground solution?

1. We abstract the focusVisible hook logic, it's currently way too hard to use.
2. We use the abstraction in the Modal.

It might solve @gregnb's FAAMNG issue (it needs to be checked because it's not perfect) and definitely solve #23532. We would still have two downsides:

• The focusVisible style will be hard to identify, large modal, existing modal borders can harm its visibility.
• Screen readers might flood users when reading the content of the whole container.

[gregnb]

• Screen readers might flood users when reading the content of the whole container.

Kind of worried about this part tbh. I wonder how that is going to play out

[joshwooding]

• Screen readers might flood users when reading the content of the whole container.

Kind of worried about this part tbh. I wonder how that is going to play out

This is something that was raised to me personally when taking part in a wcag 2.1 audit at work. It’s definitely not ideal for screen readers.

[gregnb]

• Screen readers might flood users when reading the content of the whole container.

Kind of worried about this part tbh. I wonder how that is going to play out

This is something that was raised to me personally when taking part in a wcag 2.1 audit at work. It’s definitely not ideal for screen readers.

I agree

@eps1lon Thanks for providing more details on the limitations of #23364, I wish you could have shared it sooner, I felt like you had the information but kept it for yourself, simply saying: it will never work. This helps evaluate the tradeoff.

So we have two downsides with the proposed approach:

1. false-negative. We can miss tabbable scroll containers. I could find whatwg/html#2851 and https://www.chromestatus.com/feature/5231964663578624 on this topic. It seems that it's something Firefox has been doing for a long time (at least since 2017). I can't reproduce on Chrome on Windows and macOS (Linux has <2% of the desktop usage). I have asked about it on focus-trap/tabbable#167.
   From what I understand: 1. developers can hedge it by manually setting a tabIndex, 2. end-users can manually overcome it by tabbing in the opposite direction. 3. wai-aria discourage focusing on large scrollable areas. 4. this behavior affects <10% of the end-users on desktop (~8% Firefox + ~2% Linux, so far). 5. the TrapFocus could inspect the DOM to find them (once more user agents do it).
2. false-positive. On Safari macOS buttons aren't tabbable. With the current approach on YouTube's modals, the code focuses the x icon button when it should have skipped it.
   From what I understand, it's OK-ish, we get from time to time questions about why MUI's Button isn't tabbable.

On the other hand, we have two downsides with the current approach:

1. It loses track of where the focus is. It's hard to make the modal container focus indicator visible. The modal usually have a border on their own. This needs to be done with a :focus-visible logic. If you don't, you either get this crappy UX: [DatePicker] Strange black border in the calendar #23532 or the focus visible is not visible at all. The focus-visible logic is hard to get right and has to be repeated every single time you need it. It's easy to give up on it (unless we decide to create a proper hook abstraction to make it easier).
2. It floods screen readers. Modal containers usually contain a lot of content that we don't want screen readers to read.

@oliviertassinari if i boil this down (and they're both broken implementations. ie: current vs PR):

PR implementation:

• (~8% Firefox + ~2% Linux, so far) affected
• MacOS false positive, but this is already occurring

Current implementation:

• Loss of focus every time for all users
• Screen readers are flooded with content every time for all users

I'm still at a loss seeing how the current implementation is better

[oliviertassinari]

@eps1lon What do you think about the above :)? So far I felt that you have been opposing the change without contributing significantly to a solution. It you could engage in this follow-up discussion, it would be great. e.g what alterrnative solution did you consider? You suggested that an alternative was possible :).

It looks like we have a clear direction from the community. This is a step forward. I plan to apply the changes back in the coming days unless we uncover new important information, leave enough time to get more data. We can also keep integrate in the problem after.

[eps1lon]

The problem is that we have two problems and people think they can fix both of them. Neither problem is clearly defined and the solution is proofably wrong. This makes for a disaster which we should absolutely avoid.

In addition, it doesn't help if bugs are reported without reproduction. Sentinels being focused is definitely a bug which we need to solve. So if this happens, please follow the standard procedure.

With regards to focusing the container: This is the tradeoff we're making if authors are not properly wiring up their dialog by declaring which element should be focused when opened. We could just disable that behavior. This seems like a far less intrusive change. Why not go down this road instead?

I'm still at a loss seeing how the current implementation is better

It is not. This is not a one-dimensional problem. Both have different problems but at least for the current problem we have a workaround. Your implementation makes that impossible.

Because I am not seeing any other element that was accessible being no longer accessible in the PR other than the sentential

Because you are probably using a different user agent than I am using. I don't think anyone acknowledged the spec behavior that user agents are implementing: focus is up to the user agent. However, we do not have an API to ask the user agent if an item is focusable.

So far I felt that you have been opposing the change without contributing significantly to a solution.

I showed you why we can't know which items are tabbable with the APIs that are available to us. It's like I just explained to you what a black hole is and you complain that I'm not contributing to how we can reach the singularity within.

I plan to apply the changes back in the coming days unless we

And I will veto them until we actually know what we are doing. This means an extensive set of fixtures that we can test the new approach with different user agent + screen reader combinations. I do not understand why you're forcing this change without acknowledging what I told and showed you.

[eps1lon]

You know what, I've got plenty things to do so I'm leaving this up for grabs and wait for the issues to come in how the new approach prevents focusing focusable elements. I did my job by referencing the spec and a reference implementation of that spec (chrome). If people want to ignore me, so be it.

[oliviertassinari]

the solution is proofably wrong.

@eps1lon I think that we all agree on this. The current approach is provable wrong too. So I think that it's about the evaluation of what's the least worse option.

In addition, it doesn't help if bugs are reported without reproduction. Sentinels being focused is definitely a bug which we need to solve. So if this happens, please follow the standard procedure.

👍 for reporting.

So far, we have assumed that the auditor that didn't realize the focus would move to the container, with no outline. If it was truly an issue with focusing the sentinel, I would have expected the auditor to reject the patch we did in #23364 because it relies on it too.

With regards to focusing the container: This is the tradeoff we're making if authors are not properly wiring up their dialog by declaring which element should be focused when opened. We could just disable that behavior. This seems like a far less intrusive change. Why not go down this road instead?

How would the focus round-robing technically work without a container and without inspecting to DOM to determine the tabbable nodes? Would it mean leaving the focus on an invisible sentinel? If you think there is potential, it would be great to see a POC or, even better, an existing implementation in the wild that show the potential :).

Nodes can still be focused on when aria-hidden is set. So if the container has no tab-index to intercept clicks on blank areas of the modal, we would need to move the focus from the body, back to the modal. Where should it go? If there is a scrollbar, would it reset the scroll, anytime an end-user tries to use the scrollbar?

I don't think anyone acknowledged the spec behavior that user agents are implementing: focus is up to the user agent.

This point is important. I think that we have acknowledged it by trying to demonstrate how it's OK-ish in practice (with the false-negative and false-positive of the logic that tries to emulate the user-agent behavior with heuristics).

Both have different problems but at least for the current problem, we have a workaround. Your implementation makes that impossible.

What's the workaround? Applying the same logic by dropping react-focus-lock inside a Modal?

Why is it impossible? I believe we have previously demonstrated that the two downsides of #23364 are negligible. false-positive (shouldn't have focused) barely harm and false-negative (should have been focused) can be hedged by 1. manually adding tab-index, 2. providing a custom tabbable logic with the new prop, 3. ignoring it (<10% of frequency)