[scheduler] Allow creating events via keyboard - EventCalendar#21967
[scheduler] Allow creating events via keyboard - EventCalendar#21967rita-codes wants to merge 16 commits intomui:masterfrom
Conversation
…llow-creating-events-via-keyboard
|
Deploy preview: https://deploy-preview-21967--material-ui-x.netlify.app/ Bundle size report
|
flaviendelangle
left a comment
There was a problem hiding this comment.
PR Review Summary: #21967
[scheduler] Allow creating events via keyboard - EventCalendar
Critical Issues (2 found)
-
ArrowRighthas no upper-bound check — focus silently lost
getNavigationTarget.ts:19—ArrowLeftguardscolumnIndex > 0, butArrowRightunconditionally returnscolumnIndex + 1. When pressed on the last column,setFocusedCellstores an out-of-range index, no cell matcheshasFocus, and the roving tabindex breaks — focus disappears with no feedback.Fix: Pass
columnCountinto the function and guard:return columnIndex < columnCount - 1 ? { rowType, columnIndex: columnIndex + 1 } : null; -
ArrowDown/ArrowUpnavigate to row types that may not be rendered
getNavigationTarget.ts:6—ROW_ORDERis hardcoded as['header', 'day-grid', 'time-grid']. Not all views render all three row types (e.g., month view may lacktime-grid). Navigating to a non-existent row silently loses focus.Fix: Make
ROW_ORDERdynamic (derived from which rows are actually rendered), or validate coordinates before applying focus.
Important Issues (2 found)
-
CalendarGridHeaderCellstarts withtabIndex: -1, unreachable via Tab
CalendarGridHeaderCell.tsxusestabIndex: hasFocus ? 0 : -1, whileDayCellandTimeColumnusetabIndex: focusedCell === null || hasFocus ? 0 : -1. In the initial state (focusedCell === null), all header cells gettabIndex=-1and are completely unreachable by Tab. This is inconsistent and likely a bug.Fix: Use the same pattern:
tabIndex: focusedCell === null || hasFocus ? 0 : -1 -
handleFocusinCalendarGridHeaderCellmissing child-event guard
DayCellandTimeColumncheckevent.target === event.currentTargetbefore callingsetFocusedCell. The header cell'shandleFocuslacks this guard, so focus events bubbling from child buttons will incorrectly triggersetFocusedCell.Fix: Add
if (event.target === event.currentTarget)guard.
Test Coverage Gaps (3 found)
-
No unit tests for
getNavigationTarget.ts(Criticality: 9/10) — This pure function contains all navigation logic and is the ideal candidate for thorough unit testing. All boundary conditions (ArrowLeft at 0, ArrowUp from header, row transitions, non-arrow keys) should be covered. -
No tests for
CalendarGridHeaderCellkeyboard behavior (Criticality: 8/10) — The header cell has unique Enter-delegates-to-button behavior that is completely untested. -
No tests for arrow key navigation between cells (Criticality: 7/10) — The headline feature (arrow key movement) has zero test coverage. Only Enter-key creation is tested.
Type Design Suggestions (3 found)
-
GridCellRowTypeoverlaps withEventSurfaceType— Consider extracting shared members:type GridSurfaceType = 'day-grid' | 'time-grid'; type GridCellRowType = 'header' | GridSurfaceType; type EventSurfaceType = GridSurfaceType | 'timeline';
-
setFocusedCellcannot reset tonull— No public API to clear focus when it leaves the grid. Worth documenting whether this is intentional. -
Add
satisfies GridCellRowType[]toROW_ORDERso TypeScript catches invalid entries if the union changes.
Strengths
- Clean separation of keyboard creation (
useKeyboardEventCreation) from mouse creation (useEventCreation) getNavigationTargetis properly extracted as a pure utility — excellent for testability- Consistent
event.target === event.currentTargetguards in DayCell and TimeColumn handlers - Well-structured tests with proper provider wrappers and both positive/negative cases
- Good use of
nullinitial state for roving tabindex first-render behavior - No error suppression — errors propagate naturally through React boundaries
CreationPlaceholderFieldstype is well-designed with cleanOmitpattern
Recommended Action
- Must fix: Add upper-bound check for
ArrowRightingetNavigationTarget - Must fix: Align
CalendarGridHeaderCelltabIndex/focus patterns with the other cells - Should fix: Add unit tests for
getNavigationTarget(pure function, easy to test) - Should fix: Handle non-existent row types in vertical navigation
- Consider: Tests for arrow key navigation and header cell keyboard behavior
- Consider: Extract shared keyboard navigation logic into a reusable hook to reduce duplication across the 3 cell components
Issue #21927