[TrapFocus] Fix trap to only focus on tabbable elements

[gregnb]

• I have followed (at least) the PR section of the contributing guide.

In this PR I'm attempting to build upon the work done #21857 -- I scanned the PR/ticket and looked into how tabbable works and pulled pieces from that as well. Before I put anymore effort just want to see where we stand with this attempt? For my own purposes as I mentioned in another thread my company has a flagship product being audited by a FAAMNG. They are holding us to the extra tabs trigged from the sententials. Because TrapFocus wraps some core components we are getting marked up quite a bit on this issue. Hopefully, this PR can go somewhere and would be nice to have it land here if it fits.

What remains:

• 4 broken unit tests for <Menu variant="menu">: The selected menu as it stands I think if this approach is accepted those unit tests might need be adjusted? I'm not sure. The tests are failing because it expects focus to be put on elements with a tabIndex of -1?
  - 1 broken unit test for <TrapFocus /> does not bounce focus around due to sync focus-restore + focus-contain:
  I am stumped on this one at the current moment. Could use some help

Closes #19651.

[mui-pr-bot]

@material-ui/core: parsed: +0.36% , gzip: +0.45%
@material-ui/lab: parsed: +0.40% , gzip: +0.53%

Details of bundle changes

Generated by 🚫 dangerJS against 1b6990f

[oliviertassinari]

I haven't done a full review, I have stopped at 10%, it seems that the PR should be in a draft-mode, do you confirm?

[gregnb]

I haven't done a full review, I have stopped at 10%, it seems that the PR should be in a draft-mode, do you confirm?

Agree. I just wanted to raise something to know if we're ok with the approach I've taken + need some help where I got stuck on those remaining tests

[gregnb]

@oliviertassinari so I have solved the last broken test in Unstable_FocusTrap.js -- what remains is an answer if it is ok to adjust the current unit tests in packages/material-ui/test/integration/Menu.test.js

There are 4 remaining tests that fail because they're written to first focus on the root but with this change as we know we would focus on the first tabbable element instead.

[oliviertassinari]

@gregnb I have pushed the pull-request further. I see one test that is left failing, and that look expected: https://github.com/mui-org/material-ui/blob/ad703afd116560f6591e215def61dc8fb83fb449/packages/material-ui/test/integration/Menu.test.js#L277-L280

I think that we can leave the focus untouched, so on the body. What do you think?

I have also noticed some issues with the dialog demos. How about we focus the container by default?

[gregnb]

@gregnb I have pushed the pull-request further. I see one test that is left failing, and that look expected:

https://github.com/mui-org/material-ui/blob/ad703afd116560f6591e215def61dc8fb83fb449/packages/material-ui/test/integration/Menu.test.js#L277-L280

I think that we can leave the focus untouched, so on the body. What do you think?

Agreed.

I have also noticed some issues with the dialog demos. How about we focus the container by default?

Anything you can point out in particular? I'm not sure about focusing on the container tbh. At least make it optional to not focus on the container? my expectation in using a component like this would be to focus on the first tabbable (tabIndex >=0) element

[eps1lon]

#19651 (comment) still applies especially

I say this because the approach in #22062 is fundamentally flawed since it assumes that determining what element is tabbable is something we can do by reading the DOM. However, what elements are tabbable (in sequential focus order) can determined by the user agent (html.spec.whatwg.org/multipage/interaction.html#sequentially-focusable). We can ask if an element is in sequential focus order by checking tag names or tabIndex. But returning "No" from that question does not imply that the element is not tabbable.

The way focus is specified, works across user agents and relates to accessibility (especially the customization part), I'm pretty sure this approach does not work.

So this absolutely needs a comprehensive test harness that covers all popular screen reader combinations and more research. The PR and issue falls a bit short on explaining this approach, what problem it solves, what alternatives you have considered and where it falls short.

[oliviertassinari]

Anything you can point out in particular?

@gregnb The body scrollable demo was focusing a button at the bottom, breaking the scroll position. I have fixed it by disabling the autofocus. Instead, we only solve the issue reported: remove the "root" in the focus loop.

I'm pretty sure this approach does not work.

@eps1lon Do you have a reproduction?

what alternatives you have considered

I think that searching in detail all the pros & cons of each option can be very time-consuming. I think that we could benchmark what the others are doing, pick the one that seems best, and push it to production, get hard feedback from users. This is essential deferring the problem to the developers, have them run the in-depth exploration.

• WAI-ARIA uses a tabbable array combined with a focus attempts approach. They find all the tabbable nodes and try to focus them.
• focus-trap uses the tabbable npm dependency. They pick the first and last index from the array. This almost the approach used here, but at the difference as they don't have any sentinels. It doesn't seem to cause any significant issues
• Fluent UI is the closest approach I could find to what it's done here. It seems to be essentially the same. A sentinel to catch leaking tab events and an array logic to identify the first and last tabbable elements.

---

On a related note, there are some refactoring and a bug fix done here that could be isolated into a different pull request.

[oliviertassinari]

This is a bug fix for another issue. I could have moved it to a different PR.

[gregnb]

Perhaps it could be something like 'hey, here's a sandbox, can you test this?' type of thing

@gregnb Are you aware of this one #23583 (comment)? You could ship a release with the npm package published by this pull request.

Thanks for the info. I will share that when I'm back. sounds great

[eps1lon]

I disagree, I think that it's a significant part of how we learn and improve,

But how do you decide whom you listen to? It's obviously not working since I gave you ample evidence that this PR is just incorrect yet you don't listen to me. It's wrong by spec, it's wrong by example. So why do you not listen to the evidence I gave you?

The reason you are giving here is just dishonest. It has nothing to do with collaborating or learning from others. You're actively and knowingly harming users so that you can merge this PR. Why, I have no idea. Maybe you're just not sharing private information you got from gregnb.

[eps1lon]

Revert until #23827 is resolved.

[gregnb]

@eps1lon you haven't answered my question above. how about we start with that?

i'm not really seeing how the current implementation is any better. when a user tabs through and lands on a sentential that's confusing to the user when they have to hit yet ANOTHER tab to get off of that sentential.

[oliviertassinari]

I gave you ample evidence that this PR is just incorrect yet you don't listen to me. It's wrong by spec, it's wrong by example. So why do you not listen to the evidence I gave you?

@eps1lon I believe this concern was already addressed. The current approach is an approximation, it's not perfect for sure. However, we have evidence that it's better than the current logic. It also been good enough for some of the most popular websites, e.g. YouTube, Fluent UI, etc.

I don't understand. Why should we reject option a (new version) over option b (current version) when we have evidence that option a is better than option b, even if both are incorrect?
I think that getting 80% of the job done over 40% is still an improvement, even if we don't reach 100%.

The reason you are giving here is just dishonest

Which reason, the ones above? What do you mean by dishonest?

Maybe you're just not sharing private information you got from gregnb.

@gregnb has sent this pull request for review by an a11y expert working for a FAAMNG. They gave us feedback that it was an improvement. The change is important enough to have @gregnb & FAAMNG dedicate time to the matter. Isn't what the community is here for, so we can iterate? We can very well try the changes, release them, and wait for feedback. If it's wrong, somebody will raise it. If not, then it's likely good enough.