WB-1638: Fix tabbing in popover #2159
Conversation
There is a bug in the popover where tabbing out of the popover (while it is still open) will cause the focus order to be incorrect. More specifically, the focus will be returned to the trigger element, while it should be returned to the next element in the tab order. This is because the popover is not properly handling the case where the trigger element is the last element in the tab order. This PR fixes that by making all the focusable elements in the popover non-tabbable, and then make them tabbable again when the popover gains focus.
🦋 Changeset detectedLatest commit: 4b70ebd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2159 +/- ##
==========================================
+ Coverage 94.91% 95.36% +0.44%
==========================================
Files 245 245
Lines 28292 28442 +150
Branches 2335 2358 +23
==========================================
+ Hits 26854 27124 +270
+ Misses 1438 1318 -120
... and 32 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
Size Change: +556 B (+1%) Total Size: 92.5 kB
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (421b170) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2159"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2159 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-bnpynoxjar.chromatic.com/ Chromatic results:
|
| render( | ||
| <FocusManager anchorElement={anchorElementNode}> | ||
| <div> | ||
| <button>first focusable element inside</button> | ||
| </div> | ||
| </FocusManager>, | ||
| ); |
There was a problem hiding this comment.
This is cool. I didn't realize we had tests that used multiple calls to render() in this way.
| // Act | ||
| // 1. focus on the previous element before the popover | ||
| userEvent.tab(); | ||
|
|
||
| // 2. focus on the anchor element | ||
| userEvent.tab(); | ||
|
|
||
| // 3. focus on focusable element inside the popover | ||
| userEvent.tab(); | ||
|
|
||
| // 4. focus on the next focusable element outside the popover (this will | ||
| // be the first focusable element outside the popover) | ||
| userEvent.tab(); | ||
|
|
||
| // The elements inside the focus manager should not be focusable anymore. | ||
| const focusableElementInside = screen.getByRole("button", { | ||
| name: "first focusable element inside", | ||
| }); | ||
|
|
||
| // Assert | ||
| expect(focusableElementInside).toHaveAttribute("tabIndex", "-1"); | ||
| }); |
There was a problem hiding this comment.
praise: thanks for labelling what each userEvent method call is for.
There was a problem hiding this comment.
Does tabbing back into the popover making elements inside the popover focusable again?
There was a problem hiding this comment.
That's correct.... I make them focusable as soon as the focus goes back to the popover wrapper. One important thing to mention is that I just noticed an issue when the trigger element is the last focusable element in the DOM. I might need to completely change this approach if needed, so wanted to give a heads up.
| <PopoverContent | ||
| title="Popover title" | ||
| content="content" | ||
| actions={<Button>Button inside popover</Button>} | ||
| /> |
There was a problem hiding this comment.
question: are there multiple focusable items in this popover content? If not, could you add another button?
| // Focus on the first element outside the popover | ||
| userEvent.tab(); | ||
| // open the popover by focusing on the trigger element | ||
| userEvent.tab(); | ||
| userEvent.keyboard("{enter}"); | ||
|
|
||
| // Wait for the popover to be open. | ||
| await screen.findByRole("dialog"); | ||
|
|
||
| // Focus on the next element after the popover | ||
| userEvent.tab(); | ||
|
|
||
| // Focus on the document body | ||
| userEvent.tab(); |
There was a problem hiding this comment.
suggestion (non-blocking): it might good to assert what some of these elements are in addition to the comments.
| @@ -86,6 +87,11 @@ export default class FocusManager extends React.Component<Props> { | |||
| */ | |||
| focusableElementsInPopover: Array<HTMLElement> = []; | |||
There was a problem hiding this comment.
The name of this field is a little confusing because we're setting these elements to be non-focusable when the focus exits the popover or whatever.
suggestion: consider renaming this to something like elementsThatCanBeFocusable or something like that to avoid confusion as to whether the elements in the array are currently focusable or not.
Summary:
There is a bug in the popover where tabbing out of the popover (while it
is still open) will cause the focus order to be incorrect. More
specifically, the focus will be returned to the trigger element, while
it should be returned to the next element in the tab order.
This is because the popover is not properly handling the case where the
trigger element is the last element in the tab order. This PR fixes that
by making all the focusable elements in the popover non-tabbable, and
then make them tabbable again when the popover gains focus.
For more reference on the actual error, see: https://khanacademy.atlassian.net/browse/CL-1259
TabTrapWhenPopupDisplayed.mp4
Issue: WB-1638
Test plan:
order is still correct
focus order is still correct (using shift+tab).
Screen.Recording.2024-01-04.at.2.30.37.PM.mov