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: 2ada9bd 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 (84fd008) 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-agpotuhfru.chromatic.com/ Chromatic results:
|
|
Size Change: +5 B (+0.01%) Total Size: 98.3 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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
packages/wonder-blocks-clickable/src/components/clickable-behavior.ts
Outdated
Show resolved
Hide resolved
packages/wonder-blocks-dropdown/src/components/select-opener.tsx
Outdated
Show resolved
Hide resolved
| @@ -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!
There was a problem hiding this comment.
It was weird that " " wasn't matching for a space any longer. I only changed it because userEvent passed back the key name, in mixed casing! I considered doing an OR statement so that either one would match. One of them would have to be named something else, though...probably the literal space " " since the rest are represented by key names (Escape, Space, etc.)?
There was a problem hiding this comment.
Here's the event object under test when passing userEvent.keyboard('{space}') as was written in these tests, with some irrelevant parts stripped out for brevity:
SyntheticBaseEvent {
_reactName: 'onKeyDown',
type: 'keydown',
target: <ref *1> HTMLButtonElement {
'__reactFiber$dkck3rhjopo': FiberNode {
tag: 5,
key: null,
elementType: 'button',
type: 'button',
},
'__reactProps$dkck3rhjopo': {
'aria-label': undefined,
'aria-selected': 'false',
'aria-checked': undefined,
role: 'option',
'aria-current': undefined,
children: [Object],
'data-testid': undefined,
onClick: [Function: handleClick],
onMouseEnter: [Function: handleMouseEnter],
onMouseLeave: [Function: handleMouseLeave],
onMouseDown: [Function: handleMouseDown],
onMouseUp: [Function: handleMouseUp],
onTouchStart: [Function: handleTouchStart],
onTouchEnd: [Function: handleTouchEnd],
onTouchCancel: [Function: handleTouchCancel],
onKeyDown: [Function: handleKeyDown],
onKeyUp: [Function: handleKeyUp],
onFocus: [Function: handleFocus],
onBlur: [Function: handleBlur],
tabIndex: undefined,
rel: undefined,
type: 'button',
'aria-disabled': false,
className: '',
style: [Object]
},
currentTarget: <ref *1> HTMLButtonElement {
'__reactFiber$dkck3rhjopo': FiberNode {
tag: 5,
key: null,
elementType: 'button',
type: 'button',
},
eventPhase: 3,
bubbles: true,
cancelable: true,
timeStamp: 1736555344807,
defaultPrevented: false,
isTrusted: false,
view: null,
detail: 0,
key: 'space', // <----- this one!
code: 'Unknown',
location: 0,
ctrlKey: false,
shiftKey: false,
altKey: false,
metaKey: false,
repeat: false,
locale: undefined,
getModifierState: [Function: modifierStateGetter],
charCode: 0,
keyCode: 0,
which: 0,
isDefaultPrevented: [Function: functionThatReturnsFalse],
isPropagationStopped: [Function: functionThatReturnsFalse]
}
I suppose the test would have to pass a literal space character to match on that? I was trying to get the existing tests to pass, and they had lowercase key names in them ('{space}')!
There was a problem hiding this comment.
Hmmm I wonder if we need to update how we simulate the space press key event in our tests: testing-library/user-event#972 (comment) ({space} or {Space} isn't shown as an example)
Looking up {space} in the React Testing Library docs shows documentation for user events v13: https://testing-library.com/docs/user-event/v13/#special-characters so it could be related to why the tests broke after upgrading to v14!
By updating the tests, it will be more similar to what happens in the browser! Testing things out in Storybook, I found that the space key functionality in the Clickable example is not showing the pressed styles as expected since the browser sets the key to instead of Space:
Screen.Recording.2025-01-13.at.3.27.35.PM.mov
There was a problem hiding this comment.
Great catch, Bea! This prompted me to check the tests again and we can fire the literal string space instead! I made some updates to use " " instead of "{Space}" and ClickableBehavior should work again.
packages/wonder-blocks-clickable/src/components/clickable-behavior.ts
Outdated
Show resolved
Hide resolved
packages/wonder-blocks-dropdown/src/components/__tests__/single-select.test.tsx
Outdated
Show resolved
Hide resolved
packages/wonder-blocks-dropdown/src/components/__tests__/single-select.test.tsx
Show resolved
Hide resolved
1. Use core keys over wonder-blocks-dropdown constants 2. Use keyName over keyCode for key names 3. Use one assertion per test 4. Use CapitalCase event names to simplify code
.changeset/mean-cherries-press.md
Outdated
| "@khanacademy/wonder-blocks-clickable": minor | ||
| "@khanacademy/wonder-blocks-dropdown": minor | ||
| "@khanacademy/wonder-blocks-core": minor |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Great question. I think yes, and this could be released along with the other breaking changes. So it's good timing!
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
|
I did a bit of an audit as part of creating subtasks and found a few more tests that could be enabled! There are 5 outstanding tests in WB Dropdown that I've indicated in the subtask issue: https://khanacademy.atlassian.net/browse/FEI-6114 |
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.
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.