Skip to content

Fix Popover focus management when used with Tabs #2863

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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Fix Popover focus management when used with Tabs #2863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fad9b9b
[fix-popover-with-tabs] add repro story
beaesguerra Nov 13, 2025
1ff1233
[fix-popover-with-tabs] Only consider buttons without tabindex=-1 as …
beaesguerra Nov 13, 2025
188329d
[fix-popover-with-tabs] docs(changeset): Fix issue where Popover focu…
beaesguerra Nov 13, 2025
209b1af
[fix-popover-with-tabs] make story functional
beaesguerra Nov 13, 2025
9eec570
[fix-popover-with-tabs] only set tabindex to 0 for a tabpanel if we'v…
beaesguerra Nov 13, 2025
61224bd
[fix-popover-with-tabs] apply wb focus styling to the tab panel
beaesguerra Nov 13, 2025
0976e3d
[fix-popover-with-tabs] move story to tabs playtesting
beaesguerra Nov 13, 2025
412fed1
[fix-popover-with-tabs] docs(changeset): Fix issue where TabPanel is …
beaesguerra Nov 13, 2025
1b2f733
[fix-popover-with-tabs] add more tabs to example to test flow: going …
beaesguerra Nov 14, 2025
a28c480
[fix-popover-with-tabs] Add example of when tabs are wrapped in a com…
beaesguerra Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/serious-comics-mate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-blocks-tabs": patch
---

Fix issue where TabPanel is 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)
5 changes: 5 additions & 0 deletions .changeset/thirty-dolphins-battle.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-blocks-popover": patch
---

Fix issue where Popover focus management was interfering with Tabs focus management
127 changes: 127 additions & 0 deletions __docs__/wonder-blocks-tabs/tabs-testing-playtesting.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {PropsFor, View} from "@khanacademy/wonder-blocks-core";
import {PhosphorIcon} from "@khanacademy/wonder-blocks-icon";
import Button from "@khanacademy/wonder-blocks-button";
import {sizing} from "@khanacademy/wonder-blocks-tokens";
import {Popover, PopoverContentCore} from "@khanacademy/wonder-blocks-popover";

export default {
title: "Packages / Tabs / Tabs / Testing / Tabs - Playtesting",
Expand Down Expand Up @@ -137,3 +138,129 @@ export const DynamicIcon: Story = {
);
},
};

const TabsWrapperComponent = () => {
const [selectedTab, setSelectedTab] = React.useState("tab-1");
return (
<Tabs
aria-label="tabs"
selectedTabId={selectedTab}
onTabSelected={setSelectedTab}
tabs={[
{
label: "Tab 1",
id: "tab-1",
panel: (
<div>
Tab 1{" "}
<Button kind="secondary" size="small">
Focusable element
</Button>
</div>
),
},
{
label: "Tab 2",
id: "tab-2",
panel: (
<div>
Tab 2
<Button kind="secondary" size="small">
Focusable element
</Button>
</div>
),
},
{
label: "Tab 3",
id: "tab-3",
panel: <div>Tab 3</div>,
},
]}
/>
);
};
/**
* An example of a popover that has tabs inside of it.
*/
export const PopoverWithTabs = {
parameters: {
chromatic: {
disableSnapshot: true,
},
},
render: function Example() {
const [selectedTab, setSelectedTab] = React.useState("tab-1");
return (
<View style={{alignItems: "center", gap: sizing.size_960}}>
<Popover
content={() => (
<PopoverContentCore closeButtonVisible={true}>
<TabsWrapperComponent />
</PopoverContentCore>
)}
>
{({open}) => (
<Button onClick={open}>
Popover with wrapped tabs
</Button>
)}
</Popover>
<Popover
content={() => (
<PopoverContentCore closeButtonVisible={true}>
<Tabs
aria-label="tabs"
selectedTabId={selectedTab}
onTabSelected={setSelectedTab}
tabs={[
{
label: "Tab 1",
id: "tab-1",
panel: (
<div>
Tab 1{" "}
<Button
kind="secondary"
size="small"
>
Focusable element
</Button>
</div>
),
},
{
label: "Tab 2",
id: "tab-2",
panel: (
<div>
Tab 2
<Button
kind="secondary"
size="small"
>
Focusable element
</Button>
</div>
),
},
{
label: "Tab 3",
id: "tab-3",
panel: <div>Tab 3</div>,
},
]}
/>
</PopoverContentCore>
)}
>
{({open}) => (
<Button onClick={open}>
Popover with tabs as direct children
</Button>
)}
</Popover>
</View>
);
},
};
11 changes: 11 additions & 0 deletions packages/wonder-blocks-popover/src/util/__tests__/util.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,17 @@ describe("isFocusable", () => {
expect(result).toBe(true);
});

it("should mark a button with tabindex -1 as non-focusable", () => {
// Arrange
render(<button tabIndex={-1}>Open popover</button>);

// Act
const result = isFocusable(screen.getByRole("button"));

// Assert
expect(result).toBe(false);
});

it("should mark a div as non-focusable", () => {
// Arrange
render(<div>placeholder</div>);
Expand Down
2 changes: 1 addition & 1 deletion packages/wonder-blocks-popover/src/util/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* @see https://www.w3.org/TR/html5/editing.html#can-be-focused
*/
const FOCUSABLE_ELEMENTS =
'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])';
'button:not([tabindex="-1"]), [href], input, select, textarea, [tabindex]:not([tabindex="-1"])';
Copy link
Member Author

@beaesguerra beaesguerra Nov 14, 2025

Choose a reason for hiding this comment

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

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


export function findFocusableNodes(
root: HTMLElement | Document,
Expand Down
19 changes: 13 additions & 6 deletions packages/wonder-blocks-tabs/src/components/tab-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as React from "react";
import {addStyle, StyleType} from "@khanacademy/wonder-blocks-core";
import {StyleSheet} from "aphrodite";
import {focusStyles} from "@khanacademy/wonder-blocks-styles";
import {findFocusableNodes} from "../../../wonder-blocks-core/src/util/focus";

type Props = {
Expand Down Expand Up @@ -48,12 +49,16 @@ export const TabPanel = (props: Props) => {
} = props;

const ref = React.useRef<HTMLDivElement>(null);
const [hasFocusableElement, setHasFocusableElement] = React.useState(false);
// Initialize to null to indicate that the focusable element status is not
// yet determined
const [hasFocusableElement, setHasFocusableElement] = React.useState<
boolean | null
>(null);

React.useEffect(() => {
// Whenever tab panel contents change, determine if the panel has a
// focusable element
if (ref.current) {
// focusable element (only if the tab panel has children)
if (ref.current && children) {
Copy link
Member Author

@beaesguerra beaesguerra Nov 14, 2025

Choose a reason for hiding this comment

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

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

<TabPanel
key={tab.id}
id={getTabPanelId(tab.id)}
aria-labelledby={getTabId(tab.id)}
active={selectedTabId === tab.id}
testId={tab.testId && getTabPanelId(tab.testId)}
style={stylesProp?.tabPanel}
>
{/* Tab panel contents are rendered if the tab has
been previously visited or if mountAllPanels is enabled.
If mountAllPanels is off, it prevents unnecessary
re-mounting of tab panel contents when switching tabs.
Note that TabPanel will only display the contents if it
is the active panel. */}
{(mountAllPanels ||
visitedTabsRef.current.has(tab.id)) &&
tab.panel}
</TabPanel>

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)

setHasFocusableElement(findFocusableNodes(ref.current).length > 0);
}
}, [active, ref, children]);
Expand All @@ -64,9 +69,10 @@ export const TabPanel = (props: Props) => {
role="tabpanel"
id={id}
aria-labelledby={ariaLabelledby}
// If the tab panel doesn't have focusable elements, it should be
// focusable so that it is included in the tab sequence of the page
tabIndex={hasFocusableElement ? undefined : 0}
// 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}
Copy link
Member Author

@beaesguerra beaesguerra Nov 14, 2025

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

@beaesguerra beaesguerra Nov 14, 2025

Choose a reason for hiding this comment

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

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!

// Only show the tab panel if it is active
hidden={!active}
data-testid={testId}
Expand All @@ -82,5 +88,6 @@ const styles = StyleSheet.create({
tabPanel: {
// Apply flex so that panel supports rtl
display: "flex",
...focusStyles.focus,
Copy link
Member Author

@beaesguerra beaesguerra Nov 14, 2025

Choose a reason for hiding this comment

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

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

},
});
Loading