[FocusTrap] 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.

---

This also fixes #42989

---

• [X] 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

Bundle	Parsed Size	Gzip Size
@mui/material	🔺+158B(+0.03%)	🔺+64B(+0.04%)
@mui/lab	0B(0.00%)	0B(0.00%)
@mui/system	0B(0.00%)	0B(0.00%)
@mui/utils	0B(0.00%)	0B(0.00%)

Details of bundle changes

Generated by :no_entry_sign: dangerJS against 94070e59c0c2ef5c20db5a949b33f9c0fdcdf4fe

[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 https://github.com/mui/material-ui/pull/37672#issuecomment-1602399822 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?

[ZeeshanTamboli]

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

Yes.

[Gr3q]

I did except the test cases, they fail without those changes.

PS. still can't generate the documentation

[ZeeshanTamboli]

PS. still can't generate the documentation

Generated.

[Greg-NetDuma]

One test is failing and that error should be expected, but I don't know the best way to fix it.

describeConformance either should allow passing props (so I can pass tabIndex={-1}) or should be able to expect errors.

[DiegoAndai]

Hey @Greg-NetDuma @ZeeshanTamboli, thanks for working on this, I'm so sorry for the delay.

Could we instead add an initialFocus prop that works like floating UI's?: https://floating-ui.com/docs/FloatingFocusManager#initialfocus

By default, it would be 0 (first tabbable element, the direct child that has tabIndex={-1}), but it can also receive an index (i) to focus the i-th tabbable element, or a ref.

Would this remove the need for reverting the way modal and focus trap force tabIndex?

[Greg-NetDuma]

Could we instead add an initialFocus prop that works like floating UI's?: https://floating-ui.com/docs/FloatingFocusManager#initialfocus By default, it would be 0 (first tabbable element, the direct child that has tabIndex={-1}), but it can also receive an index (i) to focus the i-th tabbable element, or a ref.

Would this remove the need for reverting the way modal and focus trap force tabIndex?

An equivalent of initialFocus is nice to have but if I have to choose a prop in floating this PR is similar to, it would be https://floating-ui.com/docs/floatingfocusmanager#order instead.

If I would take the current state of MUI and translate it to floating according to it's docs, this is would be MUI's default config with no way to change it:

<FloatingFocusManager
  context={context}
  // Initially focuses the floating element. Subsequent tabs
  // will cycle through the tabbable contents of the floating
  // element.
  order={['floating', 'content']}
>
  {/* floating element */}
</FloatingFocusManager>

This PR would change with disableInitialContentFocus prop so you can also have order={['content']} (EDIT: in addition of fixing that Modal just blanket adds tabindex={-1} causing issues). The first tabbabe element is still simply calculated by looking at the tabindexes in the content in line with regular html rules.

Edit: I don't particularly mind how the new prop is named or what it can accept.

[Greg-NetDuma]

You guys had a bug in React Strict Mode where the first user-initiated focus change was not handled after opening the FocusTrap. I fixed it, my new tests would not work otherwise.

[DiegoAndai]

@Greg-NetDuma I don't follow the reasoning here. The initialFocus prop seems to me like it targets the issue this PR is intended to fix directly. The order prop seems like a similar but different use case. I agree it's useful, but I want to keep the PRs scope focused.

[Greg-NetDuma]

@DiegoAndai disableInitialContentFocus is not needed to fix the original issue of this PR at all: tabIndex={-1} is applied to the wrong elements in components that are using Modals inside this library, resulting in FocusTrap not focusing on the correct elements initially. this includes:

• Modal blanket applying tabIndex={-1} to the incorrect children
• plus other components using Modal not adding it to the correct place, e.g. Dialog not adding it to the element that has role="dialog".

If tabIndex={-1} is added to the correct elements inside this library than the issue is solved, meaning the current "floating" element will be focused on opening. I only adjusted the FocusTrap logic to be in line with how Accessibility guidelines recommend tabIndxex={-1} to be handled (focus on it first, then ignore during cycling).

I added disableInitialContentFocus (if I had to name it again it would be disableInitialFloatingFocus for correctness) so devs can disable this behavior - which is still the same behaviour as the original except it's not buggy - exclusively for convenience. I can remove it completely from this PR and it still will be ok.

initialFocus in floating library is just used to select which element should be focused first based on the order prop, so it's different. Without using initialFocus floating just resolves the first focusable element based on the tabindex attributes you have inside your floating element (aka "content"). If you add "floating" to the order prop as the first element it will focus the floating element first, then cycles through the "content" based on tabindex order inside.