[pickers] Commit TimeClock value when the drag ends outside the clock#22877
Merged
Conversation
Dragging the analog clock hand and releasing the pointer outside the clock left the value uncommitted (mui/material-ui#30083): the move/up handlers were bound to the clock mask only, so a release outside the mask never fired the 'finish' commit and the displayed time reverted. Migrate the Clock to Pointer Events as a single source of truth for mouse, touch and pen (matching DateRangeCalendar's useDragRange): pointerdown on the mask, with pointermove/pointerup/pointercancel on the owner document, so a drag keeps tracking and commits the angle wherever it is released. Convert the clock test infra from touch events to pointer events via a shared fireClockPointerEvent helper; the interaction tests now run in both jsdom and the browser. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
LukasTy
added
scope: pickers
LukasTy
added
type: bug
LukasTy
marked this pull request as ready for review
June 18, 2026 07:58
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where dragging the TimeClock hand and releasing the pointer outside the clock mask would fail to commit the final value (no final onChange(..., 'finish')), causing the UI to appear updated while the value reverted.
Changes:
- Migrates
TimeClock/Clockinteractions from separate mouse + touch handlers to unified Pointer Events, withpointermove/pointerup/pointercancellisteners attached to the owner document to keep tracking outside the mask. - Adds a shared
fireClockPointerEventtest helper and converts existing clock interaction tests from touch events to pointer events. - Adds a regression test that releases the pointer on
document.body(outside the clock) and asserts the value commits and the view advances.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/utils/pickers/viewHandlers.ts | Switches the shared time clock view handler from touch events to pointer events via the new helper. |
| test/utils/pickers/clock.ts | Adds fireClockPointerEvent helper that converts clock-relative coordinates into viewport clientX/clientY. |
| packages/x-date-pickers/src/TimeClock/tests/timezone.TimeClock.test.tsx | Converts timezone-related TimeClock interaction tests from touch to pointer events. |
| packages/x-date-pickers/src/TimeClock/tests/TimeClock.test.tsx | Converts interaction tests to pointer events and adds regression coverage for releasing outside the clock. |
| packages/x-date-pickers/src/TimeClock/Clock.tsx | Replaces touch/mouse logic with document-level pointer event tracking to ensure commits occur even when released outside the mask. |
| packages/x-date-pickers/src/StaticTimePicker/StaticTimePicker.test.tsx | Converts StaticTimePicker clock interaction tests from touch to pointer events (no longer touch-support gated). |
| packages/x-date-pickers/src/MobileTimePicker/tests/MobileTimePicker.test.tsx | Updates pointer-based interactions to use viewport coordinates derived from the clock’s live bounding rect. |
| packages/x-date-pickers/src/MobileTimePicker/tests/describeValue.MobileTimePicker.test.tsx | Converts describe-value interaction steps from touch events to pointer events. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A document-level pointermove/pointerup can fire between React detaching the mask ref and the cleanup effect removing the listeners (unmount mid-drag), which would dereference a null ref. Bail out of setTime when the mask is gone. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Use stopTracking() instead of only detaching the document listeners when a new primary pointerdown supersedes a previous gesture, so isMoving/activePointerId are reset in one place. Matches useDragRange's "reset before starting" pattern. Co-Authored-By: Claude Opus 4.8 <[email protected]>
michelengelen
approved these changes
Jun 23, 2026
michelengelen
left a comment
michelengelen
left a comment
Member
There was a problem hiding this comment.
Review: PR #22877 — [pickers] Commit TimeClock value when the drag ends outside the clock
Summary
Fixes #22863: dragging the analog clock hand and releasing the pointer outside the 220×220 clock mask left the value uncommitted because mouse/touch handlers were bound only to the mask. The PR migrates Clock from separate mouse/touch handlers to Pointer Events, attaching pointermove/pointerup/pointercancel to the owner document so a drag keeps tracking and commits wherever it's released. It also converts the clock test suite from touch events to pointer events so the tests run in both jsdom and the browser.
Correctness — looks solid
- handlePointerDown bails on disabled/readOnly before any state is touched, matching the existing handleValueChange gate.
- Plain clicks (no movement) still work: pointerdown fires setTime(..., 'shallow'), and the document pointerup listener (matched on pointerId) calls setTime(..., 'finish') — equivalent to the old unconditional setTime call in handleMouseUp.
- pointercancel correctly drops the gesture without committing ("UA interrupted, not the user"), which is the right semantic and is not covered by a test (see below).
- stopTracking() on a fresh pointerdown recovers from a previous gesture whose pointerup was lost — same defensive pattern as useDragRange in x-date-pickers-pro, which this PR explicitly mirrors.
- The isMoving.current ref is still correctly toggled so handleKeyDown's early-return guard keeps working.
- Cleanup on unmount (React.useEffect(() => () => removeDragListenersRef.current?.(), [])) avoids leaking document listeners, and setTime's if (!mask) return guards the React-detaches-ref-before-cleanup-runs race during unmount.
- touchAction: 'none' is already set on ClockSquareMask, so the lack of event.preventDefault() in handlePointerDown (only present in the document move handler) isn't a scroll-hijack regression.
Minor issues / suggestions
- event.button !== 0 vs event.button > 0 (Clock.tsx handlePointerDown): the sibling implementation in useDragRange.ts (x-date-pickers-pro) deliberately uses > 0 with the comment "keeps the gesture permissive when event.button is left unset by a synthetic event (some test environments)". Clock.tsx uses the stricter !== 0, which would silently reject a pointerdown if button were undefined. Real browsers set button: 0 for touch/primary-mouse, so this likely isn't a live bug, but it's an inconsistency between two near-identical patterns in the same codebase — worth aligning, or at least leaving a comment on why this one differs.
- Missing test for pointercancel: the PR adds handleDocumentPointerCancel (drag interrupted → no commit), but no test exercises it. Given the regression test already added for "release outside the clock," a parallel one for "pointercancel mid-drag → value does not commit" would lock in that behavior and catch regressions.
- Missing test for the "lost pointerup" recovery path: stopTracking() is called at the top of handlePointerDown specifically to handle a fresh pointerdown while a previous gesture is still considered active. No test exercises two consecutive pointerdowns without an intervening pointerup.
- removeDragListenersRef.current?.() double-duty: stopTracking is called both from handlePointerDown (to clear a stale gesture) and from the document pointerup/pointercancel handlers. This is fine functionally, but since removeDragListenersRef.current is reset to undefined inside its own cleanup closure, double-calling stopTracking() (e.g., cancel firing right after up due to a race) is already safely a no-op — good, just worth noting as intentional rather than incidental.
Test coverage
- Touch→pointer test conversion looks mechanical and correct; the new fireClockPointerEvent helper in test/utils/pickers/clock.ts is a reasonable, minimal addition.
- The new regression test (TimeClock.test.tsx, "should keep tracking the drag and commit the value when released outside the clock") directly targets the reported bug and is well-targeted — drags from 13:-- to 19:-- and releases on document.body.
- Removing it.skipIf(!hasTouchSupport) guards is correct now that these are pointer-based and run in jsdom, and the hasTouchSupport import cleanup in StaticTimePicker.test.tsx/TimeClock.test.tsx is appropriately tidied up.
- As noted above, pointercancel and the "stale gesture recovery" path are the two behavior branches added by this PR that aren't directly tested.
Style / conventions
- Comments are well-targeted at non-obvious invariants (the unmount race in setTime, the rationale for resolving coordinates via document-level events rather than offsetX/offsetY), consistent with the project's "why, not what" comment guidance.
- No public API changes, so no proptypes/docs regen needed.
- useEventCallback/ownerDocument usage matches the established x-date-pickers-pro drag pattern referenced in the PR description.
Risk
Low — this is an internal interaction-handling change with no public API surface change. The main risk is platform-specific Pointer Events quirks (Safari/older Android), but using pointerId gating and isPrimary checks is the right defensive approach, again mirroring the already-shipped useDragRange implementation.
… tests - Use `event.button > 0` (not `!== 0`) in handlePointerDown so a synthetic pointerdown with an unset button isn't rejected, matching useDragRange. - Note that stopTracking is idempotent (safe pointercancel/pointerup race). - Add tests for the two behavior branches that were uncovered: pointercancel mid-drag drops the gesture without committing, and a fresh pointerdown recovers and commits when a previous gesture's pointerup was lost. Co-Authored-By: Claude Opus 4.8 <[email protected]>
mbrookes
pushed a commit
to mbrookes/mui-x
that referenced
this pull request
Jun 27, 2026
…mui#22877) Co-authored-by: Claude Opus 4.8 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #22863.
Dragging the analog clock hand and releasing the pointer outside the clock left the value uncommitted: the displayed hand had moved, but
onChangenever fired the final commit and the value reverted. Themousemove/mouseup(and touch) handlers were bound to the 220x220 clock mask only, so once the pointer left the mask the drag stopped tracking and a release outside never triggered the'finish'selection state.Changes
Clockto Pointer Events as a single source of truth for mouse, touch and pen, replacing the separate mouse + touch handlers. This matches the convention established byDateRangeCalendar'suseDragRange.onPointerDownon the mask;pointermove/pointerup/pointercancelare attached to the owner document, so a drag keeps tracking and commits the angle wherever it is released (inside or outside the clock).isPrimaryonly,pointerIdgating,pointercanceldrops the gesture, and listeners are cleaned up on unmount.isTimeDisabled(unchanged).Tests
fireClockPointerEventhelper and convert the clock interaction tests (and the sharedtimeClockHandler) from touch events to pointer events. They now run in both jsdom and the browser (the old touch tests were browser-only).document.body(outside the clock) and asserts the value commits and the view advances.