Repository navigation
[scheduler] Finetune touch experience for time grid events and introduce editing drawer - #22624
Conversation
…mobile-week-view-poc
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
github-actions
Bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…mobile-week-view-poc
…te/mui-x into finetune-touch-experience-for-time-grid-events
github-actions
Bot
added
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
flaviendelangle
left a comment
flaviendelangle
left a comment
There was a problem hiding this comment.
PR review
This PR reworks touch editing for time-grid events (arming, touch drag/resize, mobile drawer) and consolidates the editing/positioning layer. The new pointer-resize, store state machine, and arming hooks are largely correct and well-tested. One merge-blocking regression stands out: dragging the desktop edit dialog now snaps back to the anchor whenever the dialog's content height changes (tab switch, validation message, wrapped title). Beyond that, a few medium issues cluster around recurring-scope repointing and multi-resource commits, plus some low-severity edge cases. Verification was static (JSDOM can't exercise the touch/pointer-capture paths).
Bugs (7)
1. 🔴 Dragged edit dialog snaps back to the anchor on any content resize
Location: packages/x-scheduler/src/internals/hooks/useAnchoredPosition.ts:23-31 (wired at packages/x-scheduler/src/internals/components/event-dialog/EventDialog.tsx:83)
popup.style.top = `${position.top}px`;
popup.style.left = `${position.left}px`;
onReposition?.(); // <-- fires on every reposition, incl. popup ResizeObserveruseAnchoredPosition now observes the popup with a ResizeObserver that calls updatePosition, and updatePosition unconditionally calls onReposition. For the desktop dialog onReposition is resetDrag (useDraggableDialog), which zeroes the drag offset and clears the translate(...). The previous PaperComponent reset the drag only on mount and on window resize — it had no ResizeObserver.
Failure scenario: User drags the edit dialog aside, then switches to the Recurrence tab (or a validation helperText appears, or the title wraps). The paper's height changes → ResizeObserver → updatePosition → resetDrag → the dialog jumps back onto the event, discarding the dragged position. Reproducible in normal desktop editing.
Fix: Don't call onReposition from the size-driven reposition path — only reset the drag on anchor identity change / window resize. E.g. pass a reason to updatePosition and skip onReposition for ResizeObserver-triggered updates, or observe only the anchor (not the popup) for the drag-reset case.
2. 🟠 Confirming a recurring-scope change repoints an occurrence the user never armed
Location: packages/x-scheduler-internals/src/internals/utils/SchedulerStore/SchedulerStore.ts:643-656 / repointEditingOccurrence (880-909)
The non-recurring commit path guards the editing-surface sync with isEditedOccurrence(state, placeholder.occurrenceKey) (useDropTarget.ts:261), but the recurring path in selectRecurringEventScope repoints editingOccurrence unconditionally — repointEditingOccurrence/setEditingOccurrenceTimes only check editingOccurrence == null, never that the resized occurrence is the armed one.
Failure scenario: Occurrence A of a recurring series is armed (toolbar visible). The user touch-drags a different recurring occurrence B (a pointer-drag emits no click, so useDisarmOnOutsidePointer doesn't disarm A) and confirms "this-and-following". editingOccurrence silently jumps from A to B — the toolbar and highlight move to an occurrence the user never armed.
Fix: Gate the repoint on the resized occurrence matching editingOccurrence (mirror the isEditedOccurrence check the non-recurring path already uses).
3. 🟠 Repointed recurring key is built in the display timezone but rendered keys use the data timezone
Location: packages/x-scheduler-internals/src/internals/utils/SchedulerStore/SchedulerStore.ts:896-897
key: isRecurring ? getRecurringOccurrenceKey(eventId, start, adapter) : ...start here is pendingRecurringEventOperation.changes.start, which originates from displayTimezone (the placeholder). But occurrence keys are rendered from the data timezone day (getRecurringEventOccurrencesForVisibleDays.ts:222, day = startOfDay(...dataTimezone)). getRecurringOccurrenceKey reduces the date to a calendar-day string, which is timezone-boundary sensitive.
Failure scenario: With displayTimezone ≠ the event's dataTimezone such that an occurrence's start lands on a different calendar date in each zone, a "this-and-following" resize produces a key matching no rendered occurrence. isEditedOccurrenceArmed/isEditedOccurrence then return false everywhere, the armed toolbar disappears, and the re-anchor effect (EventEditingContext.tsx:94-98) never fires — stranding the surface on the unmounted node. Default (same-tz) usage is unaffected.
Fix: Derive the repoint key from the data-timezone start, consistent with occurrence expansion.
4. 🟠 Resize/drag commit collapses multi-resource events to their primary resource
Location: packages/x-scheduler-internals/src/internals/utils/useDropTarget.ts:222-226, session built at CalendarGridTimeEventResizeHandler.tsx:74
resource is public API as SchedulerResourceId | SchedulerResourceId[] | null and stored verbatim by processEvent. getResizeSession/drop uses getPrimaryResourceId(...resource) (first element only), and the commit writes changes.resource = placeholder.resourceId. A resize that should never touch resources drops all but the first.
Failure scenario: An event with resource: ['r1','r2'] is resized → changes.resource = 'r1' → r2 is permanently lost. This is pre-existing shared commit logic (already reachable via mouse-drag on master), but this PR routes the new touch-resize path through it and re-touches the function.
Fix: Preserve the original resource shape on resize (only overwrite when the drop actually reassigns resources). Related: in hasChanged (useDropTarget.ts:242) an array single-resource ['r1'] compares !== 'r1', so a no-op resize registers as changed and, for recurring events, spuriously reopens the scope dialog (🟡 low).
5. 🟡 In-progress touch-resize placeholder is orphaned if the handler unmounts mid-gesture
Location: packages/x-scheduler-internals/src/internals/utils/useEventPointerResizeHandler.ts:195-203
The cleanup intentionally leaves sessionRef/activePointerIdRef and any written placeholder untouched (so a dependency-flip re-run continues the gesture). But on a true unmount mid-gesture (view nav, event re-keys out of view, resource filter toggled) the detached element's pointerup/pointercancel never fire, and the !enabled revert branch only runs on a dependency flip — so an internal-resize placeholder can stay in the store as a stuck ghost preview.
Fix: On unmount, if a resize session is active and its placeholder is internal-resize, clear it (store.setOccurrencePlaceholder(null)).
6. 🟡 Edited trigger never clears the anchor on unmount, so the null-guard can't catch a detached node
Location: packages/x-scheduler/src/internals/components/event-editing/EventEditingContext.tsx:94-98
The re-anchor effect only ever sets the anchor while isEdited; there's no setAnchor(null) cleanup. If the edited event's trigger unmounts while the dialog stays open (event filtered out or removed by external data while editingOccurrence is still set), anchorRef.current keeps pointing at a detached node; calculatePosition reads zeros and the dialog recenters on screen with stale data.
Fix: Return a cleanup that clears the anchor when the edited trigger unmounts.
7. ℹ️ Notes
getResizeSessiontreats any activecreationplaceholder as the resize target without checkingsurfaceType/handle ownership (CalendarGridTimeEventResizeHandler.tsx:55-63) — grabbing an existing event's handle while a creation preview is live resizes the creation instead. Requires coexistence; unlikely.repointEditingOccurrence/setEditingOccurrenceTimesrefresh onlydisplayTimezone, leavingoccurrence.dataTimezone(andrrule) stale (SchedulerStore.ts:861-871, 899-906). Visible behavior readsdisplayTimezone, so harmless today — worth a comment noting the intentional partial update.
Tests (1)
1. 🟡 No regression test pins the dialog drag-position across content resizes
Location: packages/x-scheduler/src/internals/hooks/useAnchoredPosition.ts
Finding #1 (drag snaps back on content-height change) and the asymmetric/timezone repoint cases (#2, #3) are untested. The store-level repointEditingOccurrence tests are good, but they exercise the private method directly rather than the selectRecurringEventScope → repoint flow, so the missing occurrence-key guard and the display-vs-data-tz key mismatch slip through.
Failure scenario: A future refactor of useAnchoredPosition or the repoint path re-breaks drag persistence / toolbar tracking with no failing test to catch it.
Simplifications (0)
No findings.
Docs (0)
No findings. The EventDialog* → EventEditing* renames and localization/API JSON regen are consistent.
Verdict
Request changes — the draggable edit dialog resetting its position on any content resize (#1) is a user-visible desktop regression of a shipped feature; the recurring-scope repoint issues (#2, #3) and multi-resource collapse (#4) are worth resolving before merge.
🤖 Review generated with Claude Code
github-actions
Bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…uch-experience-for-time-grid-events
github-actions
Bot
removed
the
PR: out-of-date
github-actions
Bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…uch-experience-for-time-grid-events
…uch-experience-for-time-grid-events
github-actions
Bot
added
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…uch-experience-for-time-grid-events
github-actions
Bot
added
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
flaviendelangle
left a comment
flaviendelangle
left a comment
There was a problem hiding this comment.
Sorry 😆
PR review
Large, well-structured refactor: the createModal factory is gone, editing state moves onto the store with an explicit 'armed' | 'edit' mode, and the touch/desktop TimeGridEvent forks collapse into one CSS-driven component. The consolidation is sound and the store-level logic (repointEditingOccurrence, clampResizedEventEdge, the selector split) is well tested. One merge-blocking issue: the creation/resize placeholder is now rendered as a real, focusable role="button" that also carries aria-hidden="true". A second cluster of concerns is the armed toolbar's document-global click/scroll blockers, which don't account for dialogs stacked on top of it.
Bugs (4)
1. 🔴 The time-grid placeholder is now a focusable role="button" marked aria-hidden="true"
Location: packages/x-scheduler/src/internals/components/event/time-grid-event/TimeGridEvent.tsx:483
<TimeGridEventPlaceholderRoot
isDraggable={false}
eventId={occurrence.id}
occurrenceKey={occurrence.key}
renderDragPreview={(parameters) => <EventDragPreview {...parameters} />}
data-armed={placeholderHasResizeHandles || undefined}
// Inert preview — hide its button role/empty name from assistive tech (handles are
// pointer-only, so this doesn't trap focus).
aria-hidden="true"The placeholder root switched from CalendarGrid.TimeEventPlaceholder (an inert div — no role, no tabIndex) to CalendarGrid.TimeEvent. CalendarGridTimeEvent runs useButton({ disabled: false, native: false, tabIndex: columnHasFocus ? 0 : -1 }), which emits role="button" plus a tabIndex attribute (packages/x-scheduler-internals/src/calendar-grid/time-event/CalendarGridTimeEvent.tsx:123-127, base-ui-copy/utils/useButton.ts:212). Combining that with aria-hidden="true" is exactly the aria-hidden-focus violation: a keyboard-reachable element that assistive tech cannot see or announce. The inline comment's premise ("handles are pointer-only, so this doesn't trap focus") only covers the resize handles, not the root itself.
The same construct is repeated for the drag preview root at TimeGridEvent.tsx:444.
Failure scenario: Click an empty slot in the week view to create an event — the column now owns roving focus, so the placeholder renders with role="button" tabIndex="0" aria-hidden="true". Because the dialog also gained disableEnforceFocus (finding 3), tabbing out of the form lands focus on that element: a screen-reader user hears nothing, and axe reports an aria-hidden-focus violation on any page embedding the scheduler.
Fix: Keep the interactive TimeEvent root (it is needed to host the pointer resize handles) but drop aria-hidden and make it non-focusable and non-semantic instead — e.g. pass role="presentation" / tabIndex={-1} explicitly, or add an interactive={false} escape hatch to CalendarGridTimeEvent (the // TODO: Expose a real \interactive` propatCalendarGridTimeEvent.tsx:47is exactly this) so the placeholder renders a plaindiv` with the resize-handle children.
2. 🟠 The armed toolbar's global handlers swallow the recurring-scope dialog's backdrop click and block its scrolling
Location: packages/x-scheduler/src/internals/components/event-toolbar/AnchoredEventToolbar.tsx:41
useDisarmOnOutsidePointer({
ref: nodeRef,
active: true,
onDisarm: stopEditing,
ignoreSelector: `.${eventCalendarClasses.timeGridEventResizeHandler}, [role="dialog"]`,
global: true,
});
useBlockScrollWhileArmed({
active: true,
ignoreSelector: `.${eventCalendarClasses.timeGridEventResizeHandler}`,
});While an occurrence is armed, EventToolbar's Delete on a recurring event opens the recurring scope dialog without leaving the armed mode (EventToolbar.tsx:76-88 only calls stopEditing via onSubmit). The toolbar therefore keeps both document-global handlers installed on top of the dialog:
- The click handler exempts
[role="dialog"], but MUI putsrole="dialog"on the paper; theMuiBackdrop-rootandMuiDialog-containerare siblings. A backdrop click isstopPropagation()/preventDefault()-ed and only callsstopEditing(), so MUI's ownonClose(_, 'backdropClick')never fires. useBlockScrollWhileArmedhas no dialog exemption at all: itpreventDefault()s everywheel/touchmoveat document capture, including inside the open scope dialog and anywhere else on the host page.
Failure scenario: On a tablet, tap a recurring event (armed), tap Delete → the scope dialog opens. Tapping the dim area outside it does nothing: the dialog stays open while the toolbar silently disappears. On a short viewport, the scope radio list also cannot be scrolled to reach the confirm button.
Fix: Deactivate both hooks while schedulerOtherSelectors.isRecurringScopeDialogOpen is true (pass active={!isScopeDialogOpen}), and broaden the ignore selector to the whole modal root (.MuiModal-root, not just [role="dialog"]) so stacked surfaces keep their own dismissal semantics.
3. 🟠 disableEnforceFocus lets keyboard focus land behind a backdrop that still blocks the pointer
Location: packages/x-scheduler/src/internals/components/event-dialog/EventDialog.tsx:130
aria-modal="false"
// Non-modal: keep the grid behind usable (focus + scroll) while the dialog is open.
disableEnforceFocusThe comment's premise doesn't hold: EventDialogRoot still renders a full-viewport Backdrop and .MuiDialog-container (only their background is made transparent), so the grid behind is not usable — any click there is consumed by the container and closes the dialog. Removing focus enforcement therefore only lets keyboard users reach controls they cannot activate, and it is what makes the placeholder in finding 1 tab-reachable.
Failure scenario: Open the event dialog and press Tab past the last field. Focus moves onto a time-grid event behind the backdrop; pressing Enter/Space or clicking it closes the dialog instead of activating it, and the focus ring sits on an element the mouse cannot reach.
Fix: Either drop disableEnforceFocus (keeping the dialog's focus scope consistent with its blocking backdrop), or go fully non-modal — hideBackdrop, disableScrollLock, and sx={{ pointerEvents: 'none' }} on the container with pointerEvents: 'auto' on the paper — so the stated "keep the grid behind usable" is actually true.
4. 🟡 Resize handles collapse to zero size on devices matching neither (hover: hover) nor (pointer: coarse)
Location: packages/x-scheduler/src/internals/components/event/time-grid-event/TimeGridEvent.tsx:337
})({
position: 'absolute',
zIndex: 3,
cursor: 'ns-resize',
// Mouse: a thin, full-width bar revealed when the event is hovered.
[HOVER_MEDIA]: { height: 4, left: 0, right: 0, /* … */ },
// Touch: a circular dot with a large hit area, shown only once the event is armed.
[TOUCH_MEDIA]: { ...getTouchResizeHandleStyles(), display: 'none', /* … */ },
});Before this PR, height: 4; left: 0; right: 0 and the [data-start]/[data-end] offsets were unconditional. They are now only inside the two media blocks, which are assumed to partition every device. A device reporting hover: none with pointer: fine (stylus-primary and some kiosk/embedded browsers) matches neither, leaving position: absolute with no size and no offsets. prefersArmedOnTouch() also returns false there, so the event opens the dialog directly and never gets the armed dot either.
Failure scenario: On a hover: none + pointer: fine device, the resize handle renders as a 0×0 box: an event that reports isResizable can no longer be resized by any input method.
Fix: Keep the base geometry (height: 4; left: 0; right: 0, [data-start] { top: 0 }, [data-end] { bottom: 0 }) outside the media queries and let TOUCH_MEDIA override it, so the mouse layout is the fallback rather than one of two exclusive branches.
Tests (2)
1. 🟠 No view-level coverage of the outside-tap disarm contract
Location: packages/x-scheduler/src/day-view/tests/eventToolbar.DayView.test.tsx:40
useDisarmOnOutsidePointer and useBlockScrollWhileArmed have thorough standalone hook tests, but nothing exercises them wired into a view. The two behaviors that carry the real risk are precisely the integration ones: that a grid tap while armed disarms without arming the tapped event (the "two-tap contract" in DayTimeGrid.tsx:359), and that the global blockers don't break surfaces stacked above the toolbar (finding 2).
Failure scenario: The [role="dialog"] exemption in AnchoredEventToolbar could be deleted, or active: isEditing in DayTimeGrid widened to include 'edit' mode on a non-modal surface, and the whole suite would still pass.
Fix: Add to eventToolbar.DayView.test.tsx: (a) arm event A, click event B, assert B is not armed and A is disarmed; (b) with the premium anchoredToolbar suite, arm a recurring event, click Delete, and assert a backdrop click closes the scope dialog.
2. 🟡 pointercancel reverting an in-progress resize is untested
Location: packages/x-scheduler-internals/src/internals/utils/useEventPointerResizeHandler.ts:119
// Gesture aborted by the system (e.g. scroll took over): discard the resize preview so it never edits the event.
const onPointerCancel = (event: PointerEvent) => { /* … */ };touchResize.*.test.tsx only drives the happy path (simulatePointerResize → down/move/up). The cancel path, and the enabled-flips-mid-gesture cleanup right above it, are the branches where a stale placeholder would silently commit or strand a preview.
Failure scenario: A regression that drops the store.setOccurrencePlaceholder(null) in onPointerCancel leaves the resized preview on screen after the OS takes over the gesture, with no test failing.
Fix: Add a case that dispatches pointerdown → pointermove → pointercancel and asserts onEventsChange was not called and no placeholder remains.
Simplifications (3)
1. 🟡 eventTitlePlaceholder was added to 39 locale files for an unreachable branch
Location: packages/x-scheduler/src/internals/components/compact-event-drawer/CompactReadonlyContent.tsx:54
const showPlaceholder = isCreating && !occurrence.title;CompactReadonlyContent renders only when schedulerEventSelectors.isReadOnly(occurrence.id) is true (CompactEventDrawer.tsx:89), while isCreating requires an active creation placeholder. isEventReadOnlySelector returns false for an id with no processed event (schedulerEventSelectors.ts:19-23), and a globally read-only scheduler doesn't allow creation, so the two conditions can't hold together. showPlaceholder is always false.
Failure scenario: A new required key was threaded through EventEditingLocaleText, every one of the 39 locale files, and data.json (totalKeysCount 95 → 99) for a string that never renders — future translators will spend effort on it and reviewers will keep re-checking it.
Fix: Drop showPlaceholder/eventTitlePlaceholder for now, or move the placeholder into the editable drawer path where a titleless creation draft actually appears.
2. 🟡 CreationResizeSession in the pointer resize hook is unreachable
Location: packages/x-scheduler-internals/src/internals/utils/useEventPointerResizeHandler.ts:92
if (session.kind === 'creation') {
// Creation placeholder has no underlying event: keep its `creation` type so it stays editable.
store.setOccurrencePlaceholder({ type: 'creation', /* … */ });
return;
}getInitialEditingMode returns 'edit' whenever isCreating is true (editingModePolicy.ts:26), and TimeGridEventPlaceholder only renders handles when editingMode !== 'edit' (TimeGridEvent.tsx:399-401). Since TimeGridColumn's effect starts editing as soon as a creation placeholder appears in range, the creation placeholder's handles exist for at most one frame — the sizing gesture they exist for can never start.
Failure scenario: ~40 lines of hook code plus a public CreationResizeSession type on useEventPointerResizeHandler.ResizeSession are carried and must be kept correct for a path no user reaches; the EventResizeSession | CreationResizeSession union also forces every caller to narrow.
Fix: Either remove the creation branch and narrow ResizeSession to EventResizeSession, or (if creation-sizing is the intent) make getInitialEditingMode return 'armed' for creation on the drawer surface so the handles survive.
3. ℹ️ CalendarGrid.TimeEventPlaceholder has no remaining consumer
Location: packages/x-scheduler-internals/src/calendar-grid/time-event-placeholder/CalendarGridTimeEventPlaceholder.tsx:10
With TimeGridEventShared.tsx deleted and TimeGridEventPlaceholderRoot now built on CalendarGrid.TimeEvent, the only references left are index.parts.ts and the component's own test.
Failure scenario: A dead CalendarGrid part stays in the internals surface, and the next author writing a placeholder reaches for it and re-introduces the inert-vs-interactive mismatch this PR just worked around.
Fix: Drop the part, or add a comment on it explaining it's kept for the day-grid / external consumers.
Docs (1)
1. 🟡 The new resize-hook JSDoc describes an either/or that the wiring doesn't implement
Location: packages/x-scheduler-internals/src/internals/utils/useEventResizeHandler.ts:7
/**
* Native drag-and-drop resize for calendar events. The host picks this or the pointer-based resize
* ({@link useEventPointerResizeHandler}) via `enabled`.
*/and, on the same parameter:
/**
* Whether to attach the native drag-and-drop listeners (false when the side is clipped or the
* pointer interaction is active).
*/
enabled: boolean;The actual host computes a single enabled from isResizeHandlerEnabled and passes the same value to both hooks — its own comment says so: // Shared by both resize handlers running together (CalendarGridTimeEventResizeHandler.tsx:311). Nothing ever sets enabled to false "because the pointer interaction is active"; the mouse-vs-touch split happens inside onPointerDown via event.pointerType === 'mouse'.
Two related type docs are stale for the same reason: SchedulerEditingMode's 'armed' is documented as "Touch-only." and SchedulerEditingState.mode as "non-touch opens directly in 'edit'" (SchedulerStore.types.ts:35-49), but getInitialEditingMode arms unconditionally on the 'drawer' surface — editingModePolicy.test.ts:38-43 asserts exactly that for a fine pointer.
Failure scenario: A maintainer debugging a double-commit reads "the host picks this or the pointer-based resize" and rules out both handlers running on the same handle, which is precisely what happens.
Fix: Reword to "Both handlers share one enabled (the edge is inside the collection); the mouse is served here and touch/pen by useEventPointerResizeHandler, which bails on pointerType === 'mouse'", and change the 'armed' docs to "the drawer surface always arms; the dialog surface arms on a coarse pointer".
Verdict
Request changes - the placeholder now renders a focusable role="button" under aria-hidden="true", an axe-detectable accessibility regression that ships to every time-grid view.
🤖 Review generated with Claude Code
…uch-experience-for-time-grid-events
…uch-experience-for-time-grid-events
github-actions
Bot
removed
the
PR: out-of-date
Behaviors added
Major code changes
createModalfactory (previously backingEventDialogandMoreEventsPopover). Positioning is now handled by a reusableuseAnchoredPositionhook +AnchoredEventToolbar, decoupling the surface from its anchoring logic.event-editinglayer (EventEditingContext,FormContent,ReadonlyEventDetails,editingModePolicy) so the desktop dialog and mobile drawer render from the same code instead of duplicating it.editingOccurrence(with an'armed' | 'edit'mode) andstartEditing/setEditingMode/stopEditing, giving arming, resize, and the toolbar a single source of truth.TimeGridEvent— merged the separateTimeGridEventShared/TimeGridEventTouchcomponents into one, with a shareduseEventPointerResizeHandler.event-toolbarandarmed-occurrence(useDisarmOnOutsidePointer,useBlockScrollWhileArmed) internals, plus new locale keys for the toolbar buttons.Why
The previous long-press-to-drag interaction was hard to discover on touch, and the desktop-only dialog didn't fit small screens. Arming + a dedicated drawer make editing on touch discoverable and ergonomic, while consolidating the editing/positioning logic removes the parallel touch/desktop code paths.
Coming in a follow-up