Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[base] Removes invisible tabbable elements from TrapFocus component #33543

Merged
merged 6 commits into from Aug 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,33 @@
import * as React from 'react';
import Box from '@mui/material/Box';
import Stack from '@mui/material/Stack';
import TrapFocus from '@mui/base/TrapFocus';

export default function BasicTrapFocus() {
const [open, setOpen] = React.useState(false);

return (
<Box
sx={{
display: 'flex',
alignItems: 'center',
flexDirection: 'column',
}}
>
<TrapFocus open={open} disableRestoreFocus disableAutoFocus>
<Stack>
<Stack alignItems="center">
<button type="button" onClick={() => setOpen(!open)}>
{open ? 'Close' : 'Open'}
</button>
</Stack>
{open && (
<label>
First name: <input type="text" />
</label>
)}
</Stack>
</TrapFocus>
</Box>
);
}
@@ -0,0 +1,33 @@
import * as React from 'react';
import Box from '@mui/material/Box';
import Stack from '@mui/material/Stack';
import TrapFocus from '@mui/base/TrapFocus';

export default function BasicTrapFocus() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name doesn't match

Suggested change
export default function BasicTrapFocus() {
export default function ContainedToggleTrappedFocus() {

const [open, setOpen] = React.useState(false);

return (
<Box
sx={{
display: 'flex',
alignItems: 'center',
flexDirection: 'column',
}}
>
Comment on lines +10 to +16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary, we can simplify the demo without

Suggested change
<Box
sx={{
display: 'flex',
alignItems: 'center',
flexDirection: 'column',
}}
>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going for consistency with the other demos

e.g. DisableEnforceFocus, LazyTrapFocus, PortalTrapFocus

Would you suggest that they all be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nevermind I went to do that and then immediately saw why it's necessary in those cases and not this one!

<TrapFocus open={open} disableRestoreFocus disableAutoFocus>
<Stack>
<Stack alignItems="center">
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would have been simpler like this, we don't need two Stack.

Suggested change
<Stack>
<Stack alignItems="center">
<Stack alignItems="center" spacing={2}>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense!

<button type="button" onClick={() => setOpen(!open)}>
{open ? 'Close' : 'Open'}
</button>
</Stack>
{open && (
<label>
First name: <input type="text" />
</label>
)}
</Stack>
</TrapFocus>
</Box>
);
}
@@ -0,0 +1,14 @@
<TrapFocus open={open} disableRestoreFocus disableAutoFocus>
<Stack>
<Stack alignItems="center">
<button type="button" onClick={() => setOpen(!open)}>
{open ? 'Close' : 'Open'}
</button>
</Stack>
{open && (
<label>
First name: <input type="text" />
</label>
)}
</Stack>
</TrapFocus>
7 changes: 7 additions & 0 deletions docs/data/base/components/trap-focus/trap-focus.md
Expand Up @@ -84,3 +84,10 @@ When auto focus is disabled—as in the demo below—the component only traps th
The following demo uses the [`Portal`](/base/react-portal/) component to render a subset of the `TrapFocus` children into a new "subtree" outside of the current DOM hierarchy, so they are no longer part of the focus loop:

{{"demo": "PortalTrapFocus.js"}}

### Using a toggle inside the trap

The most common use case for the `TrapFocus` component is to maintain focus within a [modal](/base/react-modal/) component that is entirely separate from the element that opens the modal.
But you can also create a toggle button for the `open` prop of the `TrapFocus` component that is stored inside of the component itself, as shown in the following demo:

{{"demo": "ContainedToggleTrappedFocus.js"}}
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -88,6 +88,7 @@
"@rollup/plugin-replace": "^4.0.0",
"@testing-library/dom": "^8.16.0",
"@testing-library/react": "^13.3.0",
"@testing-library/user-event": "^14.3.0",
"@types/chai": "^4.3.1",
"@types/chai-dom": "^0.0.13",
"@types/enzyme": "^3.10.12",
Expand Down
11 changes: 8 additions & 3 deletions packages/mui-base/src/TrapFocus/TrapFocus.js
Expand Up @@ -325,13 +325,18 @@ function TrapFocus(props) {
return (
<React.Fragment>
<div
tabIndex={0}
tabIndex={open ? 0 : -1}
onFocus={handleFocusSentinel}
ref={sentinelStart}
data-test="sentinelStart"
data-testid="sentinelStart"
/>
{React.cloneElement(children, { ref: handleRef, onFocus })}
<div tabIndex={0} onFocus={handleFocusSentinel} ref={sentinelEnd} data-test="sentinelEnd" />
<div
tabIndex={open ? 0 : -1}
onFocus={handleFocusSentinel}
ref={sentinelEnd}
data-testid="sentinelEnd"
/>
</React.Fragment>
);
}
Expand Down
31 changes: 31 additions & 0 deletions packages/mui-base/src/TrapFocus/TrapFocus.test.js
Expand Up @@ -4,6 +4,7 @@ import { expect } from 'chai';
import { act, createRenderer, screen } from 'test/utils';
import TrapFocus from '@mui/base/TrapFocus';
import Portal from '@mui/base/Portal';
import userEvent from '@testing-library/user-event';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK @eps1lon has been pushing toward not using this plugin, I would be in favor of doing such as well. The logic that implements userEvent.tab(); looks brittle, I don't think that we should trust it for tests. Instead, we could continue to use the real browser tab as the existing tests are doing in:

describe('<TrapFocus />', () => {

How about we do a follow-up to remove it?

Suggested change
import userEvent from '@testing-library/user-event';

Copy link
Member

@eps1lon eps1lon Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we already have Playwright tests for TrapFocus or any other more involved user-interaction tests? I would still recomend using that approach over using user-event. Especiallly for a component library. Unless you have good reason for using JSDOM + replaying events in which case I'd be curious to hear why you'd prefer JSDOm + replaying events over browser tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be fine with moving the meaning of this test to Playwright and removing the new dependency. If I'm to be honest, I didn't notice the e2e tests initially 😅


describe('<TrapFocus />', () => {
const { clock, render } = createRenderer();
Expand Down Expand Up @@ -283,6 +284,36 @@ describe('<TrapFocus />', () => {
expect(screen.getByTestId('root')).toHaveFocus();
});

it('does not create any tabbable elements when open={false}', () => {
function Test(props) {
return (
<div>
<button autoFocus data-testid="initial-focus">
Test
</button>
<TrapFocus open={false} {...props}>
<div tabIndex={-1}>
<button data-testid="inside-focus">Test</button>
</div>
</TrapFocus>
<button data-testid="end-focus">Test</button>
</div>
);
}

render(<Test />);

expect(screen.getByTestId('initial-focus')).toHaveFocus();
act(() => {
userEvent.tab();
});
expect(screen.getByTestId('inside-focus')).toHaveFocus();
act(() => {
userEvent.tab();
});
expect(screen.getByTestId('end-focus')).toHaveFocus();
});

describe('interval', () => {
clock.withFakeTimers();

Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
Expand Up @@ -3163,6 +3163,11 @@
"@testing-library/dom" "^8.5.0"
"@types/react-dom" "^18.0.0"

"@testing-library/user-event@^14.3.0":
version "14.3.0"
resolved "https://registry.yarnpkg.com/@testing-library/user-event/-/user-event-14.3.0.tgz#0a6750b94b40e4739706d41e8efc2ccf64d2aad9"
integrity sha512-P02xtBBa8yMaLhK8CzJCIns8rqwnF6FxhR9zs810flHOBXUYCFjLd8Io1rQrAkQRWEmW2PGdZIEdMxf/KLsqFA==

"@theme-ui/color-modes@0.14.7":
version "0.14.7"
resolved "https://registry.yarnpkg.com/@theme-ui/color-modes/-/color-modes-0.14.7.tgz#6f93bf4d0890ffe3386df311663eaee9bea40796"
Expand Down