fix(ui): make clickable Card activate from the keyboard - #952
Open
marianmoldovan wants to merge 1 commit into
Open
fix(ui): make clickable Card activate from the keyboard#952marianmoldovan wants to merge 1 commit into
marianmoldovan wants to merge 1 commit into
Conversation
Tabbing to a collection card on the home page and pressing Enter did
nothing, and pressing Space scrolled the page away from the focused card.
The card was already focusable (role="button" + tabIndex={0} when onClick
is present), but the keyboard handling was wrong in two ways:
- It was bound to onKeyPress, which is deprecated and no longer a
reliable activation hook on React 19. The handler simply never ran for
a keydown, so Enter never activated the card.
- It never called preventDefault(), so Space additionally performed its
default browser action and scrolled the page, moving the focused card
out of view.
Switch to onKeyDown and preventDefault() for both Enter and Space. The
handler only activates when the event target is the card itself, so key
events bubbling up from focusable children (the action IconButtons that
NavigationCard renders inside the card, whose wrapper stops click but not
key propagation) no longer activate the card as well. A caller supplied
onKeyDown is now composed rather than silently overwritten by the props
spread, and can opt out of activation via preventDefault().
Also add a focus-visible ring to cardClickableMixin, following the
existing convention, so keyboard users can see which card holds focus.
Refs #726
This fixes the deprecated `onKeyPress` handler, but not the dnd-kit
wrapper in `HomePageDnD` that is the likely primary cause of the
reported disappearing card. Leaving the issue open for that.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marianmoldovan
force-pushed
the
fix/726-card-keyboard-activation
branch
from
July 29, 2026 16:51
6a4a7c5 to
2980866
Compare
marianmoldovan
requested review from
Copilot and
fgatti675
and removed request for
Copilot
July 29, 2026 17:07
There was a problem hiding this comment.
Pull request overview
This PR improves keyboard accessibility for the Card component by correcting how “clickable” cards handle Enter/Space activation and by adding a keyboard focus indicator via the shared clickable-card style mixin.
Changes:
- Replace deprecated
onKeyPresshandling withonKeyDownactivation for Enter/Space, withpreventDefault()to avoid Space scrolling. - Add guarding logic to avoid activating the card when key events originate from focusable children, and honor caller-provided
onKeyDown. - Add
focus-visiblering styling tocardClickableMixinand introduce a dedicatedCardtest suite covering these behaviors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/ui/test/Card.test.tsx | Adds regression/unit tests for keyboard activation, preventDefault behavior, bubbling from focusable children, and focus-visible class presence. |
| packages/ui/src/styles.ts | Extends cardClickableMixin with focus-visible ring classes for keyboard focus indication. |
| packages/ui/src/components/Card.tsx | Updates keyboard handling to onKeyDown, prevents default activation behavior, avoids bubbling activation from children, and composes with caller onKeyDown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
33
to
38
| if (e.key === "Enter" || e.key === " ") { | ||
| onClick?.(); | ||
| // Without this, Space also performs its default browser action and | ||
| // scrolls the page away from the focused card. | ||
| e.preventDefault(); | ||
| onClick(); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #726 — but see the open question below; this may not be the whole story.
The bug
The card is focusable (
role="button",tabIndex={0}whenonClickis set), but keyboard handling was wrong twice over:onKeyPressis deprecated and does not fire for keydown — this codebase is on React 19.2.7. Against unmodifiedCard,activates on Enter keydowncallsonClick0 times.e.preventDefault()for Space, so Space kept its browser default (scroll the page).Both confirmed by test, not inference: 5 of the 15 new tests fail against the unmodified component.
The fix
onKeyPress→onKeyDown,preventDefault()on activation, plus:e.target !== e.currentTargetguard. Load-bearing:NavigationCardBindingrenders a favouriteIconButton(a real<button>) inside the card, so switching toonKeyDownwithout this guard would have introduced a new bug where Enter on the star both toggles the favourite and navigates.onKeyDowncoming through...propsis invoked rather than silently dropped, ande.defaultPreventedis respected.focus-visiblering oncardClickableMixin, following the existing convention (FilterChip,Tabs) and matching the hover ring already on that mixin. This also givesSmallNavigationCardthe focus indicator it lacked.Fixing
Cardimproves every clickable card in the product at once.While verifying, a second and likely more significant cause of the reported "the collection just disappears" was found, and deliberately left alone:
In
HomePageDnD.tsx,SortableNavigationCardspreads dnd-kit's{...attributes} {...listeners}onto a wrapper div around the card, and the same element carriesopacity: isDragging ? 0 : 1. Per the installed dnd-kit 6.3.1:attributesaddsrole="button" tabIndex={0}, anddefaultKeyboardCodes.start = [Space, Enter].So the first Tab lands on an invisible drag handle that looks identical to the card, and Space or Enter there starts a keyboard drag and hides the source card at opacity 0 — the collection literally vanishes. That matches the report precisely, including the otherwise-odd mention of TAB. It also means every card has a duplicate tab stop.
This was not changed because keyboard drag is a genuine a11y feature and rewiring it risks breaking drag-and-drop — it needs a maintainer decision. Good news for this PR: dnd-kit's activator checks
event.target !== activator, so it will not hijack the new Enter/Space handler.Recommendation: keep #726 open after merging this, to track the dnd-kit tab stop.
Tests
15 tests in
packages/ui/test/Card.test.tsx.packages/ui: 145/145 passing (was 130/130).@firecms/coreuntouched at 237/240.Not verified
jsdom has no scrolling and no real focus ring, so the mechanism was asserted (
preventDefaultviafireEvent's return value, and the class string) rather than the visual outcome. Not run in a browser.🤖 Generated with Claude Code