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.

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

FEI-5533: Re-enable select keyboard tests for Dropdown and Clickable #2420

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

FEI-5533: Re-enable select keyboard tests for Dropdown and Clickable #2420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25bbdf2
Fix some keyboard tests with normalized keyCodes
marcysutton Jan 7, 2025
631850f
docs(changeset): Fixes keyboard tests in Dropdown and Clickable
marcysutton Jan 7, 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
7 changes: 7 additions & 0 deletions .changeset/mean-cherries-press.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@khanacademy/wonder-blocks-clickable": patch
"@khanacademy/wonder-blocks-dropdown": patch
"@khanacademy/wonder-blocks-core": patch
---

Fixes keyboard tests in Dropdown and Clickable
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: It would be helpful to include details in the changeset around the changes to the components since it will show up in the package changelog (dropdown package example)! That way, it can also communicate changes in behaviour that consuming apps may want to know about.

For example, in ClickableBehaviour, it looks like we now check event.key instead of event.which or event.keyCode. This might impact tests in other projects so it would be helpful to come back to the changelog and see what changes could cause new behaviour!

22 changes: 22 additions & 0 deletions __docs__/wonder-blocks-dropdown/single-select.accessibility.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,25 @@ export const UsingAriaAttributes = {
render: SingleSelectAccessibility.bind({}),
name: "Using aria attributes",
};

const SingleSelectKeyboardSelection = () => {
const [selectedValue, setSelectedValue] = React.useState("");
return (
<View>
<SingleSelect
placeholder="Choose"
onChange={setSelectedValue}
selectedValue={selectedValue}
>
<OptionItem label="apple" value="apple" />
<OptionItem label="orange" value="orange" />
<OptionItem label="pear" value="pear" />
</SingleSelect>
</View>
);
};

export const UsingKeyboardSelection = {
Copy link
Member

@beaesguerra beaesguerra Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do we want to disable this story in Chromatic tests? It seems similar to other SingleSelect stories!

Also, do we want to highlight anything around keyboard selection for accessibility? We can reference stories in the single-select.accessibility.mdx file to document anything we want to highlight!

render: SingleSelectKeyboardSelection.bind({}),
name: "Using the keyboard",
};
93 changes: 44 additions & 49 deletions packages/wonder-blocks-clickable/src/components/__tests__/clickable-behavior.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,12 @@ import * as React from "react";
import {render, screen, fireEvent, waitFor} from "@testing-library/react";
import {MemoryRouter, Switch, Route} from "react-router-dom";
import {userEvent} from "@testing-library/user-event";
import {keys} from "@khanacademy/wonder-blocks-core";

import getClickableBehavior from "../../util/get-clickable-behavior";
import ClickableBehavior from "../clickable-behavior";
import type {ClickableState} from "../clickable-behavior";

const keyCodes = {
tab: 9,
enter: 13,
space: 32,
} as const;

const labelForState = (state: ClickableState): string => {
const labels: Array<any> = [];
if (state.hovered) {
Expand Down Expand Up @@ -195,8 +190,8 @@ describe("ClickableBehavior", () => {
);
const button = await screen.findByRole("button");
expect(button).not.toHaveTextContent("focused");
fireEvent.keyDown(button, {keyCode: keyCodes.space});
fireEvent.keyUp(button, {keyCode: keyCodes.space});
fireEvent.keyDown(button, {key: keys.space});
fireEvent.keyUp(button, {key: keys.space});
// NOTE(kevinb): await userEvent.click() fires other events that we don't want
// affecting this test case.
fireEvent.click(button);
Expand All @@ -219,8 +214,8 @@ describe("ClickableBehavior", () => {
);
const button = await screen.findByRole("button");
expect(button).not.toHaveTextContent("focused");
fireEvent.keyDown(button, {keyCode: keyCodes.space});
fireEvent.keyUp(button, {keyCode: keyCodes.space});
await userEvent.tab();
await userEvent.keyboard("{space}");
// NOTE(kevinb): await userEvent.click() fires other events that we don't want
// affecting this test case.
fireEvent.click(button);
Expand All @@ -244,14 +239,14 @@ describe("ClickableBehavior", () => {
);
const button = await screen.findByRole("button");
expect(button).not.toHaveTextContent("pressed");
fireEvent.keyDown(button, {keyCode: keyCodes.space});
fireEvent.keyDown(button, {key: keys.space});
expect(button).toHaveTextContent("pressed");
fireEvent.keyUp(button, {keyCode: keyCodes.space});
fireEvent.keyUp(button, {key: keys.space});
expect(button).not.toHaveTextContent("pressed");

fireEvent.keyDown(button, {keyCode: keyCodes.enter});
fireEvent.keyDown(button, {key: keys.enter});
expect(button).toHaveTextContent("pressed");
fireEvent.keyUp(button, {keyCode: keyCodes.enter});
fireEvent.keyUp(button, {key: keys.enter});
expect(button).not.toHaveTextContent("pressed");
});

Expand Down Expand Up @@ -280,14 +275,14 @@ describe("ClickableBehavior", () => {
);
const link = await screen.findByRole("link");
expect(link).not.toHaveTextContent("pressed");
fireEvent.keyDown(link, {keyCode: keyCodes.enter});
fireEvent.keyDown(link, {key: keys.enter});
expect(link).toHaveTextContent("pressed");
fireEvent.keyUp(link, {keyCode: keyCodes.enter});
fireEvent.keyUp(link, {key: keys.enter});
expect(link).not.toHaveTextContent("pressed");

fireEvent.keyDown(link, {keyCode: keyCodes.space});
fireEvent.keyDown(link, {key: keys.space});
expect(link).not.toHaveTextContent("pressed");
fireEvent.keyUp(link, {keyCode: keyCodes.space});
fireEvent.keyUp(link, {key: keys.space});
expect(link).not.toHaveTextContent("pressed");
});

Expand Down Expand Up @@ -462,19 +457,19 @@ describe("ClickableBehavior", () => {

expect(button).not.toHaveTextContent("focused");
fireEvent.keyUp(button, {
keyCode: keyCodes.tab,
key: keys.tab,
});
expect(button).not.toHaveTextContent("focused");
fireEvent.keyDown(button, {keyCode: keyCodes.tab});
fireEvent.keyDown(button, {key: keys.tab});
expect(button).not.toHaveTextContent("focused");

expect(button).not.toHaveTextContent("pressed");
fireEvent.keyDown(button, {keyCode: keyCodes.space});
fireEvent.keyDown(button, {key: keys.space});
expect(button).not.toHaveTextContent("pressed");
fireEvent.keyUp(button, {keyCode: keyCodes.space});
fireEvent.keyUp(button, {key: keys.space});
expect(button).not.toHaveTextContent("pressed");

fireEvent.keyDown(button, {keyCode: keyCodes.space});
fireEvent.keyDown(button, {key: keys.space});
fireEvent.blur(button);
expect(button).not.toHaveTextContent("pressed");

Expand All @@ -501,9 +496,9 @@ describe("ClickableBehavior", () => {

const anchor = await screen.findByRole("link");
expect(anchor).not.toHaveTextContent("pressed");
fireEvent.keyDown(anchor, {keyCode: keyCodes.enter});
fireEvent.keyDown(anchor, {key: keys.enter});
expect(anchor).not.toHaveTextContent("pressed");
fireEvent.keyUp(anchor, {keyCode: keyCodes.enter});
fireEvent.keyUp(anchor, {key: keys.enter});
expect(anchor).not.toHaveTextContent("pressed");
});

Expand All @@ -525,16 +520,16 @@ describe("ClickableBehavior", () => {
await userEvent.click(button);
expect(onClick).toHaveBeenCalledTimes(1);

fireEvent.keyDown(button, {keyCode: keyCodes.space});
fireEvent.keyUp(button, {keyCode: keyCodes.space});
fireEvent.keyDown(button, {key: keys.space});
fireEvent.keyUp(button, {key: keys.space});
expect(onClick).toHaveBeenCalledTimes(2);

fireEvent.keyDown(button, {keyCode: keyCodes.enter});
fireEvent.keyUp(button, {keyCode: keyCodes.enter});
fireEvent.keyDown(button, {key: keys.enter});
fireEvent.keyUp(button, {key: keys.enter});
expect(onClick).toHaveBeenCalledTimes(3);

fireEvent.touchStart(button, {keyCode: keyCodes.space});
fireEvent.touchEnd(button, {keyCode: keyCodes.space});
fireEvent.touchStart(button, {key: keys.space});
fireEvent.touchEnd(button, {key: keys.space});
fireEvent.click(button);
expect(onClick).toHaveBeenCalledTimes(4);
});
Expand Down Expand Up @@ -600,21 +595,21 @@ describe("ClickableBehavior", () => {
const link = await screen.findByRole("link");

// Space press should not trigger the onClick
fireEvent.keyDown(link, {keyCode: keyCodes.space});
fireEvent.keyUp(link, {keyCode: keyCodes.space});
fireEvent.keyDown(link, {key: keys.space});
fireEvent.keyUp(link, {key: keys.space});
expect(onClick).toHaveBeenCalledTimes(0);

// Navigation didn't happen with space
expect(window.location.assign).toHaveBeenCalledTimes(0);

// Enter press should trigger the onClick after keyup
fireEvent.keyDown(link, {keyCode: keyCodes.enter});
fireEvent.keyDown(link, {key: keys.enter});
expect(onClick).toHaveBeenCalledTimes(0);

// Navigation doesn't happen until after enter is released
expect(window.location.assign).toHaveBeenCalledTimes(0);

fireEvent.keyUp(link, {keyCode: keyCodes.enter});
fireEvent.keyUp(link, {key: keys.enter});
expect(onClick).toHaveBeenCalledTimes(1);

// Navigation happened after enter click
Expand Down Expand Up @@ -730,14 +725,14 @@ describe("ClickableBehavior", () => {

// Enter press should not do anything
const checkbox = await screen.findByRole("checkbox");
fireEvent.keyDown(checkbox, {keyCode: keyCodes.enter});
fireEvent.keyDown(checkbox, {key: keys.enter});
expect(onClick).toHaveBeenCalledTimes(0);
fireEvent.keyUp(checkbox, {keyCode: keyCodes.enter});
fireEvent.keyUp(checkbox, {key: keys.enter});
expect(onClick).toHaveBeenCalledTimes(0);

// Space press should trigger the onClick
fireEvent.keyDown(checkbox, {keyCode: keyCodes.space});
fireEvent.keyUp(checkbox, {keyCode: keyCodes.space});
fireEvent.keyDown(checkbox, {key: keys.space});
fireEvent.keyUp(checkbox, {key: keys.space});
expect(onClick).toHaveBeenCalledTimes(1);
});

Expand All @@ -757,15 +752,15 @@ describe("ClickableBehavior", () => {

// Enter press
const button = await screen.findByRole("button");
fireEvent.keyDown(button, {keyCode: keyCodes.enter});
fireEvent.keyDown(button, {key: keys.enter});
expect(onClick).toHaveBeenCalledTimes(0);
fireEvent.keyUp(button, {keyCode: keyCodes.enter});
fireEvent.keyUp(button, {key: keys.enter});
expect(onClick).toHaveBeenCalledTimes(1);

// Space press
fireEvent.keyDown(button, {keyCode: keyCodes.space});
fireEvent.keyDown(button, {key: keys.space});
expect(onClick).toHaveBeenCalledTimes(1);
fireEvent.keyUp(button, {keyCode: keyCodes.space});
fireEvent.keyUp(button, {key: keys.space});
expect(onClick).toHaveBeenCalledTimes(2);
});

Expand Down Expand Up @@ -794,10 +789,10 @@ describe("ClickableBehavior", () => {
}

// Enter press on a div
fireEvent.keyDown(clickableDiv, {keyCode: keyCodes.enter});
fireEvent.keyDown(clickableDiv, {key: keys.enter});
expect(onClick).toHaveBeenCalledTimes(expectedNumberTimesCalled);
fireEvent.keyUp(clickableDiv, {
keyCode: keyCodes.enter,
key: keys.enter,
});
expectedNumberTimesCalled += 1;
expect(onClick).toHaveBeenCalledTimes(expectedNumberTimesCalled);
Expand All @@ -808,9 +803,9 @@ describe("ClickableBehavior", () => {
expect(onClick).toHaveBeenCalledTimes(expectedNumberTimesCalled);

// Space press on a div
fireEvent.keyDown(clickableDiv, {keyCode: keyCodes.space});
fireEvent.keyDown(clickableDiv, {key: keys.space});
expect(onClick).toHaveBeenCalledTimes(expectedNumberTimesCalled);
fireEvent.keyUp(clickableDiv, {keyCode: keyCodes.space});
fireEvent.keyUp(clickableDiv, {key: keys.space});
expectedNumberTimesCalled += 1;
expect(onClick).toHaveBeenCalledTimes(expectedNumberTimesCalled);

Expand Down Expand Up @@ -885,10 +880,10 @@ describe("ClickableBehavior", () => {

const checkbox = await screen.findByRole("checkbox");
// Enter press should not do anything
fireEvent.keyDown(checkbox, {keyCode: keyCodes.enter});
fireEvent.keyDown(checkbox, {key: keys.enter});
// This element still wants to have a click on enter press
fireEvent.click(checkbox);
fireEvent.keyUp(checkbox, {keyCode: keyCodes.enter});
fireEvent.keyUp(checkbox, {key: keys.enter});
expect(onClick).toHaveBeenCalledTimes(0);
});

Expand Down
24 changes: 10 additions & 14 deletions packages/wonder-blocks-clickable/src/components/clickable-behavior.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as React from "react";
import {keys} from "@khanacademy/wonder-blocks-core";

// NOTE: Potentially add to this as more cases come up.
export type ClickableRole =
Expand Down Expand Up @@ -229,11 +230,6 @@ const disabledHandlers = {
onKeyUp: () => void 0,
} as const;

const keyCodes = {
enter: 13,
space: 32,
} as const;

const startState: ClickableState = {
hovered: false,
focused: false,
Expand Down Expand Up @@ -560,21 +556,21 @@ export default class ClickableBehavior extends React.Component<
if (onKeyDown) {
onKeyDown(e);
}

const keyCode = e.which || e.keyCode;
// Allow tests to use mixed case commands ("Space" or "space")
Copy link
Member

Choose a reason for hiding this comment

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

It seems interesting to modify the component logic to accommodate the casing used in tests!

It looks like user-event will also use KeyboardEvent.key (or KeyboardEvent.code). I wonder if it's enough to solve the issue in tests by using the keys constant as well so we're testing with the proper keys that the browser uses (which you've updated already!). Let me know what you think! I'm still learning more about user-event so let me know if I am missing something!

const keyCode = e.key.toLowerCase();
Comment on lines +559 to +560
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable should probably be called keyName or just key to avoid confusing with e.keyCode which is a number.

const {triggerOnEnter, triggerOnSpace} =
getAppropriateTriggersForRole(role);
if (
(triggerOnEnter && keyCode === keyCodes.enter) ||
(triggerOnSpace && keyCode === keyCodes.space)
(triggerOnEnter && keyCode === keys.enter.toLowerCase()) ||
(triggerOnSpace && keyCode === keys.space.toLowerCase())
Comment on lines +564 to +565
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're normalizing the key name that we're getting from the event to be lowercase, why not have the constants in keys be lowercase to begin with?

) {
// This prevents space from scrolling down. It also prevents the
// space and enter keys from triggering click events. We manually
// call the supplied onClick and handle potential navigation in
// handleKeyUp instead.
e.preventDefault();
this.setState({pressed: true});
} else if (!triggerOnEnter && keyCode === keyCodes.enter) {
} else if (!triggerOnEnter && keyCode === keys.enter.toLowerCase()) {
// If the component isn't supposed to trigger on enter, we have to
// keep track of the enter keydown to negate the onClick callback
this.enterClick = true;
Expand All @@ -587,17 +583,17 @@ export default class ClickableBehavior extends React.Component<
onKeyUp(e);
}

const keyCode = e.which || e.keyCode;
const keyCode = e.key.toLowerCase();
const {triggerOnEnter, triggerOnSpace} =
getAppropriateTriggersForRole(role);
if (
(triggerOnEnter && keyCode === keyCodes.enter) ||
(triggerOnSpace && keyCode === keyCodes.space)
(triggerOnEnter && keyCode === keys.enter.toLowerCase()) ||
(triggerOnSpace && keyCode === keys.space.toLowerCase())
) {
this.setState({pressed: false, focused: true});

this.runCallbackAndMaybeNavigate(e);
} else if (!triggerOnEnter && keyCode === keyCodes.enter) {
} else if (!triggerOnEnter && keyCode === keys.enter) {
this.enterClick = false;
}
};
Expand Down
1 change: 1 addition & 0 deletions packages/wonder-blocks-core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ export {RenderStateRoot} from "./components/render-state-root";
export {RenderState} from "./components/render-state-context";
export type {AriaRole, AriaAttributes} from "./util/aria-types";
export type {AriaProps, StyleType, PropsFor} from "./util/types";
export {keys} from "./util/keys";
8 changes: 8 additions & 0 deletions packages/wonder-blocks-core/src/util/keys.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export const keys = {
enter: "Enter",
escape: "Escape",
tab: "Tab",
space: "Space",
up: "ArrowUp",
down: "ArrowDown",
} as const;
5 changes: 3 additions & 2 deletions packages/wonder-blocks-dropdown/src/components/__tests__/select-opener.test.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 {fireEvent, render, screen} from "@testing-library/react";
import {userEvent} from "@testing-library/user-event";
import {keys} from "@khanacademy/wonder-blocks-core";

import SelectOpener from "../select-opener";

Expand Down Expand Up @@ -57,11 +58,11 @@ describe("SelectOpener", () => {
// the default behavior of the button.
// eslint-disable-next-line testing-library/prefer-user-event
fireEvent.keyDown(button, {
key: " ",
key: keys.space,
});
// eslint-disable-next-line testing-library/prefer-user-event
fireEvent.keyUp(button, {
key: " ",
key: keys.space,
});

// Assert
Expand Down
Loading
Loading