[FocusTrap] Correctly focus on the correct element on initial focus

[Greg-NetDuma]

with a fallback to the root component.

The Problem:
Dialog has a DOM layout something like this:

<modal>
   <backdrop>
  <focusTrap start />
   <dialog tabindex="-1" role="presentation">
        <dialogPaper role="dialog">
        </dialogPaper>
   </dialog>
  <focusTrap end />
</modal>

Modal applies tabindex to dialog for accessibility reasons (see link for possible explanation) and FocusTrap automatically focuses on it's direct child (Dialog). But Dialog has an incorrect role set in Dialog.js for this specific case.

According to the guidelines the modal should focus on either:

• The first tabbable element
• The element that makes most sense
• On the first element specified by tabindex={-1}

The Solution
Make sure FocusTrap immediately focuses on the first tabbable element including elements with tabIndex={-1} set. Regular tabbing should ignore them. I also removed Modal injecting tabindex to the root child forcefully (the only place we seem to use FocusTrap)

This means

• Modal doesn't need to forcefully apply tabindex="-1" to it's child
• FocusTrap will still apply tabindex="-1" to its direct child if no better candidate was found (and complain, currently)
• By adding tabIndex={-1} to the element that actually has "role="dialog" in Dialog will be announced correctly by screenreaders.
• By adding a new prop to Dialog where users can disable adding tabIndex={-1} to the content they can properly control the focus order including the first focus, where they can just choose to set tabIndex={-1} on another element inside the dialog or just let FocusTrap use the implicit/explicit tabindex order.

Alternate Solution
We could just reverse the how roles and aria-props applied from DialogPaper to Dialog instead of all of this. We still can't implement something that controls the initial focus, but it is much simpler.

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

[mui-bot]

Netlify deploy preview

https://deploy-preview-37672--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 2c862b0

[Greg-NetDuma]

Test Cases

are not written. Tests are numbered and bullet points note "expected input" -> "expected output"

FocusTrap

1. On open check the current focus
   • 2 tabbable elements inside -> First one is the document.activeElement
   • 2 tabbable elements plus one more set to tabindex="-1" inside -> the one containing tabindex="-1" is is the document.activeElement
   • no tabbale elements inside -> the root element is the document.activeElement and tabindex="-1" is automatically set on it. FocusTrap will post console.error

Dialog

1. On Dialog open -> DialogPaper should be the document.activeElement with tabindex="-1" on it.
2. On Dialog open and disableInitialContentFocus={true} -> FocusTrap test 1. test cases

[Greg-NetDuma]

Please help me find a better name for disableInitialContentFocus. Its for disabling focusing on DialogPaper element on opening, this causes something else that is tabbable/set to tabindex=1 to be focused inside the dialog.

[Greg-NetDuma]

How this affects other Components

In general tests are failing because now that Modal doesn't set tabIndex on it's child there are modals without any focusable elements inside them (in tests).

The question is should I fix the tests so if you use the modal you are expected to do that manually or should I allow FocusTrap to gracefully handle it instead? Probably the former.

Menu and MenuList

https://github.com/Greg-NetDuma/material-ui/blob/0f44030a4020ea0b5c93eefc8c90d799b7ce4f22/packages/mui-material/src/Menu/Menu.test.js#L203

Menu looks like this:

<Menu (modal)>
  <FocusTrap start />
  <MenuList />
  <FocusTrap end />
</Menu (modal)>

Tests are failing because if you set autoFocus={false} it somehow expects the focus to be on the Modal outside FocusTrap.

[Greg-NetDuma]

For whoever review this, I appeased the CI gods as much as I can, but I can't seem to generate the docs for some reason.

Also it's not possible for me spend more time on this, if someone can take over to write the new tests based on the test cases I've written and generate the docs I would appreciate that.

[mj12albert]

@Greg-NetDuma Thanks for working on this 💜 I've pushed a commit to generate the docs

Would you mind giving me a TLDR of the state of the tests before I dig into this? 🙏

[Greg-NetDuma]

@mj12albert All existing tests are fixed and passing. (Most of the changes are to make sure all test Modals used have tabbable elements inside now that Modal doesn't mess with tabIndexes - FocusTrap will still inject tabIndex={-1} if it can't find anything with an error)

None of the new tests in #37672 (comment) are written, these are all the cases that I can think of for the changes in this PR.

They need to be implemented. I tried to write the description for them as simple as possible.

[ZeeshanTamboli]

Any updates on this PR?

[Greg-NetDuma]

@ZeeshanTamboli

Also it's not possible for me spend more time on this, if someone can take over to write the new tests based on the test cases I've written and generate the docs I would appreciate that.

This is the current state of this PR. I'm using this fix in production for the past 6 months and it also passed an accessibility audit as well.

[ZeeshanTamboli]

@mj12albert Since you assigned this to yourself, are you planning to work on it by adding the new tests and reviewing the implementation?

[Gr3q]

I added all the tests needed and implemented the fix across all your duplicated codebase.

I still can't generate the docs properly because pnpm docs:api still just wipes the docs for me. ll fix as many other CI steps as I can. I still don't know why your argos check is failing.

Will I need to wait another year? It looks like the interval for the accessibility PRs.

[ZeeshanTamboli]

@michaldudak @DiegoAndai Can you take a look?

[Gr3q]

I'll add 2 more tests to FocusTrap today involving expected behaviour when tabbing inside.

1. if tabindex=-1 was set on a implicitly tabbable (interactive) element initial focus starts there and it can be focused when tabbing through
2. if tabindex=-1 was set on a non-interactive element initial focus starts there and it cannot be focused when tabbing through

As far as I remember that's the expected behaviour but I'll re-read the guidelines and check how it behaves now.

Edit: it seems there are some more test failures because of the changes (or maybe not?) I'll have a look at them too.

[michaldudak]

I'm seeing some typecheck failures as well. I'll try to figure out what's going on.

[Gr3q]

While I was trying to write the test I realized that I'm trying to test is just native browser behaviour. So I only implemented/amended tests for things FocusTrap actually does, like where to it goes next where it reaches it's sentinels.

Initially I thought my implementation logic was wrong but I can see now I implemented it just like https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex#sect2 says.

I'll look at the failure in Select next.

[Gr3q]

I looked at it (/mui-base/**/Select.test.tsx#1257), I'm not sure how to fix it. You might need to revert changes to the lockfile because most of the problems seem to come from that, but probably you know better than me.

[ZeeshanTamboli]

Should this pointed against master branch?

[michaldudak]

I believe so. cc @DiegoAndai

[Gr3q]

Is master going to be v6 or is it still going into v5?

If I have to do a backport after it's merged into master I rather open a new PR for it so I don't have to do the work again (for v5)

[ZeeshanTamboli]

Is master going to be v6 or is it still going into v5?

master is already on v6, the latest stable version of Material UI. If we want this in v5, we could cherrypick it to the v5.x branch, though I’m unsure if or when future v5 releases will happen.

[Gr3q]

@ZeeshanTamboli based on this should I remove all changes inside the mui-base directory?