Skip to content

feat: add focusSelectedItem flag to combo-box#9194

Closed
DiegoCardoso wants to merge 7 commits into
fix/combo-box/connector-range-managementfrom
feat/combo-box/focus-selected-item
Closed

feat: add focusSelectedItem flag to combo-box#9194
DiegoCardoso wants to merge 7 commits into
fix/combo-box/connector-range-managementfrom
feat/combo-box/focus-selected-item

Conversation

@DiegoCardoso
Copy link
Copy Markdown
Contributor

@DiegoCardoso DiegoCardoso commented Apr 24, 2026

Summary

Re-enables an opt-in version of the dropdown behavior removed in web-components#6055: when the dropdown opens, the overlay scrolls to and visually focuses the currently-selected item. Useful for long lists where the user wants to see/confirm their selection on open.

Adds ComboBox.setFocusSelectedItem(boolean) / isFocusSelectedItem() plus an @ClientCallable resolveSelectedItemIndex() that resolves the authoritative index for the current value via ItemIndexProvider (lazy) or the list data view (in-memory). The connector listens for the dropdown-open event and scrolls to the resolved index.

Fixes vaadin/web-components#6061.

Depends on

Notes for reviewers

A summary comment with the deeper rationale (token-based cancellation, close-time KeyMapper re-key handling, microtask polling for the connector flag) is posted as a separate PR comment to keep the diff readable.

comboBox.addEventListener('vaadin-combo-box-dropdown-opened', resolveFocusSelectedItem);
// `_onOpened` doesn't re-run on filter change, so the web component can't
// re-arm its own `__shouldFocusSelectedItem` latch — re-resolve ourselves.
comboBox.addEventListener('filter-changed', () => {
Copy link
Copy Markdown
Contributor

@sissbruecker sissbruecker Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer if it would not do that. If I enter a new filter, I likely want to select a different item. Based on the filtered results I then want to scan the list from top to bottom. Then it doesn't sound helpful that it scrolls down somewhere to the current selection.

@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from cfa1c60 to 81e662d Compare April 27, 2026 16:56
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from 81e662d to c9c2fac Compare April 29, 2026 16:04
@DiegoCardoso DiegoCardoso changed the base branch from main to fix/combo-box/connector-range-management April 29, 2026 16:04
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from b953e19 to d2b850b Compare April 29, 2026 16:14
@vaadin vaadin deleted a comment from github-actions Bot Apr 29, 2026
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch 2 times, most recently from 988b0ff to bcb3096 Compare April 30, 2026 08:33
DiegoCardoso added a commit that referenced this pull request Apr 30, 2026
Drop comments that explain choices made during the PR rather than
non-obvious WHY of the resulting code. Historical context (why the
microtask poll, why the connector holds the flag, full close-time
reset rationale) moved to PR #9194.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from bcb3096 to 8576f92 Compare April 30, 2026 09:46
@DiegoCardoso
Copy link
Copy Markdown
Contributor Author

Reviewer notes — context for the trimmed comments

The latest commit (8576f92e7d) shortens a few inline comments that read more like commit-message material than code documentation. Posting the historical context here so reviewers have it without bloating the source files.

comboBoxConnector.jsfocusSelectedItemEnabled flag storage

The flag lives in a closure variable in the connector rather than as a property on the web-component element because the web-component does not declare or react to a `focusSelectedItem` property — Flow is the only consumer of this signal, so an element property would just be ignored by the WC and would expose a non-functional surface to non-Flow callers. `comboBox.$connector.setFocusSelectedItem(value)` keeps the surface explicit and Flow-scoped.

comboBoxConnector.jsopened-changed reset arm: KeyMapper rationale

The full version of the (now shortened) close-time-reset comment, for context:

Arm a DataCommunicator reset when the dropdown closes with focusSelectedItem enabled. On the next open, the feature scrolls to the selected item's position — the same viewport range the server last sent — and the server would no-op the fetch, leaving pending callbacks unresolved. Forcing a reset on the next setViewportRange RPC ensures the server re-sends data for the requested range.

The reset also re-keys items via the data communicator's normal passivate-and-unregister flow — items outside the post-reset active range have their KeyMapper entries removed and get fresh keys when re-fetched. Cached pages in the connector and the data-provider controller would then hold items with stale keys that no longer match the freshly-keyed selectedItem, breaking selection highlighting (and leaving them as targets for the focusSelectedItem RPC's scrollToIndex when it sees a real item at the resolved index but with a stale key). Wipe all client-side cache state on close so the next open populates from the post-reset server data with current keys.

The trimmed version keeps the load-bearing parts (server no-ops on unchanged range; KeyMapper re-keying invalidates cache) and drops the narrative buildup. The reasoning chain is preserved here.

ComboBox.javasetFocusSelectedItem microtask polling

Why the polling approach instead of runBeforeClientResponse ordering:

The connector's setFocusSelectedItem becomes available only once initLazy has run. Both this call (the JS in setFocusSelectedItem) and initLazy are queued via executeJs, but on the first attach there's no guarantee initLazy runs first. Polling via microtask until \$connector is ready is the simplest way to make ordering immaterial.

The trimmed code-comment now says only the load-bearing part: poll because attach order isn't guaranteed.

DiegoCardoso added a commit that referenced this pull request Apr 30, 2026
Drop comments that explain choices made during the PR rather than
non-obvious WHY of the resulting code. Historical context (why the
microtask poll, why the connector holds the flag, full close-time
reset rationale) moved to PR #9194.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from 8576f92 to ee8cb4c Compare April 30, 2026 14:11
DiegoCardoso added a commit that referenced this pull request May 4, 2026
Drop comments that explain choices made during the PR rather than
non-obvious WHY of the resulting code. Historical context (why the
microtask poll, why the connector holds the flag, full close-time
reset rationale) moved to PR #9194.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from ee8cb4c to d5c473f Compare May 4, 2026 14:14
@DiegoCardoso DiegoCardoso force-pushed the fix/combo-box/connector-range-management branch from 4e16446 to dd555eb Compare May 4, 2026 16:24
DiegoCardoso added a commit that referenced this pull request May 4, 2026
Drop comments that explain choices made during the PR rather than
non-obvious WHY of the resulting code. Historical context (why the
microtask poll, why the connector holds the flag, full close-time
reset rationale) moved to PR #9194.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from d5c473f to d3d5dfd Compare May 4, 2026 16:29
DiegoCardoso added a commit that referenced this pull request May 5, 2026
Drop comments that explain choices made during the PR rather than
non-obvious WHY of the resulting code. Historical context (why the
microtask poll, why the connector holds the flag, full close-time
reset rationale) moved to PR #9194.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from d3d5dfd to d1e5bb3 Compare May 5, 2026 08:34
DiegoCardoso added a commit that referenced this pull request May 5, 2026
Drop comments that explain choices made during the PR rather than
non-obvious WHY of the resulting code. Historical context (why the
microtask poll, why the connector holds the flag, full close-time
reset rationale) moved to PR #9194.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from d1e5bb3 to 90f8fb0 Compare May 5, 2026 11:33
@DiegoCardoso DiegoCardoso force-pushed the fix/combo-box/connector-range-management branch from 41130c6 to ec9814b Compare May 5, 2026 11:39
DiegoCardoso added a commit that referenced this pull request May 5, 2026
Drop comments that explain choices made during the PR rather than
non-obvious WHY of the resulting code. Historical context (why the
microtask poll, why the connector holds the flag, full close-time
reset rationale) moved to PR #9194.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from 90f8fb0 to e37d071 Compare May 5, 2026 11:40
@vaadin vaadin deleted a comment from github-actions Bot May 5, 2026
DiegoCardoso added a commit that referenced this pull request May 5, 2026
Drop comments that explain choices made during the PR rather than
non-obvious WHY of the resulting code. Historical context (why the
microtask poll, why the connector holds the flag, full close-time
reset rationale) moved to PR #9194.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from e37d071 to b484d19 Compare May 5, 2026 14:36
DiegoCardoso added a commit that referenced this pull request May 6, 2026
Drop comments that explain choices made during the PR rather than
non-obvious WHY of the resulting code. Historical context (why the
microtask poll, why the connector holds the flag, full close-time
reset rationale) moved to PR #9194.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from b484d19 to 720a058 Compare May 6, 2026 07:58
@DiegoCardoso DiegoCardoso force-pushed the fix/combo-box/connector-range-management branch from 6e45890 to 7da590b Compare May 6, 2026 12:15
DiegoCardoso and others added 7 commits May 6, 2026 14:15
Adds `setFocusSelectedItem(boolean)` / `isFocusSelectedItem()` to
`ComboBox`. When enabled and a value is set, opening the dropdown
scrolls to and focuses the selected item.

Index resolution happens server-side via a `@ClientCallable
resolveSelectedItemIndex()`:
- In-memory data view: `getListDataView().getItemIndex(T)`.
- Lazy data view: `getLazyDataView().getItemIndex(T)`, which delegates
  to the configured `ItemIndexProvider`.
- Returns null (silent fallback) when no value, no data provider, no
  ItemIndexProvider configured for a lazy data provider, or in-memory
  client-side filter mode (where the server doesn't see the filter).

The connector wires `vaadin-combo-box-dropdown-opened` to the resolve
flow with three-guard token cancellation (microtask entry, pre-RPC,
post-RPC) so stale invocations under rapid filter / open changes drop
correctly. The Java setter writes a connector-side flag via
`$connector.setFocusSelectedItem(value)` rather than a web-component
property, since the closed `vaadin/web-components#11554` PR removed
the WC's own `focusSelectedItem` property.

An `opened-changed` listener arms `serverFacade.needsDataCommunicatorReset()`
on close when `focusSelectedItem && selectedItem` is set, so the next
open's `setViewportRange` doesn't no-op against the server's
last-sent range and leave the combo stuck loading.

Adds `ComboBoxFocusSelectedItemPage` and `ComboBoxFocusSelectedItemIT`
covering preset values, lazy / in-memory / Person / Binder paths,
default-off, throwing-provider, no-provider, filter interactions, and
edge cases (push-update, detach-reattach, replace-items).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the dropdown closes with `focusSelectedItem` enabled, the
existing armed `serverFacade.needsDataCommunicatorReset()` triggers
the server's `dataCommunicator.reset()` on the next setViewportRange
RPC. That reset interacts with the data communicator's normal
passivate-and-unregister flow: items outside the active range
post-reset get their `KeyMapper` entries removed and are issued fresh
keys when re-fetched.

Cached pages in the connector (`cache`, `committedPages`) and in the
WC's data-provider controller (`rootCache`) survive the reset on the
client side, so they hold items with stale `key` values. After a
subsequent `setValue(...)` re-allocates a fresh key for the selected
item, selection-highlight comparison via `itemIdPath="key"` no longer
matches the cached item — the dropdown shows the selected item but
without the "selected" attribute, and the focusSelectedItem RPC's
`scrollToIndex(index)` lands on a real (cached) item whose key is
already orphaned.

Wipe all client-side cache state on close so the next open populates
from the post-reset server data with current keys.

Reproduces with the three-sequence test: click 123 + open/close,
click 9000 + open/close, click 123 + open. Without this fix, the
final open shows Item 123 highlighted as focused but not as selected,
because filteredItems[123].key still holds the original key from the
first sequence while selectedItem.key is the freshly-allocated one.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop comments that explain choices made during the PR rather than
non-obvious WHY of the resulting code. Historical context (why the
microtask poll, why the connector holds the flag, full close-time
reset rationale) moved to PR #9194.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lter

Listens on `opened-changed` instead of `vaadin-combo-box-dropdown-opened`
so the auto-focus resolve only fires on real opens — the WC briefly
toggles `_overlayOpened` during filter changes when the items list
empties and refills, and the previous listener was re-running the
resolve mid-filter, parking `__scrollToPendingIndex` on the next fetch
and re-scrolling once the data landed.

Also adds a `filter-changed` cleanup: resets `_focusedIndex` so the
WC's `_setDropdownItems` restoration logic falls through to label-match
(returning -1 for an empty filter) instead of carrying the previously-
focused item across the filter change, and bumps the resolve token to
cancel any in-flight resolve whose late `scrollToIndex` would otherwise
land after the filter changed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r code

Tightens the long block above `focusSelectedItemToken`, the per-async-edge
narration inside `resolveFocusSelectedItem`, and the listener docstrings.
WHY-comments stay (switchMap-style cancellation, the loading-settle
rationale, the `opened-changed` vs. `dropdown-opened` choice, the
`_setDropdownItems` filter-carry behavior, the close-handler reset
chain); WHAT-comments shrink or go.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@DiegoCardoso DiegoCardoso force-pushed the feat/combo-box/focus-selected-item branch from 376c4ff to a73e2ed Compare May 6, 2026 12:17
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 6, 2026

@DiegoCardoso
Copy link
Copy Markdown
Contributor Author

Closing in favor of #9238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[combo-box] Add API to re-enable focusing the selected item

2 participants