Skip to content

Commit 5bceab6

Browse files
benjamincanaczernoniaclaude
authored
fix(Listbox): prevent page scroll on mount caused by changeHighlight (#2666)
* fix(Listbox): prevent page scroll on mount caused by changeHighlight The immediate watcher on modelValue calls highlightSelected() on mount, which triggers el.focus() and el.scrollIntoView(). When the Listbox is below the fold, this scrolls the entire page down without user interaction. Pass preventScroll: true to focus() and skip scrollIntoView on the initial mount invocation, while preserving scroll behavior for subsequent programmatic changes and all user-driven highlights. * fix: skip focus and scrollIntoView on first changeHighlight call Simplify the approach: use a one-shot isInitialHighlight flag that skips focus() and scrollIntoView() on the very first changeHighlight invocation (the mount highlight). This covers all code paths including virtual listbox hooks without any timing dependencies (no rAF, no window API). * fix(Listbox): release initial-highlight guard after mount cycle The one-shot guard skipped focus/scroll on the first changeHighlight call ever, which could fire at the wrong time for deferred-render consumers (Combobox/Autocomplete open their listbox later). Release the guard on the nextTick after the initial mount highlight instead, so only the mount highlight is suppressed and every later highlight scrolls/focuses normally. Add tests: Listbox must not scroll the page or steal focus on mount but must do so on interaction, and Combobox must scroll the selected item into view on first open. * test(Listbox): drop unrelated Combobox test and story change The Combobox test didn't discriminate this fix (it passes on the old behavior too) and the story tweak only existed to support it. Keep the PR scoped to the Listbox page-scroll-on-mount fix and its mount test. * refactor(Listbox): thread scroll intent instead of mount-release guard Replaces the `isInitialHighlight` flag (released on a later `nextTick`) with an explicit `scroll` argument threaded through `highlightSelected()`. The "is this the mount highlight?" decision is now flipped synchronously in the modelValue watcher, so suppression travels with the call as an argument rather than via shared state cleared on a subsequent tick — it no longer depends on nextTick ordering, which differs between a client mount and SSR hydration. Behaviour is unchanged: the mount highlight only sets the roving-tabindex target (no focus, no page scroll), while user navigation and a Combobox opening its dropdown still focus/scroll as before. `changeHighlight` is restored to its original form (it already accepts scrollIntoView/focus). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: zernonia <zernonia@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 8212fa5 commit 5bceab6

2 files changed

Lines changed: 52 additions & 4 deletions

File tree

packages/core/src/Listbox/Listbox.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,41 @@ describe('given default Listbox', () => {
132132
})
133133
})
134134

135+
describe('given a Listbox on initial mount', () => {
136+
let wrapper: VueWrapper<InstanceType<typeof Listbox>>
137+
let scrollSpy: ReturnType<typeof vi.fn>
138+
139+
window.HTMLElement.prototype.releasePointerCapture = vi.fn()
140+
window.HTMLElement.prototype.hasPointerCapture = vi.fn()
141+
142+
beforeEach(async () => {
143+
scrollSpy = vi.fn()
144+
window.HTMLElement.prototype.scrollIntoView = scrollSpy
145+
document.body.innerHTML = ''
146+
wrapper = mount(Listbox, { attachTo: document.body })
147+
// let the immediate watcher's highlight cycle resolve
148+
await nextTick()
149+
await nextTick()
150+
})
151+
152+
it('should highlight the first item without scrolling the page or stealing focus', () => {
153+
const items = wrapper.findAll('[role=option]')
154+
// the item is highlighted for keyboard entry...
155+
expect(items[0].attributes('data-highlighted')).toBe('')
156+
// ...but the mount highlight must not focus it or scroll it into view,
157+
// otherwise a Listbox below the fold scrolls the whole page on load.
158+
expect(scrollSpy).not.toHaveBeenCalled()
159+
expect(document.activeElement).not.toBe(items[0].element)
160+
})
161+
162+
it('should focus and scroll once the user interacts', async () => {
163+
await wrapper.find('[role=listbox]').trigger('focus')
164+
const items = wrapper.findAll('[role=option]')
165+
expect(document.activeElement).toBe(items[0].element)
166+
expect(scrollSpy).toHaveBeenCalled()
167+
})
168+
})
169+
135170
describe('given multiple `true` Listbox', () => {
136171
const kbd = useKbd()
137172
let wrapper: VueWrapper<InstanceType<typeof Listbox>>

packages/core/src/Listbox/ListboxRoot.vue

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,7 @@ function handleMultipleReplace(event: KeyboardEvent, targetEl: HTMLElement) {
324324
}
325325
}
326326
327-
async function highlightSelected(event?: Event) {
327+
async function highlightSelected(event?: Event, scroll = true) {
328328
// highlightSelected is called inside a watch with immediate set to true.
329329
// This results in code execution during SSR.
330330
// Ensure this code only runs in a browser environment, since it performs
@@ -339,18 +339,31 @@ async function highlightSelected(event?: Event) {
339339
else {
340340
const collection = getCollectionItem()
341341
const item = collection.find(i => i.dataset.state === 'checked')
342+
// On the initial (mount) highlight we only set the roving-tabindex target.
343+
// Focusing/scrolling here would scroll the page to a Listbox the user never
344+
// interacted with (e.g. one below the fold). Later highlights scroll as before.
345+
const focus = scroll ? undefined : false
342346
if (item)
343-
changeHighlight(item)
347+
changeHighlight(item, scroll, focus)
344348
else if (collection.length)
345-
changeHighlight(collection[0])
349+
changeHighlight(collection[0], scroll, focus)
346350
}
347351
}
348352
353+
// `false` until the initial (mount) modelValue highlight has been queued.
354+
// Flipped synchronously in the watcher so the "is this the mount highlight?"
355+
// decision never depends on nextTick ordering, which differs between a client
356+
// mount and SSR hydration. The intent travels with the call as an argument
357+
// rather than via a shared flag released on a later tick.
358+
let hasHighlightedOnMount = false
359+
349360
// watch for only programmatic changes
350361
watch(modelValue, () => {
351362
if (!isUserAction.value) {
363+
const scroll = hasHighlightedOnMount
364+
hasHighlightedOnMount = true
352365
nextTick(() => {
353-
highlightSelected()
366+
highlightSelected(undefined, scroll)
354367
})
355368
}
356369
}, { immediate: true, deep: true })

0 commit comments

Comments
 (0)