Repository navigation
[scheduler] Complete ARIA grid semantics on EventTimeline and EventCalendar - #22266
Conversation
Deploy previewhttps://deploy-preview-22266--material-ui-x.netlify.app/ Bundle size
Check out the code infra dashboard for more information about this PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25afc96000
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| role: 'grid', | ||
| 'aria-colcount': presetConfig.tickCount + 1, | ||
| 'aria-rowcount': resources.length + 1, |
There was a problem hiding this comment.
TimelineGridHeader now emits one role="row" per header level, but TimelineGridRoot still reports aria-rowcount as resources.length + 1. In presets with multiple header levels, assistive tech sees more rows than the declared total, so row/position announcements become inconsistent (e.g., body rows indexed from 2 while extra header rows also exist). Update the rowcount calculation (and corresponding row indexing expectations) to include all rendered header rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in c379c9c
…t and shift body row indexing
|
@rita-codes please have a review on this pr. Thanks! |
| key={cell.key} | ||
| className={[classNames?.cell, level.className].filter(Boolean).join(' ') || undefined} | ||
| role="columnheader" | ||
| aria-colindex={cell.index + 2} |
There was a problem hiding this comment.
Codex review:
aria-colindex={cell.index + 2}uses the header-cell ordinal fromiterate.ts, not the starting tick column. FordayAndHour,day headers span 24 columns, so the four day cells should start at columns2, 26, 50, 74; this code emits2, 3, 4, 5. Same problem appears for clamped month cells indayAndMonth.
There was a problem hiding this comment.
addressed in 10729f8
| { | ||
| role: 'grid', | ||
| 'aria-colcount': presetConfig.tickCount + 1, | ||
| 'aria-rowcount': presetConfig.headers.length + resources.length, |
There was a problem hiding this comment.
TimelineGridRoot, TimelineGridTitleRow, and TimelineGridEventRow were changed to assume the grid always has presetConfig.headers.length header rows (which matches one of the styled versions). This breaks the headless API as it causes TitleRow/EventRow to start at aria-rowindex 3.
There was a problem hiding this comment.
@mj12albert addressed in 10729f8. Thanks for the review and I have implemented the suggested changes. Kindly do let me know if anything else is needed.
PTAL.
…nd keep timeline grid primitive headless
github-actions
Bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
# Conflicts: # packages/x-scheduler-premium/src/event-timeline-premium/content/EventTimelinePremiumContent.tsx
github-actions
Bot
removed
the
PR: out-of-date
|
@mj12albert @rita-codes PTAL. |
rita-codes
added
the
type: enhancement
rita-codes
changed the title
EventTimeline and EventCalendar
|
Hey @Anexus5919, thanks for picking this up. I'd suggest splitting this into two parts because EventCalendar and EventTimeline have different blockers. EventCalendar (MiniCalendar, MonthView, DayTimeGrid)The grid declares
Suggested fix (no DOM/CSS change): drop col 1 from the ARIA model. Use Two small unrelated fixes
EventTimelinePremiumThe Timeline part of this PR has three issues that depend on how horizontal body virtualization will be designed:
Question for @romgrkTwo options, what do you prefer?
Let me know what fits you better, I don't want to get in the way of your virtualization work 🙇♀️ Thanks! |
|
Thanks for the thorough review @rita-codes, the split makes sense. Agreed on all EventCalendar points:
I'll also drop the For EventTimeline, happy to go either way. I'll hold those changes until @romgrk weighs in on A vs B. If B, I'll revert the timeline-specific bits from this PR and let it land alongside the virtualization design. |
| return useRenderElement('div', componentProps, { | ||
| ref: [forwardedRef], | ||
| props: [elementProps, { role: 'cell' }], | ||
| props: [elementProps, { role }], |
There was a problem hiding this comment.
useRenderElement should automatically handle the priority here, we shouldn't do elementProps.xxx ?? 'customValue' 👍
But elementProps should be defined after the custom props @rita-codes (check all the other components)
There was a problem hiding this comment.
Thanks @flaviendelangle, addressed in 9a4454f.
Dropped the ?? customValue fallback in the six primitives this PR touches and put elementProps after the custom props in each props array. CalendarGridDayCell destructures aria-labelledby out first to preserve the existing column-header join.
I left the rest of the codebase on the legacy order since the broader convention sweep is yours and @rita-codes' call. Happy to follow up in a separate PR if you'd like me to.
…of nullish-coalescing fallback
|
I'd go with B, I'm still trying out options for the virtualization layout. |
|
Thanks @romgrk! @rita-codes confirming before I proceed: should I revert the EventTimeline parts of this PR (the Holding off on the revert until you confirm. |
|
@romgrk perfect, makes sense 👍 thanks for confirming! @Anexus5919 yes, let’s go with that. Please revert everything related to We can tackle Timeline a11y separately once the virtualization design is clearer 🙌 |
@rita-codes Done in PTAL. |
| const isOtherMonth = !adapter.isSameMonth(day.value, displayedMonth); | ||
|
|
||
| // Create a full date label for accessibility | ||
| const fullDateLabel = adapter.formatByString( |
There was a problem hiding this comment.
Nit, out of scope: the // Create a full date label for accessibility comment that used to sit on the line above was removed, but it doesn't relate to the ARIA grid semantics changes in this PR. Could you restore it to keep the diff focused?
|
Thanks @rita-codes, good catches. Pushed in 17e92f6:
Happy to adjust anything else. |
|
Nice iteration on this @Anexus5919! 👏 Thanks for working on this! 🙇♀️ |
Thanks a lot, @rita-codes! 🙌 Really appreciate the thorough reviews on this one, learned a bunch about how the grid semantics fit together. Happy to pick up the Timeline a11y piece once the virtualization design settles, and I'd love to keep contributing to MUI X more broadly. Thanks again for the guidance throughout 🙏 |
|
The virtualization rework is done. |
Closes #22265.
Summary
Wires the missing ARIA grid attributes that #22265 calls out across both Scheduler surfaces, so a screen-reader user always knows "row N of M, column N of M".
The data attributes required for this work were already in place after #22222 (
data-level,data-index,data-unit-leaf,--span); this PR is the a11y follow-up the author of #22222 explicitly flagged as out of scope there. Keyboard navigation across cells stays out of scope here too, per the same issue (tracked in #21909).What was missing before this PR (per #22265)
EventTimeline
role="cell"and had noaria-colindex(TODO atTimelineGridCell.tsx:18).EventTimelinePremiumHeaderin theEventTimeline#22222 had inner level rows as plain<div>s withoutrole="row". Header cells had norole="columnheader", noaria-colindex, and noaria-colspanfor clamped or multi-tick cells.EventCalendar
MiniCalendaralready had the role hierarchy but noaria-colindex,aria-rowindexon cells.DayTimeGrid(Day and Week views) hadrole="row",role="columnheader",role="gridcell"but no indices on most cells, and the time-grid container had no enclosing row.Approach
Defaults are added to the headless primitives so consumers benefit automatically; styled wrappers then override the indices explicitly only where the layout has a fixed leftmost column (the all-day column in
DayTimeGrid, the week-number column inMonthView). The override path uses the standard pattern of reading the value fromelementPropsfirst and falling back to a computed default when none is provided.The outer header row in
EventTimelinePremiumContentkeeps its existingrole="row"+aria-rowindex={1}, matching what #22265 lists in its current-state checklist; body row math (index + 2) is preserved so the keyboard-navigation tests continue to pass unchanged.Changes
Timeline (headless premium)
packages/x-scheduler-headless-premium/src/timeline-grid/cell/TimelineGridCell.tsxroleswitched from'cell'to'gridcell'. Consumers can override via theroleprop, which is what lets the styled wrappers setcolumnheaderandpresentationfurther up.aria-colindexvia Composite; the styled layer now setsaria-colindexexplicitly because the layout context (which logical column the cell sits in) is known there, not in the primitive.packages/x-scheduler-headless-premium/src/timeline-grid/header/TimelineGridHeader.tsx<div>now getsrole="row".<div>now getsrole="columnheader",aria-colindex={cell.index + 2}(column 1 is reserved for the resource title column), andaria-colspan={cell.spanInTicks}whenspanInTicks > 1(handles both grouping rows and clamped first/last cells).packages/x-scheduler-headless-premium/src/timeline-grid/root/TimelineGridRoot.tsxaria-colcount={presetConfig.tickCount + 1}andaria-rowcount={resources.length + 1}on the root.Timeline (styled premium)
packages/x-scheduler-premium/src/event-timeline-premium/content/EventTimelinePremiumContent.tsxrole="columnheader"witharia-colindex={1}.role="presentation"so the inner level rows and column headers carry the grid semantics. The outer header row keepsrole="row"andaria-rowindex={1}as before.packages/x-scheduler-premium/src/event-timeline-premium/content/timeline-title-cell/EventTimelinePremiumTitleCell.tsxaria-colindex={1}. Therole="gridcell"default comes from theTimelineGridCellprimitive change above.Calendar (headless)
packages/x-scheduler-headless/src/calendar-grid/header-row/CalendarGridHeaderRow.tsxaria-rowindexto1; an explicit value passed via element props wins.packages/x-scheduler-headless/src/calendar-grid/day-row/CalendarGridDayRow.tsxaria-rowindextorowIndex + 2(header row reserved at index 1); an explicit value passed via element props wins.packages/x-scheduler-headless/src/calendar-grid/header-cell/CalendarGridHeaderCell.tsxaria-colindextoindex + 1(fromuseCompositeListItem); an explicit value passed via element props wins.packages/x-scheduler-headless/src/calendar-grid/day-cell/CalendarGridDayCell.tsxaria-colindextoindex + 1; an explicit value passed via element props wins.packages/x-scheduler-headless/src/calendar-grid/time-column/CalendarGridTimeColumn.tsxaria-colindextoindex + 1; an explicit value passed via element props wins.Calendar (styled)
packages/x-scheduler/src/event-calendar/mini-calendar/MiniCalendar.tsxaria-rowcount={1 + weeks.length},aria-colcount={7}.aria-rowindex={1}. Each weekday header cell:aria-colindex={dayIndex + 1}.aria-rowindex={weekIndex + 2}. Each day cell:aria-colindex={dayIndex + 1}.packages/x-scheduler/src/internals/components/day-time-grid/DayTimeGrid.tsxaria-rowcount={3},aria-colcount={1 + days.length}.aria-rowindex={1}. Each day header cell:aria-colindex={index + 2}(column 1 is reserved for the all-day fixed cell).aria-rowindex={2}. The all-day header cell on the left:aria-colindex={1}.DayTimeGridGrid) now hasrole="row"andaria-rowindex={3}. Without this, theTimeGridColumncells (eachrole="gridcell") had no enclosing row, which is invalid ARIA.packages/x-scheduler/src/internals/components/day-time-grid/DayGridCell.tsxcolIndexprop and forwards it asaria-colindex.packages/x-scheduler/src/internals/components/day-time-grid/TimeGridColumn.tsxcolIndexprop and forwards it asaria-colindex.packages/x-scheduler/src/month-view/MonthView.tsxaria-rowcount={1 + weeks.length},aria-colcount={(showWeekNumber ? 1 : 0) + 7}.aria-rowindex={1}. Week-number header cell (when present):role="columnheader",aria-colindex={1}. Day header cells:aria-colindex={dayIdx + (showWeekNumber ? 2 : 1)}.packages/x-scheduler/src/month-view/month-view-row/MonthViewWeekRow.tsxcolOffset = showWeekNumber ? 2 : 1. Week-number row header cell getsaria-colindex={1}. EachMonthViewCellis givencolIndex={dayIdx + colOffset}.packages/x-scheduler/src/month-view/month-view-row/MonthViewCell.tsxcolIndexprop and forwards it asaria-colindex.Tests
New assertions cover the additions, no existing test was modified.
packages/x-scheduler-premium/src/event-timeline-premium/content/timeline-header/EventTimelinePremiumHeader.test.tsxrole="row"on each inner level row.role="columnheader"andaria-colindex(offset by 1 for the title column) on each leaf header cell of thedayAndHourpreset.aria-colspan="24"on groupingdaycells indayAndHour, and noaria-colspanon single-tickhourcells.dayAndMonthcarryaria-colspan="29"andaria-colspan="27"respectively.aria-colcountandaria-rowcounton the grid root.packages/x-scheduler-premium/src/event-timeline-premium/EventTimelinePremium.test.tsxrole="columnheader"witharia-colindex="1".role="presentation".role="gridcell"witharia-colindex="1".aria-rowcountmatches1 + resources.length.packages/x-scheduler/src/event-calendar/mini-calendar/MiniCalendar.test.tsxaria-colcount="7"andaria-rowcount="7"on the grid.aria-rowindexruns 1 through 7 across the weekday header row and the 6 week rows.aria-colindexruns 1 through 7 across the weekday header cells.aria-colindexcycles 1 through 7 across all 42 day cells.packages/x-scheduler/src/week-view/tests/WeekView.test.tsxaria-rowcount="3"andaria-colcount="8"on the day-time grid root.aria-colindexfrom 2 to 8.aria-colindex="1".aria-rowindex="2"; time row hasaria-rowindex="3".aria-colindexfrom 2 to 8.packages/x-scheduler/src/month-view/tests/MonthView.test.tsxaria-colcount="7"on the grid andaria-rowcountgreater than 1.aria-rowindex="1"with column headers ataria-colindex1 to 7.aria-rowindex={weekIdx + 2}with day cells ataria-colindex1 to 7.The pre-existing
packages/x-scheduler-headless-premium/src/timeline-grid/tests/keyboard-navigation.TimelineGrid.test.tsxwas not modified and still passes: the outer header row'saria-rowindex={1}and body row mathindex + 2are preserved.Out of scope
useTimelineGridRowKeyboard,getNavigationTarget, the focused-cell context inTimelineGridRootandCalendarGridRoot) are untouched.Verification
The new aria assertions added in this PR run as part of the same
pnpm test:unitinvocation.Test plan
--max-warnings 0.pnpm test:unit --project "x-scheduler*" --runis green (1325 passed, 12 pre-existing skips).keyboard-navigation.TimelineGrid.test.tsxstill passes without modification.