Skip to content
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.

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

fix(popover-container): ensure that tab sequence is not lost when container has radio buttons #7100

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/__internal__/focus-trap/focus-trap.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ const FocusTrap = ({
onFocus: updateCurrentFocusedElement,
});

// passes focusProps, sets tabIndex and onBlur if no tabIndex has been expicitly set on child
// passes focusProps, sets tabIndex and onBlur if no tabIndex has been explicitly set on child
Copy link
Contributor

Choose a reason for hiding this comment

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

question: since this isn't related to the commit should we separate it out as it's own chore commit?

const clonedChildren = React.Children.map(children, (child) => {
const focusableChild = child as React.ReactElement;
return React.cloneElement(
Expand Down
29 changes: 29 additions & 0 deletions src/components/popover-container/components.test-pw.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import Pill from "../pill";
import Badge from "../badge";
import Box from "../box";
import { Select, Option } from "../select";
import RadioButton, { RadioButtonGroup } from "../radio-button";
import PopoverContainer, {
PopoverContainerProps,
} from "./popover-container.component";
Expand Down Expand Up @@ -434,3 +435,31 @@ export const PopoverContainerFocusOrder = (
</Box>
);
};

export const WithRadioButtons = () => {
const [open1, setOpen1] = useState(false);
return (
<Box padding="25px" display="inline-flex">
<PopoverContainer
position="left"
onOpen={() => setOpen1(true)}
onClose={() => setOpen1(false)}
open={open1}
renderOpenComponent={({ ref, onClick }) => (
<Button aria-label="open" ref={ref} onClick={onClick}>
With Radio children
</Button>
)}
p={0}
>
<Box display="flex" justifyContent="space-between" p={2}>
<RadioButtonGroup name="bar">
<RadioButton value="1" label="radio 1" />
<RadioButton value="2" label="radio 2" />
</RadioButtonGroup>
</Box>
</PopoverContainer>
<Button>foo</Button>
</Box>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import Typography from "../typography";
import Search from "../search";
import IconButton from "../icon-button";
import Icon from "../icon";
import RadioButton, { RadioButtonGroup } from "../radio-button";

export default {
title: "Popover Container/Test",
Expand All @@ -22,6 +23,7 @@ export default {
"InsideMenu",
"InsideMenuWithOpenButton",
"WithFullWidthButton",
"WithRadioButtons",
],
parameters: {
info: { disable: true },
Expand Down Expand Up @@ -246,3 +248,32 @@ export const WithFullWidthButton = () => {
</PopoverContainer>
);
};

export const WithRadioButtons = () => {
const [open1, setOpen1] = useState(false);

return (
<Box padding="25px" display="inline-flex">
<PopoverContainer
position="left"
onOpen={() => setOpen1(true)}
onClose={() => setOpen1(false)}
open={open1}
renderOpenComponent={({ ref, onClick }) => (
<Button aria-label="Notifications" ref={ref} onClick={onClick}>
With Radio children
</Button>
)}
p={0}
>
<Box display="flex" justifyContent="space-between" p={2}>
<RadioButtonGroup name="bar">
<RadioButton value="1" label="radio 1" />
<RadioButton value="2" label="radio 2" />
</RadioButtonGroup>
</Box>
</PopoverContainer>
<Button>foo</Button>
</Box>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import useFocusPortalContent from "../../hooks/__internal__/useFocusPortalConten
import tagComponent, {
TagProps,
} from "../../__internal__/utils/helpers/tags/tags";
import { defaultFocusableSelectors } from "../../__internal__/focus-trap/focus-trap-utils";

export interface RenderOpenProps {
tabIndex: number;
Expand Down Expand Up @@ -272,6 +273,38 @@ export const PopoverContainer = ({
closePopover,
);

const onFocusNextElement = useCallback((ev) => {
const allFocusableElements: HTMLElement[] = Array.from(
document.querySelectorAll(defaultFocusableSelectors) ||
/* istanbul ignore next */ [],
);
const filteredElements = allFocusableElements.filter(
(el) => el === openButtonRef.current || Number(el.tabIndex) !== -1,
);

const openButtonRefIndex = filteredElements.indexOf(
openButtonRef.current as HTMLElement,
);

filteredElements[openButtonRefIndex + 1].focus();
closePopover(ev);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

const handleFocusGuard = (
direction: "prev" | "next",
ev: React.FocusEvent<HTMLElement>,
) => {
if (direction === "next" && onFocusNextElement) {
// Focus the next focusable element outside of the popover
onFocusNextElement(ev);
return;
}

// istanbul ignore else
if (direction === "prev") openButtonRef.current?.focus();
};

const renderOpenComponentProps = {
tabIndex: 0,
"aria-expanded": isOpen,
Expand Down Expand Up @@ -335,7 +368,23 @@ export const PopoverContainer = ({
</FocusTrap>
</ModalContext.Provider>
) : (
popover()
<>
<div
data-element="tab-guard-top"
// eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex
tabIndex={0}
aria-hidden
onFocus={(ev) => handleFocusGuard("prev", ev)}
/>
{popover()}
<div
data-element="tab-guard-bottom"
// eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex
tabIndex={0}
aria-hidden
onFocus={(ev) => handleFocusGuard("next", ev)}
/>
</>
);

return (
Expand Down
20 changes: 20 additions & 0 deletions src/components/popover-container/popover-container.pw.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
WithRenderCloseButtonComponent,
PopoverContainerComponentCoverButton,
PopoverContainerFocusOrder,
WithRadioButtons,
} from "../popover-container/components.test-pw";
import Portrait from "../portrait";

Expand Down Expand Up @@ -624,6 +625,25 @@ test.describe("Check props of Popover Container component", () => {
await expect(third).toBeFocused();
});

test("should focus the next focusable element outside of the container once finished keyboard navigating through the container's content", async ({
mount,
page,
}) => {
await mount(<WithRadioButtons />);

const openButton = page.getByRole("button", { name: "open" });
const container = popoverContainerContent(page);
const additionalButton = page.getByRole("button", { name: "foo" });

await openButton.click();
await page.keyboard.press("Tab"); // focus on first radio button
await page.keyboard.press("Tab"); // focus on close icon
await page.keyboard.press("Tab"); // focus outside of container and on to additional button

await expect(container).not.toBeVisible();
await expect(additionalButton).toBeFocused();
});

test.describe("Accessibility tests", () => {
test("should check accessibility for Default example", async ({
mount,
Expand Down
85 changes: 85 additions & 0 deletions src/components/popover-container/popover-container.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import { testStyledSystemPadding } from "../../__spec_helper__/__internal__/test
import PopoverContainer from "./popover-container.component";
import { Select, Option } from "../select";
import useMediaQuery from "../../hooks/useMediaQuery";
import Button from "../button";
import RadioButton, { RadioButtonGroup } from "../radio-button";

jest.mock("../../hooks/useMediaQuery");

Expand Down Expand Up @@ -531,6 +533,89 @@ describe("closing the popup", () => {
});
});

test("when content is navigated via keyboard, the next focusable item should be focused and popup closed", async () => {
const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime });
render(
<>
<PopoverContainer
position="left"
renderOpenComponent={({ ref, onClick }) => (
<Button aria-label="open button" ref={ref} onClick={onClick}>
Open
</Button>
)}
>
<RadioButtonGroup name="bar">
<RadioButton value="1" label="radio 1" />
<RadioButton value="2" label="radio 2" />
</RadioButtonGroup>
</PopoverContainer>
<Button>Example Button</Button>
</>,
);

const openButton = screen.getByRole("button", { name: "open button" });
await user.click(openButton);
await user.tab(); // tab to close icon
await user.tab(); // tab to RadioButtonGroup
await user.tab(); // tab to Example Button (outside of popup)

const popup = await screen.findByRole("dialog");
await waitFor(() => expect(popup).not.toBeVisible());

const exampleButton = screen.getByRole("button", { name: "Example Button" });
expect(exampleButton).toHaveFocus();
});

test("when the popover is opened and shift tab key is pressed, the open button should be focused and popup closed", async () => {
const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime });
render(
<>
<PopoverContainer
position="left"
renderOpenComponent={({ ref, onClick }) => (
<Button aria-label="open button" ref={ref} onClick={onClick}>
Open
</Button>
)}
>
<RadioButtonGroup name="bar">
<RadioButton value="1" label="radio 1" />
<RadioButton value="2" label="radio 2" />
</RadioButtonGroup>
</PopoverContainer>
<Button>Example Button</Button>
</>,
);

const openButton = screen.getByRole("button", { name: "open button" });
await user.click(openButton);
await user.tab(); // tab to close icon
await user.tab(); // tab to content
await user.tab({ shift: true }); // shift tab back to close icon
await user.tab({ shift: true }); // shift tab back to the opening trigger element

const popup = await screen.findByRole("dialog");
await waitFor(() => expect(popup).not.toBeVisible());
expect(openButton).toHaveFocus();
});

test("if only the open trigger is the only focusable element on screen, when the popover is opened and tab key is used to navigate content, it should navigate back to the opening trigger", async () => {
const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime });
render(
<>
<PopoverContainer title="My popup" open />
</>,
);

const openButton = screen.getByRole("button", { name: "My popup" });
await user.click(openButton);
await user.tab(); // tab to close icon
await user.tab(); // tab back out of content to the opening trigger element

expect(openButton).toHaveFocus();
});

testStyledSystemPadding(
(props) => (
<PopoverContainer open title="My popup" {...props}>
Expand Down
Loading