fix(Select): keep the user's scroll position when the scroll down button remounts - #2109
Open
xyrolle wants to merge 1 commit into
Open
fix(Select): keep the user's scroll position when the scroll down button remounts#2109xyrolle wants to merge 1 commit into
xyrolle wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 1eb939d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
xyrolle
force-pushed
the
fix/select-scroll-button-remount-realign
branch
from
August 19, 2026 21:25
b03ed14 to
96ce35a
Compare
xyrolle
force-pushed
the
fix/select-scroll-button-remount-realign
branch
from
August 19, 2026 21:29
96ce35a to
1eb939d
Compare
…ton remounts The scroll down button unmounts at the bottom of the list and remounts as soon as the viewport leaves it, and its mount effect realigned the viewport onto the highlighted item. A small scroll up from the bottom therefore jumped back to the highlighted item, which for a pointer-driven scroll is the first item at the top of the list. SelectContentState now carries a userHasScrolled latch, set by wheel and touchmove on the viewport and by the scroll buttons' own auto-scroll, and reset when the content closes. The realign is skipped once it is set, so it still converges while the content is settling and stops once the user owns the scroll position.
xyrolle
force-pushed
the
fix/select-scroll-button-remount-realign
branch
from
August 19, 2026 21:40
1eb939d to
1b83558
Compare
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.
The problem
Scrolling a
Select(orCombobox) list back up from the bottom snaps the viewport onto the highlighted item.SelectScrollDownButtonStaterealigns the viewport onto the highlighted item on everymountedtransition of the scroll down button:That button renders under
{#if canScrollDown}, andcanScrollDowngoes false at the bottom of the list and true again as soon as the viewport leaves the bottom by more than itspaddingTop. So the button unmounts at the bottom and remounts on the user's very first scroll back up — and the realign fires straight into their gesture.The highlighted item is wherever it was put when the list opened, which for a pointer-driven scroll (no keyboard, cursor never over an item) is the first item. So "align onto the highlight" means "jump to the top".
Repro
Selectwith aSelect.ScrollDownButtonand enough items to scroll — the demo on the docs site reproduces it.The viewport jumps back to the top instead of moving a few pixels.
The test added here measures it: from
scrollTop2195 of a 2200 maxScroll, a 5px scroll up leaves the viewport at 0. Where I first hit this — a 25-option list in a 262px viewport, maxScroll 878 — a 5px wheel up from the bottom landed at 124 instead of 873, and a 30px wheel up also landed at 124. With this change they land at 873 and 848.Relationship to existing fixes
This looks like the same family as two recent ones:
fix(Select): scroll jumping) introducedcontentIsPositionedand routed this exactafterSleepthrough the newscrollHighlightedNodeIntoViewguard. But that guard only checksviewportNode && contentIsPositioned, and both stay true the whole time the content is open — so the remount realign survived it. This is the path that fix did not close.fix(DismissibleLayer)) was anafterSleepwhose callback ran without a condition that was still true when it fired. Same shape here: by the time the 5ms timer runs, "the button just mounted" no longer implies "the content is still settling".The fix
SelectContentStategains auserHasScrolledlatch. It is set when the user scrolls the viewport by hand —wheelortouchmoveon the viewport, or holding a scroll button — and reset when the content closes. The remount realign returns early once it is set.The realign therefore stays free to converge while the content is settling, and stops the moment the user takes the scroll position over.
Why not
isUserScrolling? That flag already exists onSelectScrollButtonImplStateand reusing it looked like the obvious one-line fix. It does kill this bug — but it also breaks the settle it is guarding, and the second test here demonstrates that in this repo.isUserScrollingis set from thescrolllistener, and the realign scrolls the viewport itself, so the content's own programmatic scrolls set the flag too. When the scroll buttons sit in the flex flow (as they do in the docs demo), mounting and unmounting them resizes the viewport under the alignment, so the open-time settle needs more than one pass — and withisUserScrollingas the guard the later passes are suppressed by the earlier one's own scroll. Opening a select whose selected item sits far down the list then leaves that item short of fully visible: swapping the guard makesshould still scroll the selected item into view when openingfail withexpected 244 to be less than or equal to 204, the item sitting exactly one item-height below the fold, while the remount test still passes.wheelandtouchmoveare the signals only a person can produce, which is why the latch listens for those rather than forscroll.What this deliberately does not change
setHighlightedNode, untouched.SelectScrollUpButtonStateneeded no change — it has no realign-on-mount watch.scrollbar-width: none), so there is none to drag. If that ever changes,scrollenddetection would be the thing to add.One extra line: the
watchinSelectScrollButtonImplStateended with a bareif (this.isUserScrolling) return;as its final statement, which does nothing. Removed, since it reads like the guard this bug needed and is not one.Tests
Two tests in
select.browser.test.ts, sharing a newselect-scroll-buttons-test.svelteharness:should keep the user's scroll position when the scroll down button remounts— the regression. Against the unpatched build it failsexpected 0 to be greater than or equal to 2180, deterministically across all retries.should still scroll the selected item into view when opening— pins the settle this fix must not break, and is what rules out theisUserScrollingvariant above.The existing scroll coverage (
select-scroll-jump-test.svelte, from #2005) measures page scroll on open and does not mount the scroll buttons at all — no select harness in the repo does, which is probably why this path stayed uncovered.The harness takes an
overlayScrollButtonsflag. The remount test overlays the buttons so their flapping does not change the viewport's scroll geometry, which keeps that test deterministic; in the flow they resize the viewport as they flap and the list settles ~24px short of the bottom. The settle test leaves them in the flow, which is the docs-demo layout and the one where the extra alignment pass matters.pnpm -F tests test:browser --run src/tests/selectpasses 74/74 on both chromium and webkit. The rest of the browser suite has some pre-existing failures on my machine (webkit focus-management timeouts, mostly), unchanged by this branch and in components this does not touch.Happy to rename anything or move the latch if you would rather it live on a different state.