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 5 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": minor
"@khanacademy/wonder-blocks-dropdown": minor
"@khanacademy/wonder-blocks-core": minor
Copy link
Member

Choose a reason for hiding this comment

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

Related to our thread about versioning in the other PR, would this also be a breaking change if consumer projects were relying on the event.which/event.keyCode functionality?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great question. I think yes, and this could be released along with the other breaking changes. So it's good timing!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, now I'm rethinking major on this and wondering if minor is more appropriate. The move to event.key is internal, and not part of the public API. Curious to hear your thoughts @beaesguerra!

Copy link
Member

Choose a reason for hiding this comment

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

Looks like I marked the similar event.key change for the SelectOpener as a patch change 😅 I think it is fine that we don't mark it as major since it is internal. It would be good to double check these areas in current usage though just to be sure there wasn't functionality relying on that!

And since this would be released with the other combobox changes, it'll be part of the new major version as you mentioned!

---

Fixes keyboard tests in Dropdown and Clickable with specific key events. We now check event.key instead of event.which or event.keyCode to match the keys returned from Testing Library/userEvent.
26 changes: 26 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,29 @@ export const UsingAriaAttributes = {
render: SingleSelectAccessibility.bind({}),
name: "Using aria attributes",
};

// This story exists for debugging automated unit tests.
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",
parameters: {
chromatic: {disableSnapshot: true},
},
};
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
23 changes: 9 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,20 @@ export default class ClickableBehavior extends React.Component<
if (onKeyDown) {
onKeyDown(e);
}

const keyCode = e.which || e.keyCode;
const keyName = e.key;
const {triggerOnEnter, triggerOnSpace} =
getAppropriateTriggersForRole(role);
if (
(triggerOnEnter && keyCode === keyCodes.enter) ||
(triggerOnSpace && keyCode === keyCodes.space)
(triggerOnEnter && keyName === keys.enter) ||
(triggerOnSpace && keyName === keys.space)
) {
// 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 && keyName === keys.enter) {
// 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 +582,17 @@ export default class ClickableBehavior extends React.Component<
onKeyUp(e);
}

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

this.runCallbackAndMaybeNavigate(e);
} else if (!triggerOnEnter && keyCode === keyCodes.enter) {
} else if (!triggerOnEnter && keyName === 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";
12 changes: 12 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,12 @@
/**
* Key value mapping reference:
* https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values
*/
export const keys = {
enter: "Enter",
escape: "Escape",
tab: "Tab",
space: "Space",
up: "ArrowUp",
down: "ArrowDown",
} as const;
Loading
Loading