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

DSD-1740/close dropdown multiselect #1607

Merged
merged 11 commits into from
Jun 25, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ Currently, this repo is in Prerelease. When it is released, this project will ad
- Exports `useMediaQuery` and `chakra` from Chakra UI.
- Updates `Accordion` to close focused panel when "esc" key is pressed.
- Updates the `TextInput` and `SearchBar` components to better associate the input element to the entire component's helper text.
- Updates `Accordion` to close panel when element within panel is focused and "esc" key is pressed.

## 3.1.4 (May 23, 2024)

Expand Down
2 changes: 1 addition & 1 deletion src/components/Accordion/Accordion.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ const accordionData = [
- The `ariaLabel` key available within the `accordionData` array entries can set
a unique value for each accordion within an accordion group.
- If the user presses the 'esc' key and focus is on an open accordion panel,
that panel will close.
that panel will close and focus will return to the accordion's button.

Resources:

Expand Down
74 changes: 58 additions & 16 deletions src/components/Accordion/Accordion.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,13 @@ export const accordionData = [
label: "Tom Nook",
panel: (
<p>
Tom Nook, <b>known in Japan as Tanukichi</b>, is a fictional character
in the Animal Crossing series who operates the village store.
Tom Nook,
<b>
{/* eslint-disable-next-line jsx-a11y/anchor-is-valid */}
known in <a href="#">Japan</a> as Tanukichi
</b>
, is a fictional character in the Animal Crossing series who operates
the village store.
</p>
),
},
Expand Down Expand Up @@ -146,41 +151,55 @@ describe("Accordion", () => {

const accordionLabel = screen.getByRole("button", { name: "Tom Nook" });
let accordionPanelContent = screen.queryByText(
/known in Japan as Tanukichi/i
/operates the village store/i
);
expect(accordionLabel).toHaveAttribute("aria-expanded", "false");
// The panel's content should not be in the DOM unless the Accordion is open.
expect(accordionPanelContent).not.toBeInTheDocument();

userEvent.click(accordionLabel);

accordionPanelContent = screen.queryByText(/known in Japan as Tanukichi/i);
accordionPanelContent = screen.queryByText(/operates the village store/i);
expect(accordionLabel).toHaveAttribute("aria-expanded", "true");
expect(accordionPanelContent).toBeInTheDocument();
});

it("closes the accordion when the 'esc' key is pressed", async () => {
render(<Accordion accordionData={[accordionData[0]]} isDefaultOpen />);
it("closes the accordion when the button is in focus and the 'esc' key is pressed", async () => {
render(<Accordion accordionData={[accordionData[0]]} />);
Copy link
Member

Choose a reason for hiding this comment

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

Can you add another test where the entire accordionData array is passed, so now there are three accordions. For one of the accordions, add a button or checkbox, focus on it, and then press escape. This is to test the new feature in this PR but also that it works when there are more than just one accordion.

Copy link
Member

Choose a reason for hiding this comment

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

I know there's a test that does this for MultiSelect but the feature is in the Accordion so it should also be tested there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!


const accordionLabel = screen.getByRole("button", { name: "Tom Nook" });
let accordionPanelContent = screen.queryByText(
/known in Japan as Tanukichi/i
);
expect(accordionLabel).toHaveAttribute("aria-expanded", "true");
expect(accordionPanelContent).toBeInTheDocument();
const accordionButton = screen.getByRole("button");

await userEvent.type(accordionLabel, "[Escape]");
await userEvent.click(accordionButton);

expect(accordionLabel).toHaveAttribute("aria-expanded", "false");
expect(accordionPanelContent).not.toBeInTheDocument();
expect(accordionButton.getAttribute("aria-expanded")).toEqual("true");

await userEvent.keyboard("[Escape]");

expect(accordionButton).toHaveAttribute("aria-expanded", "false");
});

it("closes the accordion when an element in the panel is focused and the 'esc' key is pressed", async () => {
render(<Accordion accordionData={[accordionData[0]]} />);

const accordionButton = screen.getByRole("button");

await userEvent.click(accordionButton);

expect(accordionButton.getAttribute("aria-expanded")).toEqual("true");

const linkInPanel = screen.getByRole("link");

await userEvent.type(linkInPanel, "[Escape]");

expect(accordionButton).toHaveAttribute("aria-expanded", "false");
});

it("always renders its content when isAlwaysRendered is true", () => {
render(<Accordion accordionData={[accordionData[0]]} isAlwaysRendered />);

const accordionLabel = screen.getByRole("button", { name: "Tom Nook" });
let accordionPanelContent = screen.queryByText(
/known in Japan as Tanukichi/i
/operates the village store/i
);
expect(accordionLabel).toHaveAttribute("aria-expanded", "false");
expect(accordionPanelContent).toBeInTheDocument();
Expand Down Expand Up @@ -226,6 +245,29 @@ describe("Accordion", () => {
expect(accordion3).toHaveAttribute("aria-expanded", "true");
});

it("closes only the focused accordion when there are multiple accordions and the 'esc' key is pressed", async () => {
render(<Accordion accordionData={accordionData} />);

const accordion1 = screen.getByRole("button", { name: "Tom Nook" });
const accordion3 = screen.getByRole("button", { name: "K.K. Slider" });

expect(accordion1).toHaveAttribute("aria-expanded", "false");
expect(accordion3).toHaveAttribute("aria-expanded", "false");

userEvent.click(accordion1);
userEvent.click(accordion3);

expect(accordion1).toHaveAttribute("aria-expanded", "true");
expect(accordion3).toHaveAttribute("aria-expanded", "true");

const linkInPanel1 = screen.getByRole("link");

userEvent.type(linkInPanel1, "[Escape]");

expect(accordion1).toHaveAttribute("aria-expanded", "false");
expect(accordion3).toHaveAttribute("aria-expanded", "true");
});

describe("Check aria-label values", () => {
it("renders with attribute generated from component prop", () => {
render(
Expand Down
39 changes: 32 additions & 7 deletions src/components/Accordion/Accordion.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -224,16 +224,41 @@ export const Accordion: ChakraComponent<
isDefaultOpen ? [0] : []
);

// If the accordionData doesn't already contain refs for the panel
// buttons, add them now.
const updatedAccordionData = accordionData.map((item) => ({
...item,
buttonInteractionRef: item.buttonInteractionRef || React.createRef(),
}));

const handleKeyDown = (e) => {
// If the 'esc' key is pressed,
if (e.keyCode === 27) {
const focusedPanelIndex = Number(e.target.dataset.index);
// collapse the currently focused panel.
// (Nothing will happen if the currently
// focused panel is already collapsed.)
// If the 'esc' key is pressed, find the panel the
// user is focused on or within, and remove it as
// an expanded panel. (Nothing will happen if the
// panel is already collapsed.)
if (e.code === "Escape") {
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why the update from e.keyCode to e.code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Two reasons. 1) I thought it was clearer than. using a keyCode, and 2) for some reason in the tests, the keyCode for Escape was a different number than it is in the code? So I used the code 'Escape' instead for the tests and wanted to be consistent.

let focusedPanelIndex;
if (e.target.dataset.index) {
// If the user is focused on an accordion button...
focusedPanelIndex = Number(e.target.dataset.index);
} else {
// If the user is focused on an element within the panel...
focusedPanelIndex = Number(
e.target.closest("[role='region']").id.split("-").pop()
Copy link
Member

Choose a reason for hiding this comment

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

Nice, but also brittle if the DOM and/or id ever changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know :/, I didn't want to do it this way, but it seemed like the best option...the id is really the only way the panel is connected to the button that needs to close.

);
}

setExpandedPanels(
expandedPanels.filter((i) => i !== focusedPanelIndex)
);

// If something *inside* the accordion was in focus and 'esc' was clicked,
// return focus to the accordion panel
if (updatedAccordionData[focusedPanelIndex].buttonInteractionRef) {
updatedAccordionData[
focusedPanelIndex
].buttonInteractionRef.current.focus();
}
}
};

Expand All @@ -248,7 +273,7 @@ export const Accordion: ChakraComponent<
{...rest}
>
{getElementsFromData(
accordionData,
updatedAccordionData,
ariaLabel,
id,
isAlwaysRendered,
Expand Down
5 changes: 4 additions & 1 deletion src/components/Accordion/accordionChangelogData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ export const changelogData: ChangelogData[] = [
version: "3.1.4",
type: "Update",
affects: ["Accessibility", "Functionality"],
notes: ["Adds logic to close accordion when 'esc' key is pressed"],
notes: [
"Adds logic to close accordion when accordion button is focused and 'esc' key is pressed",
"Adds logic to close accordion when element within panel is focused and 'esc' key is pressed",
],
},
{
date: "2024-03-14",
Expand Down
3 changes: 3 additions & 0 deletions src/components/MultiSelect/MultiSelect.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ Accordion:
- Use the `aria-controls` attribute to associate the control with the panel.
- The open and close icons are decorative (`aria-hidden` is `true`).
- Visible focus goes around full button and full button is clickable.
- Because the `MultiSelect` utilizes the `Accordion`, the same functionality applies when
the user presses the 'esc' key and focus is on an open panel: that panel will close and
focus will return to the panel's button.

Checkbox:

Expand Down
28 changes: 28 additions & 0 deletions src/components/MultiSelect/MultiSelect.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,34 @@ describe.skip("MultiSelect", () => {
).not.toBeInTheDocument();
});

it("closes the multiselect when the 'esc' key is pressed", async () => {
render(
<MultiSelect
id="multiselect-test-id"
buttonText="Multiselect button text"
defaultItemsVisible={defaultItemsVisible}
items={items}
isDefaultOpen={true}
isSearchable={false}
isBlockElement={false}
selectedItems={selectedTestItems}
onChange={() => null}
onClear={() => null}
/>
);

const multiSelectButton = screen.getByRole("button");
const multiSelectCheckbox = screen.getByRole("checkbox", { name: /dogs/i });

expect(multiSelectButton.getAttribute("aria-expanded")).toEqual("true");

await userEvent.click(multiSelectCheckbox);

await userEvent.keyboard("[Escape]");

expect(multiSelectButton).toHaveAttribute("aria-expanded", "false");
});

it("should allow user to toggle menu by clicking menu button or use the 'Enter'/'Spacebar' key", () => {
render(
<MultiSelect
Expand Down
8 changes: 4 additions & 4 deletions src/components/MultiSelect/MultiSelect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
ChakraComponent,
useMultiStyleConfig,
} from "@chakra-ui/react";
import React, { useState, forwardRef, useRef } from "react";
import React, { forwardRef, useRef, useState } from "react";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was not sure where to put this comment, but the focus needs to be addressed after the ESC key is pressed when a checkbox has focus. In that scenario, the focus should return to the main button after the ESC key is pressed.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point. If it involves having state or ref we should talk about this because it's getting overly complex and from what I understood, accordions don't generally behave this way. So if we don't have to implement this then perhaps we should not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need it for Accordion, but we do need it for MultiSelect.

Copy link
Member

Choose a reason for hiding this comment

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

That's true but the Accordion is a big part of the MultiSelect and, to me, MultiSelect is inheriting all the features of Accordion. There is currently a ref that is being passed from the MultiSelect to the Accordion to keep track of focus for when the items count button is clicked. This could be used as well when the esc key is pressed. But it we have to keep track of pressing the ESC key in the MultiSelect to close the Accordion, then it feels like the work is being duplicated because the Accordion already does check for when the ESC key is pressed.

Perhaps we should reconsider and move all the logic just to the MultiSelect from the Accordion. @oliviawongnyc As you try to implement the focus management in the Accordion, let me know if my comment makes sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay @EdwinGuzman and @bigfishdesign13, focus should now return to the button when the Accordion (or MultiSelect) is closed with the esc key.


import Accordion from "./../Accordion/Accordion";
import Button from "./../Button/Button";
Expand Down Expand Up @@ -95,7 +95,7 @@ export const MultiSelect: ChakraComponent<

// Create a ref to hold a reference to the accordian button, enabling us
// to programmatically focus it.
const accordianButtonRef: React.RefObject<HTMLDivElement> =
const accordionButtonRef: React.RefObject<HTMLDivElement> =
useRef<HTMLDivElement>();
const expandToggleButtonRef: React.RefObject<HTMLButtonElement> =
useRef<HTMLButtonElement>();
Expand Down Expand Up @@ -364,7 +364,7 @@ export const MultiSelect: ChakraComponent<
{
accordionType: "default",
// Pass the ref for interaction with the accordion button.
buttonInteractionRef: accordianButtonRef,
buttonInteractionRef: accordionButtonRef,
label: accordionLabel,
panel: accordionPanel,
},
Expand All @@ -387,7 +387,7 @@ export const MultiSelect: ChakraComponent<
selectedItemsString={selectedItemsString}
selectedItemsCount={selectedItemsCount}
onClear={onClear}
accordianButtonRef={accordianButtonRef}
accordionButtonRef={accordionButtonRef}
/>
)}
</Box>
Expand Down
6 changes: 3 additions & 3 deletions src/components/MultiSelect/MultiSelectItemsCountButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export interface MultiSelectItemsCountButtonProps {
/** The action to perform for key down event. */
onKeyDown?: () => void;
/** Ref to the Accordion Button element. */
accordianButtonRef: any;
accordionButtonRef: any;
}

/**
Expand All @@ -42,7 +42,7 @@ const MultiSelectItemsCountButton = forwardRef<
isOpen,
multiSelectId,
multiSelectLabelText,
accordianButtonRef,
accordionButtonRef,
onClear,
selectedItemsString,
selectedItemsCount,
Expand All @@ -66,7 +66,7 @@ const MultiSelectItemsCountButton = forwardRef<
onClear && onClear();
// Set focus on the Accordion Button when close the
// selected items count button.
accordianButtonRef.current?.focus();
accordionButtonRef.current?.focus();
}}
__css={styles}
>
Expand Down
Loading