fix: preserve focused index across sibling page-loads in combo-box#11815
Merged
DiegoCardoso merged 7 commits intoMay 28, 2026
Merged
Conversation
When a dataProvider returns object items without itemValuePath, calling scrollToIndex(N) sets _focusedIndex=N correctly, but a subsequent sibling page-load drops it. _setDropdownItems re-derives _focusedIndex via __getItemIndexByValue(newItems, _getItemValue(focusedItem)); for objects without a value path _getItemValue falls back to item.toString() which collapses to '[object Object]', so the lookup matches the first object in the new array and focus snaps to index 0. Keyboard users pressing Arrow Down next jump from item 1 instead of N+1. Short-circuit when the same item reference still occupies _focusedIndex in the new array: __onDataProviderPageLoaded replaces only the array reference, not the item references at unchanged indices, so this matches the page-load mutation without affecting filter changes / item resets where references differ and the value-lookup fallback still runs. Apply the same change to MultiSelectComboBox's __setDropdownItems which has the same logic and the same bug.
DiegoCardoso
requested review from
vursen and
web-padawan
and removed request for
web-padawan
vursen
reviewed
May 26, 2026
Replace the ref-equality short-circuit added in the previous commit with a two-strategy lookup. When `itemIdPath` is set and the focused item has a resolvable id, search the new array by id and update `_focusedIndex` to the new position — this also covers the case where an item moves to a different index between data-provider updates (vursen's review suggestion on #11815). When `itemIdPath` is unset or the focused entry is a `ComboBoxPlaceholder` with no id, fall back to reference equality at the same index so the original bug — focus collapsing to 0 after a sibling page-load with placeholder refs — stays fixed. Apply the same shape to `MultiSelectComboBox`'s `__setDropdownItems`.
vursen's follow-up review pointed out that the actual bug only fires when `oldItems[focusedIndex]` and `newItems[focusedIndex]` are both `ComboBoxPlaceholder` instances. The previous commit's two-strategy block (`itemIdPath` search + reference equality) was over-engineered for that. Replace both branches with a single explicit `instanceof` check on the two slots. Update the unit tests in both packages to reproduce the actual placeholder-at-focusedIndex scenario (drain page 0, set focus to an unloaded slot, re-push items with a fresh placeholder at the same position) — the previous setup exercised a different toString-collapse path that is no longer the scope of this PR.
vursen
reviewed
May 26, 2026
vursen
reviewed
May 26, 2026
…cusedItem Trim the comment to describe just the scenario and intended outcome, and reuse the existing `focusedItem` local for the first instanceof check (per vursen's review on #11815).
Using `focusedItem` relied on `_getItemElements()` finding a rendered row, which fails when the focused index is outside the virtualizer's rendered range. Check `oldItems[_focusedIndex]` directly so the guard holds regardless of scroll position.
Replace the direct `comboBox._focusedIndex = 200` write with an `arrowUpKeyDown` keyboard event that wraps focus to the last index — a placeholder slot when only page 0 is drained. Uses a real combo-box API instead of touching the private property, with no behavior change to `scrollToIndex`. Same shape applied to the MSCB test.
DiegoCardoso
force-pushed
the
fix/combo-box-preserve-focused-index-after-page-load
branch
from
5884e39 to
d8f173a
Compare
|
vursen
approved these changes
May 28, 2026
DiegoCardoso
added a commit
that referenced
this pull request
May 28, 2026
…#11851) The previous implementation special-cased placeholder targets by calling `ensureFlatIndexLoaded` directly, and short-circuited the entire call when `loading` was true. That meant two separate paths handled the "page not loaded yet" case (the explicit branch and the natural `index-requested` chain), and a scroll requested mid-loading was deferred wholesale even when items already existed. This reworks the method into a single uniform flow: - Defer only when the dropdown isn't open or the items array is empty. - Move the viewport and set the focused row in one path, regardless of whether the target is a placeholder or a real item. - Queue post-attempt when `loading` flips true (a page request was triggered by the new viewport), so the call re-fires after the page lands. Setting `_focusedIndex` propagates to the scroller and drives viewport rendering; placeholder rows in the new viewport fire `index-requested` and load any missing pages through the data-provider chain — there's no longer a need for `scrollToIndex` itself to drive page loading. > [!NOTE] > On placeholder→real transitions, the value-based focus lookup in `_setDropdownItems` may briefly reset `_focusedIndex` before the post-attempt re-fire restores it. Part of #11815 hardens `_setDropdownItems` against this transition; that PR should land alongside or before this one. Related to #11815 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>
DiegoCardoso
deleted the
fix/combo-box-preserve-focused-index-after-page-load
branch
Collaborator
|
This ticket/PR has been released with Vaadin 25.2.0-beta2. |
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
3 participants
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.
Why
scrollToIndex(N)can land_focusedIndexon an unloaded (ComboBoxPlaceholder) row in data-provider mode. If the items array is then re-pushed — for example byclearCache()or a mid-scroll Flow-connector update — and the slot at_focusedIndexis still a placeholder in the new array,_setDropdownItemsreruns its value-based focus lookup:_getItemValuecalled on a placeholder returns its stringified form, which doesn't match any real item, so__getItemIndexByValuereturns-1and_focusedIndexcollapses. The user loses their keyboard position and Arrow Down advances from the wrong row.MultiSelectComboBoxhas the same lookup in__setDropdownItemsand the same bug.Reproduction
ComboBoxwith a pagedDataProvider.clearCache()is the simplest path; the Flow connector's mid-scroll updates do the same)._focusedIndexbecomes-1. The next Arrow Down advances from row 0.Changes
Short-circuit
_setDropdownItems(and__setDropdownItems) when the entries at_focusedIndexin both the old and the new arrays areComboBoxPlaceholderinstances. That transition carries no information about where the focused row actually moved to, so preserve_focusedIndexand skip the value-lookup. Filter changes and items resets — where the new array has real items at that index — still run the fallback as before.Related to #11851
Follow-up to #11552, Part of #6061