Fix Popover focus management when used with Tabs #2863
Conversation
…focusable for popovers
…s management was interfering with Tabs focus management
🦋 Changeset detectedLatest commit: a28c480 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
Size Change: +11 B (+0.01%) Total Size: 109 kB
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (f1976fd) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR2863"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2863 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-cxbhcqrqoj.chromatic.com/ Chromatic results:
|
…e determined there's no focusable element. Previously, we were initializing hasFocusableElement to false, which would set tabindex=0. Because of this, Popover with detect the tabpanel as focusable when the FocusManager finds focusable elements inside the popover
…set with tabIndex=0 before it has determined if it has focusable elements within it (related to a focus management bug when Tabs are used inside a Popover)
…to second tab, tabbing to content, shift tabbing (was going to tabpanel because FocusManager was getting focusable elements when the tab panel hasn't rendered it's children yet so it had tabIndex set to 0. FocusManager should update when child elements change
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2863 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…ponent in a popover
| */ | ||
| const FOCUSABLE_ELEMENTS = | ||
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'; | ||
| 'button:not([tabindex="-1"]), [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'; |
There was a problem hiding this comment.
This makes it so that the tabs (which are button elements with role=tab) with tabindex="-1" aren't included in FocusManager's logic for determining focusable nodes
This prevents the issue where all the tabs are focusable by default
| tabPanel: { | ||
| // Apply flex so that panel supports rtl | ||
| display: "flex", | ||
| ...focusStyles.focus, |
There was a problem hiding this comment.
Adding the WB focus styles to tab panel so that it is compatible with dark backgrounds later on. The tab panel becomes focusable if there are no interactive elements within it
| // If the tab panel doesn't have focusable elements after being | ||
| // determined, it should be focusable so that it is included in the | ||
| // tab sequence of the page | ||
| tabIndex={hasFocusableElement === false ? 0 : undefined} |
There was a problem hiding this comment.
hasFocusableElement is initialized to null now, so we only set tabIndex=0 only once it has been determined that are no focusable elements in the tab panel
Previously, hasFocusableElement was defaulting to false before the useEffect has run. This would cause the initial render of the tab panel to have tabIndex=0. Because of this, the FocusManager in the Popover component would include all tabpanels as focusable elements once it was mounted. The FocusManager should update it's list of focusable elements whenever it was updated, however, when the Tabs component is wrapped by another component and placed in a Popover, the componentDidUpdate lifecycle in FocusManager is not triggered.
TLDR: By making sure we don't initially set the tabindex=0 on the tabpanel element, the tab panels won't get included in the FocusManager logic!
There was a problem hiding this comment.
Note: There are some edge cases still with focus management in popovers + tabs. The long term fix would be to update FocusManager so it can keep track of whenever any descendant element becomes focusable/non-focusable. The focus management in Popover is going to be re-visited as part of the work to switch to floating-ui, which can hopefully address these issues! (related Slack thread)
For now, this PR addresses the main issues!
| // focusable element | ||
| if (ref.current) { | ||
| // focusable element (only if the tab panel has children) | ||
| if (ref.current && children) { |
There was a problem hiding this comment.
If children is falsey, it means that the tab panel content hasn't been rendered yet since the Tabs component renders the panel content conditionally
wonder-blocks/packages/wonder-blocks-tabs/src/components/tabs.tsx
Lines 395 to 412 in 0bb9648
This additional check makes it so the tab index on the tabpanel is not initialized until the children prop is provided with content. This prevents the FocusManager in Popover from including inactive tabpanels in it's list of focusable elements
(related to https://github.com/Khan/wonder-blocks/pull/2863/files#r2528858860)
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
Summary:
These changes address focus management issues when Tabs are used within a Popover. The issues were:
tabpanelelement in a Popover is focusable even when there are interactive elements in the tab panel.role="tab"in a Popover could be tabbed toSee PR comment annotations for more details on how they were addressed
Issue: WB-2151
Test plan:
Navigate to
?path=/story/packages-tabs-tabs-testing-tabs-playtesting--popover-with-tabsand confirm focus behaviour works as expected:Note: There remains an issue where inactive tabs can still be focusable if Tabs are wrapped with another component in a Popover (see related Slack thread for more details). The Popover focus logic will be revisited soon with the floating-ui work happening in parallel, so I've left that logic as is (TLDR of the issue: FocusManager in Popover doesn't have an up to date list of focusable elements when Tabs are wrapped)
Screen.Recording.2025-11-14.at.11.46.18.AM.mov