fix(a11y): announce event durations correctly for screen readers#29753
fix(a11y): announce event durations correctly for screen readers#29753Alisherkhan032 wants to merge 1 commit into
Conversation
Add aria-label with full duration text (e.g. "30 minutes") while keeping abbreviated visible labels (30m, 1h). Introduces formatEventDuration helpers and DurationText; event cards use minutes-only display, booking pages keep hour-aware formatting.
|
Welcome to Cal.diy, @Alisherkhan032! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
📝 WalkthroughWalkthroughAdds shared localized duration formatting helpers and an accessible 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/web/components/apps/installation/EventTypesStepCard.tsx (1)
83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider null-guarding
DurationTextrendering instead of?? ""fallback.The
renderDurationhelper inDuration.tsxchecksif (!formatted || !label) return nullbefore renderingDurationText. Here, the?? ""fallback would render<span aria-label="">with no children ifdurationis 0 or undefined — an empty, inaccessible element inside a Badge. Consider guarding against null before rendering, consistent with therenderDurationpattern.♻️ Proposed null guard
{Boolean(durations.length) && durations.map((duration) => { + const formatted = getDurationMinutesFormatted(duration, t); + const label = getDurationMinutesAccessibleLabel(duration, t); + if (!formatted || !label) return null; return ( <Badge key={`event-type-${id}-duration-${duration}`} variant="gray" startIcon="clock"> - <DurationText label={getDurationMinutesAccessibleLabel(duration, t) ?? ""}> - {getDurationMinutesFormatted(duration, t)} - </DurationText> + <DurationText label={label}>{formatted}</DurationText> </Badge> ); })}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/components/apps/installation/EventTypesStepCard.tsx` around lines 83 - 85, Replace the empty-string fallback in the DurationText rendering within EventTypesStepCard with a null guard consistent with renderDuration in Duration.tsx: only render DurationText when getDurationMinutesAccessibleLabel(duration, t) returns a value, otherwise render nothing, avoiding an empty aria-label element.packages/lib/formatEventDuration.test.ts (1)
76-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd null/undefined edge case tests for minutes-only formatters.
getDurationMinutesFormattedandgetDurationMinutesAccessibleLabellack tests forundefinedand0inputs, unlikegetDurationFormattedandgetDurationAccessibleLabelwhich have them. Adding these ensures the null-return contract is consistent across all four functions.♻️ Proposed additional tests
describe("getDurationMinutesFormatted", () => { it("keeps durations as total minutes", () => { expect(getDurationMinutesFormatted(90, mockT)).toBe("90m"); expect(getDurationMinutesFormatted(60, mockT)).toBe("60m"); }); + + it("returns null for empty values", () => { + expect(getDurationMinutesFormatted(undefined, mockT)).toBeNull(); + expect(getDurationMinutesFormatted(0, mockT)).toBeNull(); + }); }); describe("getDurationMinutesAccessibleLabel", () => { it("announces total minutes for long durations", () => { expect(getDurationMinutesAccessibleLabel(90, mockT)).toBe("90 minutes"); expect(getDurationMinutesAccessibleLabel(60, mockT)).toBe("60 minutes"); }); + + it("returns null for empty values", () => { + expect(getDurationMinutesAccessibleLabel(undefined, mockT)).toBeNull(); + expect(getDurationMinutesAccessibleLabel(0, mockT)).toBeNull(); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/formatEventDuration.test.ts` around lines 76 - 88, Add tests in the getDurationMinutesFormatted and getDurationMinutesAccessibleLabel describe blocks covering undefined and 0 inputs, matching the null-return expectations already tested for getDurationFormatted and getDurationAccessibleLabel. Use mockT and assert each minutes-only formatter returns the established null value for both edge cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/modules/bookings/components/event-meta/Duration.tsx`:
- Around line 106-113: Remove the redundant aria-label from the duration list
item in the component rendering the element with onClick and renderDuration.
Keep DurationText’s existing aria-label as the sole accessible label, and remove
the associated getDurationAccessibleLabel usage if it becomes unused.
In `@packages/platform/atoms/event-types/__tests__/EventTypeListItem.test.tsx`:
- Around line 20-27: Update the mock `t` function’s `options` parameter type to
include an optional `unit?: string` property alongside `count?: number`,
matching the usage in the `multiple_duration_timeUnit_short` case and the
companion test.
---
Nitpick comments:
In `@apps/web/components/apps/installation/EventTypesStepCard.tsx`:
- Around line 83-85: Replace the empty-string fallback in the DurationText
rendering within EventTypesStepCard with a null guard consistent with
renderDuration in Duration.tsx: only render DurationText when
getDurationMinutesAccessibleLabel(duration, t) returns a value, otherwise render
nothing, avoiding an empty aria-label element.
In `@packages/lib/formatEventDuration.test.ts`:
- Around line 76-88: Add tests in the getDurationMinutesFormatted and
getDurationMinutesAccessibleLabel describe blocks covering undefined and 0
inputs, matching the null-return expectations already tested for
getDurationFormatted and getDurationAccessibleLabel. Use mockT and assert each
minutes-only formatter returns the established null value for both edge cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f4dcd7c-118e-4d29-ac9b-b7776972cbf0
📒 Files selected for processing (11)
apps/web/components/apps/installation/EventTypesStepCard.tsxapps/web/modules/bookings/components/BookEventForm/BookFormAsModal.tsxapps/web/modules/bookings/components/event-meta/Duration.tsxapps/web/modules/event-types/components/EventTypeDescription.tsxpackages/lib/formatEventDuration.test.tspackages/lib/formatEventDuration.tspackages/platform/atoms/event-types/__tests__/EventTypeListItem.test.tsxpackages/platform/atoms/event-types/components/EventTypeListItem.tsxpackages/ui/components/duration/DurationText.tsxpackages/ui/components/duration/index.tspackages/ui/package.json
| aria-label={getDurationAccessibleLabel(duration, t) ?? undefined} | ||
| onClick={() => setSelectedDuration(duration)} | ||
| ref={(el) => (itemRefs.current[duration] = el)} | ||
| className={classNames( | ||
| selectedDuration === duration ? "bg-emphasis" : "hover:text-emphasis", | ||
| "text-default cursor-pointer rounded-[4px] px-3 py-1.5 text-sm leading-tight transition" | ||
| )}> | ||
| <div className="w-max">{getDurationFormatted(duration, t)}</div> | ||
| <div className="w-max">{renderDuration(duration, t)}</div> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Potential double screen-reader announcement due to nested aria-labels.
The <li> at line 106 sets aria-label={getDurationAccessibleLabel(duration, t) ?? undefined}, while the inner renderDuration at line 113 returns a <DurationText> (i.e., <span aria-label={label}>) with the same label value. During list navigation, screen readers use the <li>'s accessible name — but if a user navigates into the item's content, the inner <span>'s aria-label may cause a redundant "30 minutes" announcement.
Since DurationText already provides the accessible label, the <li>'s aria-label is redundant. Removing it lets the accessible name be computed from the inner span's aria-label.
♿ Proposed fix: remove redundant aria-label from ``
<li
data-testId={`multiple-choice-${duration}mins`}
data-active={selectedDuration === duration ? "true" : "false"}
key={index}
- aria-label={getDurationAccessibleLabel(duration, t) ?? undefined}
onClick={() => setSelectedDuration(duration)}
ref={(el) => (itemRefs.current[duration] = el)}
className={classNames(
selectedDuration === duration ? "bg-emphasis" : "hover:text-emphasis",
"text-default cursor-pointer rounded-[4px] px-3 py-1.5 text-sm leading-tight transition"
)}>
<div className="w-max">{renderDuration(duration, t)}</div>
</li>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| aria-label={getDurationAccessibleLabel(duration, t) ?? undefined} | |
| onClick={() => setSelectedDuration(duration)} | |
| ref={(el) => (itemRefs.current[duration] = el)} | |
| className={classNames( | |
| selectedDuration === duration ? "bg-emphasis" : "hover:text-emphasis", | |
| "text-default cursor-pointer rounded-[4px] px-3 py-1.5 text-sm leading-tight transition" | |
| )}> | |
| <div className="w-max">{getDurationFormatted(duration, t)}</div> | |
| <div className="w-max">{renderDuration(duration, t)}</div> | |
| <li | |
| data-testId={`multiple-choice-${duration}mins`} | |
| data-active={selectedDuration === duration ? "true" : "false"} | |
| key={index} | |
| onClick={() => setSelectedDuration(duration)} | |
| ref={(el) => (itemRefs.current[duration] = el)} | |
| className={classNames( | |
| selectedDuration === duration ? "bg-emphasis" : "hover:text-emphasis", | |
| "text-default cursor-pointer rounded-[4px] px-3 py-1.5 text-sm leading-tight transition" | |
| )}> | |
| <div className="w-max">{renderDuration(duration, t)}</div> | |
| </li> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/modules/bookings/components/event-meta/Duration.tsx` around lines
106 - 113, Remove the redundant aria-label from the duration list item in the
component rendering the element with onClick and renderDuration. Keep
DurationText’s existing aria-label as the sole accessible label, and remove the
associated getDurationAccessibleLabel usage if it becomes unused.
| t: (key: string, options?: { count?: number }) => { | ||
| const count = options?.count ?? 0; | ||
|
|
||
| switch (key) { | ||
| case "minute_one_short": | ||
| return `${count}m`; | ||
| case "multiple_duration_timeUnit_short": | ||
| return `${count}${options?.unit === "hour" ? "h" : "m"}`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix options type annotation to include unit.
The mock's t function types options as { count?: number } but line 27 accesses options?.unit. This is a TypeScript error — unit doesn't exist on the type. The companion test in formatEventDuration.test.ts correctly types it as { count?: number; unit?: string }.
🐛 Proposed fix
- t: (key: string, options?: { count?: number }) => {
+ t: (key: string, options?: { count?: number; unit?: string }) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t: (key: string, options?: { count?: number }) => { | |
| const count = options?.count ?? 0; | |
| switch (key) { | |
| case "minute_one_short": | |
| return `${count}m`; | |
| case "multiple_duration_timeUnit_short": | |
| return `${count}${options?.unit === "hour" ? "h" : "m"}`; | |
| t: (key: string, options?: { count?: number; unit?: string }) => { | |
| const count = options?.count ?? 0; | |
| switch (key) { | |
| case "minute_one_short": | |
| return `${count}m`; | |
| case "multiple_duration_timeUnit_short": | |
| return `${count}${options?.unit === "hour" ? "h" : "m"}`; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/platform/atoms/event-types/__tests__/EventTypeListItem.test.tsx`
around lines 20 - 27, Update the mock `t` function’s `options` parameter type to
include an optional `unit?: string` property alongside `count?: number`,
matching the usage in the `multiple_duration_timeUnit_short` case and the
companion test.
1 participant
Add aria-label with full duration text (e.g. "30 minutes") while keeping abbreviated visible labels (30m, 1h). Introduces formatEventDuration helpers and DurationText; event cards use minutes-only display, booking pages keep hour-aware formatting.
What does this PR do?
Fixes VoiceOver (and similar screen readers) misreading duration abbreviations on event type cards and booking pages — e.g. 30m announced as "30 metres" and 1h as "one age".
Approach (Option 1 from the issue): keep short visible text, add explicit aria-label with expanded duration via i18n (minute_one, minute_other, hour_one, hour_other).
Changes:
packages/lib/formatEventDuration.ts — short formatters + accessible label helpers
packages/ui/components/duration/DurationText.tsx — {children}
Event cards / installation cards / platform lists — minutes-only display (90m) + label (90 minutes)
Booking page (Duration.tsx, BookFormAsModal) — hour-aware display (1h 30m) + matching label (1 hour 30 minutes)
Fixes Duration abbreviations pronounced incorrectly by screen readers ("30m" as "30 metres", "1h" as "one age") #29750
Visual Demo (For contributors especially)
N/A — no visible UI change on event cards (still 30m, 90m, etc.). Fix is for screen reader announcements.
Video Demo (if applicable):
N/A
Image Demo (if applicable):
N/A
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Automated
yarn vitest run packages/lib/formatEventDuration.test.ts yarn vitest run packages/platform/atoms/event-types/__tests__/EventTypeListItem.test.tsx yarn biome check packages/lib/formatEventDuration.ts packages/ui/components/duration/Manual (VoiceOver)
Environment: Standard Cal.diy dev setup; no new env vars.
Happy path: Event with length: 30 → visible 30m, VoiceOver says "30 minutes"