Add accessbile today/task card#32
Conversation
There was a problem hiding this comment.
Pull request overview
Improves accessibility for task-related UI cards and expands client-side test coverage around task selection, navigation, and deletion flows.
Changes:
- Added keyboard accessibility attributes/handlers (
role="button",tabIndex,aria-label, Enter/Space handling) toTaskCardandTodayCard. - Added new component tests for
TodayCardandMobilePageLayout. - Added a
useTaskDetailLogictest to validate non-desktop delete behavior does not callonClose.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/client/components/today-card.tsx | Adds keyboard interaction + ARIA semantics to the Today card container. |
| src/client/components/task-card.tsx | Adds keyboard interaction + ARIA semantics to the Task card container. |
| src/client/tests/today-card.test.tsx | New TodayCard tests for click behavior and event propagation boundaries. |
| src/client/tests/mobile-page-layout.test.tsx | New MobilePageLayout tests for back navigation rendering and title. |
| src/client/tests/useTaskDetailLogic.test.tsx | Adds coverage for non-desktop delete flow not invoking onClose. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const handleCardKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault() | ||
| handleCardClick() | ||
| } | ||
| } |
There was a problem hiding this comment.
handleCardKeyDown will fire for key events originating from child controls (e.g., the status button / overflow button) because keydown bubbles. This can cause Enter/Space on those controls to also trigger handleCardClick (unexpected navigation/selection). Consider guarding with if (e.target !== e.currentTarget) return (or stopping propagation in child key handlers) before handling Enter/Space.
| const handleCardKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault() |
There was a problem hiding this comment.
This file uses the React.KeyboardEvent type but does not import React (only useState). If the project isn't relying on the global React namespace for types, this will fail type-checking. Prefer import type React from 'react' or use KeyboardEvent from react in the handler signature.
| <div | ||
| onClick={handleCardClick} | ||
| onKeyDown={handleCardKeyDown} | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-label={`View details for ${task.name}`} | ||
| className={`group relative flex flex-col gap-2.5 rounded-xl border px-4 py-3.5 |
There was a problem hiding this comment.
The card is always given role="button" and tabIndex={0}, but on desktop handleCardClick becomes a no-op when onSelect is undefined. That results in a focusable element announced as a button that doesn't do anything. Consider either (a) making the card always perform an action (e.g., navigate when onSelect is absent) or (b) only adding role/tabIndex/aria-label and click/key handlers when it is actually interactive (or set aria-disabled and remove from tab order).
| const handleCardKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault() | ||
| handleCardClick() | ||
| } | ||
| } |
There was a problem hiding this comment.
handleCardKeyDown is attached to the whole card, so keydown events from nested interactive elements (the name <Input>, action <Button>s, etc.) will bubble up and can trigger handleCardClick when users press Enter/Space inside those controls. Add a guard like if (e.target !== e.currentTarget) return (or stop propagation from child controls) to avoid accidental selection.
| <Card | ||
| className="mb-3 hover:shadow-md transition-all group" | ||
| onClick={handleCardClick} | ||
| onKeyDown={handleCardKeyDown} | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-label={`View details for ${task.name}`} | ||
| > |
There was a problem hiding this comment.
role="button"/tabIndex={0}/aria-label are applied even when onSelect is undefined (and handleCardClick does nothing). Since TaskCard can be rendered without onSelect (e.g., status-column.tsx passes undefined when setSelectedTask is absent), this creates a focusable control that appears clickable but has no action. Consider only adding button semantics + handlers when onSelect is provided, or provide an alternate default action / mark it disabled and remove it from tab order.
| const card = container.querySelector('.group') as Element | ||
| // Must not throw | ||
| fireEvent.click(card) | ||
| }) |
There was a problem hiding this comment.
The “no onSelect” test finds the card via container.querySelector('.group'), which is brittle (class names can change and .group may match multiple elements). Since the component now exposes role="button" with an accessible name, prefer getByRole('button', { name: ... }) (or a stable test id) to target the card.
| it('calls onSelect when the card body is clicked', async () => { | ||
| const qc = createClient() | ||
| // @ts-ignore | ||
| apiClient.put = async (_url: string, body: Partial<Task>) => makeResponse({ ...task(), ...body }) | ||
|
|
||
| let selectCalled = 0 | ||
| const onSelect = () => { | ||
| selectCalled++ | ||
| } | ||
|
|
||
| render( | ||
| <Wrapper client={qc}> | ||
| <TodayCard task={task()} onSelect={onSelect} /> | ||
| </Wrapper>, | ||
| ) | ||
|
|
||
| await new Promise((r) => setTimeout(r, 10)) | ||
|
|
||
| const card = screen.getByRole('button', { name: /view details for test task/i }) | ||
| fireEvent.click(card) | ||
|
|
||
| await waitFor(() => { | ||
| expect(selectCalled).toBe(1) | ||
| }) |
There was a problem hiding this comment.
Keyboard accessibility was added to TodayCard (Enter/Space), but the test suite only covers mouse clicks. Add coverage that focuses the card and asserts Enter/Space trigger selection/navigation, and also verifies that Enter/Space on nested controls (status/overflow buttons) does not trigger the card action (to prevent regressions with bubbling key events).
…e proper rendering
This pull request focuses on improving accessibility and test coverage for task-related UI components. The main changes add keyboard accessibility to
TaskCardandTodayCard, and introduce new tests for these components and the mobile page layout to ensure correct behavior and event handling.Accessibility improvements:
TaskCardandTodayCardby implementingonKeyDownhandlers for Enter/Space, settingrole="button",tabIndex={0}, and appropriatearia-labels, allowing users to interact with these cards via keyboard navigation.Test coverage enhancements:
TodayCardcovering click handling, event propagation boundaries (ensuring subcomponent clicks do not trigger card selection), and safe operation withoutonSelect.MobilePageLayoutto verify back navigation (button vs. link) and title rendering.useTaskDetailLogicto ensure that on non-desktop devices, theonDeleteRequestdoes not invoke theonClosecallback, improving correctness in mobile flows.