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

EthanStandel · 2022-07-17T06:44:30Z

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

Problem description

Solution

Setting tabIndex={open ? 0 : -1} on sentinelStart and sentinelEnd elements so that they're only tabbable when the TrapFocus open prop is true. Thus removing the awkward experience where there are invisible tabbable elements.

Preview: https://deploy-preview-33543--material-ui.netlify.app/base/react-trap-focus/

mui-bot · 2022-07-17T06:48:48Z

Details of bundle changes

Generated by 🚫 dangerJS against bf6dd7d

mnajdova · 2022-07-27T14:08:37Z

packages/mui-base/src/TrapFocus/TrapFocus.test.js

@@ -283,6 +283,28 @@ describe('<TrapFocus />', () => {
    expect(screen.getByTestId('root')).toHaveFocus();
  });

+  it('does not allow sentinelStart or sentinelEnd to be tabbable until open={true}', () => {


Instead of testing the tabindex value, I would suggest testing the behavior, as the implementation may change in the future.

@mnajdova done! However, to add real tabbability testing, I needed to add @testing-library/user-event which has an awkward naming conflict with test/utils/userEvent.ts and I don't know if you have a preferred solution for that situation.

mnajdova · 2022-07-27T14:10:48Z

cc @michaldudak would be great to have a second opinion on the changes. @samuelsycamore for reviewing the copywriting.

michaldudak · 2022-07-27T14:27:57Z

I haven't thought of such a use case before, but these changes look OK to me.
As a follow-up, I think we'd have to rename the open prop. enabled would make more sense, especially for such cases.

…irrelevant (mui#33380)

EthanStandel · 2022-07-28T00:45:04Z

When I tried to resync the master branch of my fork the PR autoclosed! Reopening!

EthanStandel · 2022-07-28T01:12:22Z

@michaldudak I really like the idea of the prop being enabled, but I also know that change could be wide reaching. You're not recommending that change for this PR, are you?

michaldudak · 2022-07-28T11:13:02Z

No, not at all, this should be a separate PR that we can handle on our own.

samuelsycamore

Solid addition! I just have a couple suggestions for grammar and style in the docs.

docs/data/base/components/trap-focus/trap-focus.md

samuelsycamore · 2022-07-28T21:21:17Z

docs/data/base/components/trap-focus/trap-focus.md

+
+## 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 which is entire separete from the element which would open the modal. However, there are use cases where you may want to have a toggle button for the `open` prop of the `TrapFocus` component which is stored inside that component.


Suggested change

The most common use case for the `TrapFocus` component is to maintain focus within a [`Modal`](/base/react-modal/) component which is entire separete from the element which would open the modal. However, there are use cases where you may want to have a toggle button for the `open` prop of the `TrapFocus` component which is stored inside that component.

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:

Done, except for one thing. I think it should be "Modal" and not "modal" because directly above this section, the Portal component is referenced in this exact same way. 😄

EthanStandel · 2022-08-03T03:03:37Z

Same deal as before. Next time I won't open a PR off a fork from master. I'd change to another branch now but it seems like I'd have to open a new PR.

EthanStandel · 2022-08-05T03:20:46Z

Requesting a re-review from @samuelsycamore & @mnajdova ! 😄

Is that how I should do that? I legitimately don't know.

mnajdova

This is a really great first contribution to MUI @EthanStandel 👌 I look forward for more of these 😄

docs/data/base/components/trap-focus/trap-focus.md

oliviertassinari

@EthanStandel Thanks for working on this. Should we close #33380? I assume it's fixed.

oliviertassinari · 2022-08-17T22:31:13Z

packages/mui-base/src/TrapFocus/TrapFocus.test.js

@@ -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';


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:

material-ui/test/e2e/index.test.ts

Line 118 in 8d6bae2

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

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

Suggested change

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

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.

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 😅

oliviertassinari · 2022-08-17T22:48:46Z

docs/data/base/components/trap-focus/ContainedToggleTrappedFocus.tsx

+import Stack from '@mui/material/Stack';
+import TrapFocus from '@mui/base/TrapFocus';
+
+export default function BasicTrapFocus() {


The name doesn't match

Suggested change

export default function BasicTrapFocus() {

export default function ContainedToggleTrappedFocus() {

oliviertassinari · 2022-08-17T22:49:43Z

docs/data/base/components/trap-focus/ContainedToggleTrappedFocus.tsx

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


Unnecessary, we can simplify the demo without

Suggested change

<Box

sx={{

display: 'flex',

alignItems: 'center',

flexDirection: 'column',

}}

>

I was going for consistency with the other demos

e.g. DisableEnforceFocus, LazyTrapFocus, PortalTrapFocus

Would you suggest that they all be changed?

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

oliviertassinari · 2022-08-17T22:52:01Z

docs/data/base/components/trap-focus/ContainedToggleTrappedFocus.tsx

+        <Stack>
+          <Stack alignItems="center">


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

Suggested change

<Stack>

<Stack alignItems="center">

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

That makes sense!

…mui#33543)

EthanStandel changed the title ~~[base] Removes invisible tabbable elements from TrapFocus component (fixes #33380)~~ [base] Removes invisible tabbable elements from TrapFocus component Jul 17, 2022

EthanStandel mentioned this pull request Jul 17, 2022

[FocusTrap] sentinelStart & sentinelEnd create an awkward experience #33380

Closed

2 tasks

EthanStandel force-pushed the master branch from 782f38f to c557454 Compare July 17, 2022 07:02

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 22, 2022

mnajdova reviewed Jul 27, 2022

View reviewed changes

EthanStandel closed this Jul 28, 2022

EthanStandel force-pushed the master branch from c557454 to 239c207 Compare July 28, 2022 00:35

EthanStandel added 2 commits July 27, 2022 20:36

[base] Make invisible elements untabbable in TrapFocus when they are …

7b67699

…irrelevant (mui#33380)

[docs] adds use case for TrapFocus (mui#33380)

b7c41a0

EthanStandel mentioned this pull request Jul 28, 2022

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

Closed

1 task

EthanStandel reopened this Jul 28, 2022

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 28, 2022

[base] adds test for non-tabbability of initial-state TrapFocus

ecd6031

EthanStandel force-pushed the master branch from 21442a8 to ecd6031 Compare July 28, 2022 03:26

samuelsycamore reviewed Jul 28, 2022

View reviewed changes

[base] updates docs to match latest layout

6976938

EthanStandel force-pushed the master branch from 112594b to 6976938 Compare August 1, 2022 02:13

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 1, 2022

EthanStandel closed this Aug 3, 2022

EthanStandel force-pushed the master branch from 6976938 to 4ced48e Compare August 3, 2022 03:00

Merge branch 'temp'

7fa7bab

EthanStandel reopened this Aug 3, 2022

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 3, 2022

mnajdova approved these changes Aug 5, 2022

View reviewed changes

EthanStandel requested a review from samuelsycamore August 6, 2022 01:44

samuelsycamore reviewed Aug 8, 2022

View reviewed changes

docs/data/base/components/trap-focus/trap-focus.md Outdated Show resolved Hide resolved

[docs] fix non-existant component reference to plain link

bf6dd7d

EthanStandel requested a review from samuelsycamore August 9, 2022 22:12

samuelsycamore approved these changes Aug 9, 2022

View reviewed changes

mnajdova merged commit f703696 into mui:master Aug 10, 2022

oliviertassinari added component: FocusTrap The React component. bug 🐛 Something doesn't work labels Aug 17, 2022

oliviertassinari reviewed Aug 17, 2022

View reviewed changes

EthanStandel mentioned this pull request Aug 21, 2022

[FocusTrap] Improve tab test and simplify demo #34008

Merged

1 task

daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022

[base] Removes invisible tabbable elements from TrapFocus component (…

f96dcc8

…mui#33543)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

EthanStandel commented Jul 17, 2022 •

edited by oliviertassinari

mui-bot commented Jul 17, 2022 •

edited

mnajdova Jul 27, 2022

EthanStandel Jul 28, 2022

mnajdova commented Jul 27, 2022

michaldudak commented Jul 27, 2022

EthanStandel commented Jul 28, 2022 •

edited

EthanStandel commented Jul 28, 2022 •

edited

michaldudak commented Jul 28, 2022

samuelsycamore left a comment

samuelsycamore Jul 28, 2022

EthanStandel Aug 1, 2022 •

edited

EthanStandel commented Aug 3, 2022

EthanStandel commented Aug 5, 2022

mnajdova left a comment

oliviertassinari left a comment •

edited

oliviertassinari Aug 17, 2022

eps1lon Aug 17, 2022 •

edited

EthanStandel Aug 20, 2022

oliviertassinari Aug 17, 2022

oliviertassinari Aug 17, 2022

EthanStandel Aug 20, 2022

EthanStandel Aug 21, 2022

oliviertassinari Aug 17, 2022

EthanStandel Aug 20, 2022


		## 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 which is entire separete from the element which would open the modal. However, there are use cases where you may want to have a toggle button for the `open` prop of the `TrapFocus` component which is stored inside that component.

	The most common use case for the `TrapFocus` component is to maintain focus within a [`Modal`](/base/react-modal/) component which is entire separete from the element which would open the modal. However, there are use cases where you may want to have a toggle button for the `open` prop of the `TrapFocus` component which is stored inside that component.
	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:

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

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

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

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

Conversation

EthanStandel commented Jul 17, 2022 • edited by oliviertassinari

Problem description

Solution

mui-bot commented Jul 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Jul 27, 2022

michaldudak commented Jul 27, 2022

EthanStandel commented Jul 28, 2022 • edited

EthanStandel commented Jul 28, 2022 • edited

michaldudak commented Jul 28, 2022

samuelsycamore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EthanStandel Aug 1, 2022 • edited

Choose a reason for hiding this comment

EthanStandel commented Aug 3, 2022

EthanStandel commented Aug 5, 2022

mnajdova left a comment

Choose a reason for hiding this comment

oliviertassinari left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon Aug 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

EthanStandel commented Jul 17, 2022 •

edited by oliviertassinari

mui-bot commented Jul 17, 2022 •

edited

EthanStandel commented Jul 28, 2022 •

edited

EthanStandel commented Jul 28, 2022 •

edited

EthanStandel Aug 1, 2022 •

edited

oliviertassinari left a comment •

edited

eps1lon Aug 17, 2022 •

edited