FEI-5533: Re-enable select keyboard tests for Dropdown and Clickable #2420
Conversation
userEvent@14 changed how keyboard events are handled, so they were failing to match things like `" "` or `32` for space key codes. `keyboard('{space}')` passes back a case-sensitive event name that must be matched specifically, like "Space" or "space". To make this work in ClickableBehavior, I centralized the keys into wonder-blocks-core so it could be reused in Dropdowns and elsewhere.
I also included a story with one of the unit test fixtures so I could compare headless testing to the browser.
🦋 Changeset detectedLatest commit: 631850f The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (63b2d0f) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2420"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2420 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-qfjwerghlt.chromatic.com/ Chromatic results:
|
|
Size Change: +96 B (+0.1%) Total Size: 96.5 kB
ℹ️ View Unchanged
|
| (triggerOnEnter && keyCode === keys.enter.toLowerCase()) || | ||
| (triggerOnSpace && keyCode === keys.space.toLowerCase()) |
There was a problem hiding this comment.
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?
| // Allow tests to use mixed case commands ("Space" or "space") | ||
| const keyCode = e.key.toLowerCase(); |
There was a problem hiding this comment.
This variable should probably be called keyName or just key to avoid confusing with e.keyCode which is a number.
| const keyCode = e.key.toLowerCase(); | ||
| // Prevent default behavior for Enter key. Without this, the select | ||
| // is only open while the Enter key is pressed. | ||
| // Prevent default behavior for Space key. Without this, Safari stays in | ||
| // active state visually | ||
| if (keyCode === "Enter" || keyCode === " ") { | ||
| if ( | ||
| keyCode === keys.enter.toLowerCase() || | ||
| keyCode === keys.space.toLowerCase() | ||
| ) { | ||
| this.setState({pressed: true}); | ||
| e.preventDefault(); | ||
| } | ||
| }; | ||
|
|
||
| handleKeyUp: (e: React.KeyboardEvent) => void = (e) => { | ||
| const keyCode = e.key; | ||
| const keyCode = e.key.toLowerCase(); | ||
| // On key up for Enter and Space, trigger the click handler | ||
| if (keyCode === "Enter" || keyCode === " ") { | ||
| if ( | ||
| keyCode === keys.enter.toLowerCase() || | ||
| keyCode.toLowerCase() === keys.space.toLowerCase() | ||
| ) { |
There was a problem hiding this comment.
The variable keyCode should probably be renamed to key or keyName to avoid confusion with event.keyCode which is a number.
| export const keys = { | ||
| escape: "Escape", | ||
| tab: "Tab", | ||
| space: " ", | ||
| space: "Space", | ||
| up: "ArrowUp", | ||
| down: "ArrowDown", | ||
| } as const; |
There was a problem hiding this comment.
Can uses of this be replaced with the keys object exported by @khanacademy/wonder-blocks-core?
| "@khanacademy/wonder-blocks-core": patch | ||
| --- | ||
|
|
||
| Fixes keyboard tests in Dropdown and Clickable |
There was a problem hiding this comment.
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!
| ); | ||
| }; | ||
|
|
||
| export const UsingKeyboardSelection = { |
There was a problem hiding this comment.
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!
| @@ -8,7 +8,7 @@ import {ComboboxLabels} from "./types"; | |||
| export const keys = { | |||
| escape: "Escape", | |||
| tab: "Tab", | |||
| space: " ", | |||
| space: "Space", | |||
There was a problem hiding this comment.
I was running into similar-ish issues recently with key events and tests in PR: #2376 (comment) !
I ended up updating checks in DropdownCore for event.keyCode/event.which to event.key as well, since event.which or event.keyCode are deprecated. Testing keyboard interactions in tests weren't triggering the correct logic since we were using deprecated fields and user-event removed support for keyCode. I'm not sure if this is the same issue with all the other tests that were disabled though!
I also updated these constants based on the keyboard event key linked in the jsdocs: https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values. The expected key for space is documented to be " " so this may cause issues!
| @@ -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") | |||
There was a problem hiding this comment.
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!
| // Act | ||
| await userEvent.keyboard("{enter}"); | ||
|
|
||
| // Assert | ||
| // // Assert |
There was a problem hiding this comment.
nit: extra //!
| // // Assert | |
| // Assert |
| const firstItem = await screen.findByRole("option", { | ||
| name: /item 1/, | ||
| }); | ||
| expect(firstItem).toHaveFocus(); |
There was a problem hiding this comment.
In general, it is suggested to have only one expectation in a unit test: https://khanacademy.atlassian.net/wiki/spaces/ENG/pages/98402353/Unit+Testing+Best+Practices#One-Expectation-Per-Test-Case
I am getting used this too 😅 Could we move this to another test that checks that the first option has focus?
In working on WB-1799 for SingleSelect, I found a bunch of disabled tests that I started fixing to ensure keyboard interactions work. Fixing these tests required changes to Clickable, which is used in WB Dropdowns.
Jira issue: https://khanacademy.atlassian.net/browse/FEI-5533
PR highlights:
userEvent@14keysobject inwonder-blocks-corefor use inwonder-blocks-clickable,wonder-blocks-dropdown, and any other modules that need it.Spaceorspace. I lower-cased the key names to make sure they match in the code. We could use linting to enforce this instead... I'm open to suggestions.Test Plan
Run the tests using
yarn test.There is one pre-existing lint failure for me locally in
expect-render-error.d.ts, but it seems unrelated to these changes.