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
Changes from 5 commits
7b67699
b7c41a0
ecd6031
6976938
7fa7bab
bf6dd7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -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', | ||||||||||||||||
}} | ||||||||||||||||
> | ||||||||||||||||
Comment on lines
+10
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary, we can simplify the demo without
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||||||||||||||||
); | ||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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> |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 material-ui/test/e2e/index.test.ts Line 118 in 8d6bae2
How about we do a follow-up to remove it?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||||||
|
@@ -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(); | ||||||
|
||||||
|
There was a problem hiding this comment.
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