Skip to content

Commit 5db127a

Browse files
beaesguerrabeaesguerra
authored
Fix Popover focus management when used with Tabs (#2863)
## Summary: These changes address focus management issues when Tabs are used within a Popover. The issues were: - The `tabpanel` element in a Popover is focusable even when there are interactive elements in the tab panel. - Expected behaviour: The tab panel should only be focusable if there are no interactive elements (See [Tabs with Focusable Content docs](https://khan.github.io/wonder-blocks/?path=/docs/packages-tabs-tabs--docs#with-focusable-content) for more details) - All the elements with `role="tab"` in a Popover could be tabbed to - Expected behaviour: A user should not be able to press "Tab" to navigate to inactive tabs (switching between tabs uses arrow key navigation) See 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-tabs` and confirm focus behaviour works as expected: - Tab panel is only focusable when there are no interactive elements in the tab panel - Tabs can only be focused on via left/right arrow keys (not by pressing tabs) 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](https://khanacademy.slack.com/archives/C8Z9DSKC7/p1763146396757689?thread_ts=1762983797.611719&cid=C8Z9DSKC7) 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) - Popover and Tabs components on its own should continue to work as expected in terms of keyboard navigation and tab order https://github.com/user-attachments/assets/e54f311b-cb69-4de3-82d1-37c4099ff5a3 Author: beaesguerra Reviewers: beaesguerra, marcysutton Required Reviewers: Approved By: marcysutton Checks: ✅ 12 checks were successful, ⏭️ 3 checks have been skipped Pull Request URL: #2863
1 parent 42c0f72 commit 5db127a

File tree

6 files changed

+162
-7
lines changed

6 files changed

+162
-7
lines changed

‎.changeset/serious-comics-mate.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@khanacademy/wonder-blocks-tabs": patch
3+
---
4+
5+
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). Also apply WB focus styling to the tabpanel element since it is focusbale when there are no interactive elements in it.

‎.changeset/thirty-dolphins-battle.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@khanacademy/wonder-blocks-popover": patch
3+
---
4+
5+
Fix issue where Popover focus management was interfering with Tabs focus management

‎__docs__/wonder-blocks-tabs/tabs-testing-playtesting.stories.tsx‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {PropsFor, View} from "@khanacademy/wonder-blocks-core";
88
import {PhosphorIcon} from "@khanacademy/wonder-blocks-icon";
99
import Button from "@khanacademy/wonder-blocks-button";
1010
import {sizing} from "@khanacademy/wonder-blocks-tokens";
11+
import {Popover, PopoverContentCore} from "@khanacademy/wonder-blocks-popover";
1112

1213
export default {
1314
title: "Packages / Tabs / Tabs / Testing / Tabs - Playtesting",
@@ -137,3 +138,129 @@ export const DynamicIcon: Story = {
137138
);
138139
},
139140
};
141+
142+
const TabsWrapperComponent = () => {
143+
const [selectedTab, setSelectedTab] = React.useState("tab-1");
144+
return (
145+
<Tabs
146+
aria-label="tabs"
147+
selectedTabId={selectedTab}
148+
onTabSelected={setSelectedTab}
149+
tabs={[
150+
{
151+
label: "Tab 1",
152+
id: "tab-1",
153+
panel: (
154+
<div>
155+
Tab 1{" "}
156+
<Button kind="secondary" size="small">
157+
Focusable element
158+
</Button>
159+
</div>
160+
),
161+
},
162+
{
163+
label: "Tab 2",
164+
id: "tab-2",
165+
panel: (
166+
<div>
167+
Tab 2
168+
<Button kind="secondary" size="small">
169+
Focusable element
170+
</Button>
171+
</div>
172+
),
173+
},
174+
{
175+
label: "Tab 3",
176+
id: "tab-3",
177+
panel: <div>Tab 3</div>,
178+
},
179+
]}
180+
/>
181+
);
182+
};
183+
/**
184+
* An example of a popover that has tabs inside of it.
185+
*/
186+
export const PopoverWithTabs = {
187+
parameters: {
188+
chromatic: {
189+
disableSnapshot: true,
190+
},
191+
},
192+
render: function Example() {
193+
const [selectedTab, setSelectedTab] = React.useState("tab-1");
194+
return (
195+
<View style={{alignItems: "center", gap: sizing.size_960}}>
196+
<Popover
197+
content={() => (
198+
<PopoverContentCore closeButtonVisible={true}>
199+
<TabsWrapperComponent />
200+
</PopoverContentCore>
201+
)}
202+
>
203+
{({open}) => (
204+
<Button onClick={open}>
205+
Popover with wrapped tabs
206+
</Button>
207+
)}
208+
</Popover>
209+
<Popover
210+
content={() => (
211+
<PopoverContentCore closeButtonVisible={true}>
212+
<Tabs
213+
aria-label="tabs"
214+
selectedTabId={selectedTab}
215+
onTabSelected={setSelectedTab}
216+
tabs={[
217+
{
218+
label: "Tab 1",
219+
id: "tab-1",
220+
panel: (
221+
<div>
222+
Tab 1{" "}
223+
<Button
224+
kind="secondary"
225+
size="small"
226+
>
227+
Focusable element
228+
</Button>
229+
</div>
230+
),
231+
},
232+
{
233+
label: "Tab 2",
234+
id: "tab-2",
235+
panel: (
236+
<div>
237+
Tab 2
238+
<Button
239+
kind="secondary"
240+
size="small"
241+
>
242+
Focusable element
243+
</Button>
244+
</div>
245+
),
246+
},
247+
{
248+
label: "Tab 3",
249+
id: "tab-3",
250+
panel: <div>Tab 3</div>,
251+
},
252+
]}
253+
/>
254+
</PopoverContentCore>
255+
)}
256+
>
257+
{({open}) => (
258+
<Button onClick={open}>
259+
Popover with tabs as direct children
260+
</Button>
261+
)}
262+
</Popover>
263+
</View>
264+
);
265+
},
266+
};

‎packages/wonder-blocks-popover/src/util/__tests__/util.test.tsx‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,17 @@ describe("isFocusable", () => {
1414
expect(result).toBe(true);
1515
});
1616

17+
it("should mark a button with tabindex -1 as non-focusable", () => {
18+
// Arrange
19+
render(<button tabIndex={-1}>Open popover</button>);
20+
21+
// Act
22+
const result = isFocusable(screen.getByRole("button"));
23+
24+
// Assert
25+
expect(result).toBe(false);
26+
});
27+
1728
it("should mark a div as non-focusable", () => {
1829
// Arrange
1930
render(<div>placeholder</div>);

‎packages/wonder-blocks-popover/src/util/util.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* @see https://www.w3.org/TR/html5/editing.html#can-be-focused
44
*/
55
const FOCUSABLE_ELEMENTS =
6-
'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])';
6+
'button:not([tabindex="-1"]), [href], input, select, textarea, [tabindex]:not([tabindex="-1"])';
77

88
export function findFocusableNodes(
99
root: HTMLElement | Document,

‎packages/wonder-blocks-tabs/src/components/tab-panel.tsx‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import * as React from "react";
22
import {addStyle, StyleType} from "@khanacademy/wonder-blocks-core";
33
import {StyleSheet} from "aphrodite";
4+
import {focusStyles} from "@khanacademy/wonder-blocks-styles";
45
import {findFocusableNodes} from "../../../wonder-blocks-core/src/util/focus";
56

67
type Props = {
@@ -48,12 +49,16 @@ export const TabPanel = (props: Props) => {
4849
} = props;
4950

5051
const ref = React.useRef<HTMLDivElement>(null);
51-
const [hasFocusableElement, setHasFocusableElement] = React.useState(false);
52+
// Initialize to null to indicate that the focusable element status is not
53+
// yet determined
54+
const [hasFocusableElement, setHasFocusableElement] = React.useState<
55+
boolean | null
56+
>(null);
5257

5358
React.useEffect(() => {
5459
// Whenever tab panel contents change, determine if the panel has a
55-
// focusable element
56-
if (ref.current) {
60+
// focusable element (only if the tab panel has children)
61+
if (ref.current && children) {
5762
setHasFocusableElement(findFocusableNodes(ref.current).length > 0);
5863
}
5964
}, [active, ref, children]);
@@ -64,9 +69,10 @@ export const TabPanel = (props: Props) => {
6469
role="tabpanel"
6570
id={id}
6671
aria-labelledby={ariaLabelledby}
67-
// If the tab panel doesn't have focusable elements, it should be
68-
// focusable so that it is included in the tab sequence of the page
69-
tabIndex={hasFocusableElement ? undefined : 0}
72+
// If the tab panel doesn't have focusable elements after being
73+
// determined, it should be focusable so that it is included in the
74+
// tab sequence of the page
75+
tabIndex={hasFocusableElement === false ? 0 : undefined}
7076
// Only show the tab panel if it is active
7177
hidden={!active}
7278
data-testid={testId}
@@ -82,5 +88,6 @@ const styles = StyleSheet.create({
8288
tabPanel: {
8389
// Apply flex so that panel supports rtl
8490
display: "flex",
91+
...focusStyles.focus,
8592
},
8693
});

0 commit comments

Comments
 (0)