[FocusTrap] sentinelStart & sentinelEnd create an awkward experience

[EthanStandel]

Duplicates

• I have searched the existing issues

Latest version

• I have tested the latest version

Current behavior 😯

When <TrapFocus open={false} />, there are two elements named internally as sentinelStart & sentinelEnd. I think I have a relatively good understanding of their purpose, however, they create an incredibly awkward experience for users when open={false} because it creates two invisible, tabbable elements that the user will arbitrarily tab through, creating and awkward experience for keyboard-bound users, which is the exact opposite of the inention of this utility.

Expected behavior 🤔

Only render sentinalStart & sentinelEnd when open={true}, or, maybe more safely, have them be tabIndex={open ? 0 : undefined}

Steps to reproduce 🕹

Steps:

1. Utilize TrapFocus in a context where the TrapFocus is activated or deactivated based on their open prop and not based on it's own existance.
2. Tab through the document and note the awkward experience.

Context 🔦

In my context, I want to have a TrapFocus around my menu icon-button and default open to false. When I click the icon-button, I set open to true and render the menu also inside the TrapFocus element. At that point everything works as it should. However, up until the trap activates, I just have these arbitrary focusable nodes wrapping one of the first elements in my document that leads to an awkward user experience.

I could render a different copy of the menu icon-button with the menu alongside it when my menu is supposed to be open, but I'm using hamburgers as my menu icon and rendering a new copy would break the transition in progress.

Your environment 🌎

npx @mui/envinfo

System:
    OS: macOS 12.3.1
  Binaries:
    Node: 14.18.3 - ~/.nvm/versions/node/v14.18.3/bin/node
    Yarn: 1.22.17 - ~/.nvm/versions/node/v14.18.3/bin/yarn
    npm: 6.14.15 - ~/.nvm/versions/node/v14.18.3/bin/npm
  Browsers:
    Chrome: 103.0.5060.53
    Edge: 103.0.1264.44
    Firefox: Not Found
    Safari: 15.4
  npmPackages:
    @emotion/react: ^11.9.3 => 11.9.3
    @emotion/styled: ^11.9.3 => 11.9.3
    @mui/base: ^5.0.0-alpha.87 => 5.0.0-alpha.87
    @mui/icons-material: ^5.8.4 => 5.8.4
    @mui/lab: ^5.0.0-alpha.88 => 5.0.0-alpha.88
    @mui/material: ^5.8.6 => 5.8.6
    @mui/private-theming:  5.8.6
    @mui/styled-engine:  5.8.0
    @mui/system:  5.8.6
    @mui/types:  7.1.4
    @mui/utils:  5.8.6
    @mui/x-date-pickers:  5.0.0-alpha.1
    @types/react: ^18.0.14 => 18.0.14
    react: ^18.2.0 => 18.2.0
    react-dom: ^18.2.0 => 18.2.0
    styled-components:  5.3.5
    typescript: ^4.7.4 => 4.7.4

[mnajdova]

Thanks for the issue. It is very well described and even provide potential solutions, it is something we rarely see :)

I would vote for updating the sentinelStart and sentinelEnd's tabindex {open ? 0 : undefined} or maybe open ? 0 : -1. Before we jump to fixing it, can you please share a codesandbox with a simple code that will illustrate the issue?

[EthanStandel]

Sure @mnajdova !

Here's a reproduction: https://stackblitz.com/edit/react-ts-zvg3p8?file=App.tsx

And because I'm having trouble actually getting that link to run in an incognito tab, here's the content

Initialize a new React + TS repo in StackBlitz, add @mui/base as a dependency and then replace App.tsx with this

/** @jsxFrag React.Fragment */
import { TrapFocus } from '@mui/base';
import * as React from 'react';
import './style.css';

export default function App() {
  const [menuOpen, setMenuOpen] = React.useState(false);

  return (
    <>
      <button>Some button before the menu</button>
      <TrapFocus open={menuOpen}>
        <div>
          <button onClick={() => setMenuOpen((open) => !open)}>
            I open the menu!
          </button>
          {menuOpen && (
            <>
              <button>Enclosed menu item</button>
            </>
          )}
        </div>
      </TrapFocus>
      <button>Some button after the menu</button>
    </>
  );
};

[mnajdova]

Thanks, I also tried moving the trigger button outside of the TrapFocus, but the behavior is the same. Alright, do you want to create a PR with the proposed changes?

[EthanStandel]

@mnajdova Done! I don't know if the documentation is excessive. If so, I can back up a commit to remove those changes and just leave the described fix + test.

#33543

[EthanStandel]

Completed with PR #33543

[oliviertassinari]

@EthanStandel If you could work on a follow-up for #33543 (review) it would be awesome 🙏

[EthanStandel]

@oliviertassinari I could try to carve out some time to make that happen next week now. In the mean time, is it worth making another issue to represent this ask or possibly just reopening this issue?

Follow-up PR: #34008