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 9 commits into
base: feature/dropdown-combobox
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 2 commits
Commits
Show all changes
9 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
b4bc2b7
[FEI-5533-part1] Update changelog with additional details
marcysutton Jan 11, 2025
674d872
[FEI-5533-part1] Disable keyboard story in Chromatic
marcysutton Jan 11, 2025
b3bc70a
[FEI-5533-part1] PR feedback:
marcysutton Jan 11, 2025
226b4e7
[FEI-5533-part1] Re-enables passing tests or adds notes to fails
marcysutton Jan 13, 2025
3eea444
[FEI-5533-part1] Update changeset to indicate major breaking change
marcysutton Jan 13, 2025
aa8c34f
[FEI-5533-part1] Use " " over "{Space}" to match browser behavior
marcysutton Jan 17, 2025
2ada9bd
[FEI-5533-part1] Use Capital case Escape to match key constants
marcysutton Jan 17, 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
marcysutton marked this conversation as resolved.
Show resolved Hide resolved
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 = {
marcysutton marked this conversation as resolved.
Show resolved Hide resolved
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")
marcysutton marked this conversation as resolved.
Show resolved Hide resolved
const keyCode = e.key.toLowerCase();
marcysutton marked this conversation as resolved.
Show resolved Hide resolved
const {triggerOnEnter, triggerOnSpace} =
getAppropriateTriggersForRole(role);
if (
(triggerOnEnter && keyCode === keyCodes.enter) ||
(triggerOnSpace && keyCode === keyCodes.space)
(triggerOnEnter && keyCode === keys.enter.toLowerCase()) ||
(triggerOnSpace && keyCode === keys.space.toLowerCase())
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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. It would have to be normalized somewhere in the event of mixed casing. The tests already had lowercase key names in them, but our constants are in capital case...so it makes sense why they would fail to match. I could change the constants to all be lowercase, but that will require a lottttt of testing for every key event (Enter, Escape, ArrowDown, ArrowUp, etc.). I'm actually thinking I could just update these tests to use capital case and go with that. It might limit the scope of this a bit better!

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: I did make some updates including re-enabling more tests! But I limited it to wonder-blocks-dropdown and wonder-blocks-clickable. There are still more in other packages that could be updated later!

) {
// 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