Repository navigation
[pickers] Only scroll the Digital Clock when its active item changes - #23942
Merged
Merged
Conversation
The effect set `scrollTop` on every render, so any re-render, like the one when the day calendar loses focus in DateTimePicker, scrolled the list back to the selected time and the click on another time was lost. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
LukasTy
added
type: bug
LukasTy
changed the title
DigitalClock when its active item changes
LukasTy
reviewed
Oct 21, 2026
LukasTy
left a comment
LukasTy
left a comment
Member
There was a problem hiding this comment.
Thank you for the contribution. 🙏
I also reviewed with Claude, which found these new issues/regressions
The fix works for the reported case, and Tab / Shift+Tab focus behaves as on master. The new guard also skips two scrolls that are necessary, and the same bug remains in MultiSectionDigitalClockSection.
- Regression: hidden first mount. If the first effect runs while the list is hidden (
display: none), the guard stores the item and never scrolls to it later. Example: a single-columnDesktopDateTimePickerwithslotProps={{ popper: { keepMounted: true } }}and 12:30 PM selected opens with the list at midnight (scrollTop0, master: 1050). The clock does not take focus there, soactiveItem.focus()does not correct it. Reproduced in Chromium and WebKit. - Regression: layout change without scroll anchoring (Safari). If items above the active item change but the active element stays (
skipDisabledwith a newminTimeorshouldDisableTime, or a newtimeStep), the oldscrollTopstays and the selected time leaves the view. WebKit shows this by default. Chromium shows it only withoverflow-anchor: none. - Suggested fix for both: store
offsetTopwith the item and skip only when both match (snippet below). A "skip while hidden" check does not work in jsdom, whereoffsetParent,clientHeightandgetClientRects()are always empty. With this change, the press stays fixed, both cases scroll again in Chromium and WebKit, and the new test passes in jsdom and in the browser. - Same bug, not covered:
MultiSectionDigitalClockSection(the defaultDateTimePickertime view) also jumps. In Chromium, a 1 s press on hour "01" scrolls the hours section from 0 to 172 px, and the pointer is then over "05" (master and this PR). ItspreviousActiveguard applies only after the section focuses an item, so the comparison in the description does not hold while focus is in the calendar. Apply the same guard there, or do not close the issue with this PR. - Tests: the new test only checks that a re-render does not scroll. Add a case where a new active item (for example a new
value) still scrolls, and a browser test for the hidden mount.
const lastScrollRef = React.useRef<{ item: HTMLElement; offsetTop: number } | null>(null);
// Only scroll when the active item or its position changes, so that re-renders don't undo the user's scrolling
if (lastScrollRef.current?.item === activeItem && lastScrollRef.current.offsetTop === offsetTop) {
return;
}
lastScrollRef.current = { item: activeItem, offsetTop };Will you cover the same issue on MultiSectionDigitalClock in this PR, or should I create a separate one?
…iSectionDigitalClock Skipping the scroll whenever the active item was the same also skipped two scrolls that are needed: after a hidden first mount, and when items above the active one change. Remember the item's offsetTop too and only skip when both match. MultiSectionDigitalClockSection had the same jump, because its previousActive check only applies once the section has focused an item, so it gets the same guard. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
Author
|
Thanks a lot for the thorough review! I'll cover Pushed dfa1f84:
|
LukasTy
added
needs cherry-pick
LukasTy
changed the title
DigitalClock when its active item changes
LukasTy
approved these changes
Oct 21, 2026
LukasTy
left a comment
LukasTy
left a comment
Member
There was a problem hiding this comment.
Nice work, thank you for fixing this and promptly addressing feedback. 🙏
LukasTy
enabled auto-merge (squash)
October 21, 2026 14:11
Contributor
|
Cherry-pick PRs will be created targeting branches: v8.x |
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.
Fixes #15758
DigitalClockandMultiSectionDigitalClockcould scroll back to the selected time on unrelated re-renders. InDateTimePicker, pressing the mouse on a time blurs the day calendar, which changesfocusedViewand re-renders the clock. The list could then jump back to the selected time before the mouse was released, causing the click to land on another item or nowhere. The multi-section clock had the same issue because its previous scroll guard depended on the section having focused an item.Both
DigitalClockandMultiSectionDigitalClockSectionnow remember the last scrolled item together with itsoffsetTop, and skip scrolling only when both are unchanged. This preserves the user's scroll position on unrelated re-renders while still scrolling when the value changes, a clock mounted hidden becomes visible, or items above the active item change. Scroll tracking is separate from focus tracking, preserving the existing focus behavior.Regression tests cover unrelated re-renders and controlled value changes for both clocks, plus a browser-only test for
DigitalClockbecoming visible after a hidden first mount. TheDigitalClock,MultiSectionDigitalClock,TimePicker, andDateTimePickertests pass.I used Claude Code to write this change.