fix(autocomplete,select,tree-select): scroll into view in safari (#DS-3299)

[artembelik]

No description provided.

[Copilot]

Pull request overview

This PR fixes Safari-specific behavior where the active autocomplete option may not be scrolled into view when navigating with the keyboard, by explicitly calculating and setting the panel scroll position instead of relying on HTMLElement.focus() side effects.

Changes:

• Compute the correct scrollTop to fully reveal the active option and apply it to the panel on active-item changes.
• Add unit tests covering downward scroll, upward scroll, and “already visible” cases with stubbed geometry to work around jsdom’s lack of layout measurements.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
packages/components/autocomplete/autocomplete-trigger.directive.ts	Replaces focus-based scrolling with explicit scroll position calculation based on option geometry.
packages/components/autocomplete/autocomplete.spec.ts	Adds targeted unit tests validating the new scroll-into-view behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

Visit the preview URL for this PR (updated for commit 222e40f):

https://koobiq-next--prs-1919-gh1hq4ba.web.app

_{(expires Sat, 29 Aug 2026 11:31:27 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c}

[github-actions]

🚨 E2E tests failed

Review the report for details.

---

💡 Comment /approve-snapshots to approve snapshot changes.

[NikGurev]

Пропала рамка фокуса в tag-autocomplete, судя по скриншотам

[github-actions]

🚨 E2E tests failed

Review the report for details.

---

💡 Comment /approve-snapshots to approve snapshot changes.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

[artembelik]

/approve-snapshots

[github-actions]

🔄 Updating snapshots.

[artembelik]

Пропала рамка фокуса в tag-autocomplete, судя по скриншотам

она наоборот появилась, так и должно быть, раньше был баг - фокус уходил из инпут поля в опшен

[github-actions]

🚨 E2E tests failed

Review the report for details.

---

💡 Comment /approve-snapshots to approve snapshot changes.

[artembelik]

/approve-snapshots

[github-actions]

🔄 Updating snapshots.

[github-actions]

✅ Snapshots updated!

[lskramarov]

@artembelik а давай реализацию для "недобраузера" вынесем в отдельные хелперы и будем подключать только для него. Когда придет время и это пофиксят мы просто удалим хелперы.

[artembelik]

@artembelik а давай реализацию для "недобраузера" вынесем в отдельные хелперы и будем подключать только для него. Когда придет время и это пофиксят мы просто удалим хелперы.

как мне кажется ветвление под конкретный браузер здесь скорее усложнит поддержку, чем упростит.

мы не заводили отдельный хелпер под сафари, а переиспользовали уже имеющийся в репозитории getOptionScrollPosition (аналогично сделано в материал).

благодаря этому поведение в autocomplete / select / tree-select стало одинаковым (+ добавил тесты).

[lskramarov]

@artembelik а давай реализацию для "недобраузера" вынесем в отдельные хелперы и будем подключать только для него. Когда придет время и это пофиксят мы просто удалим хелперы.

как мне кажется ветвление под конкретный браузер здесь скорее усложнит поддержку, чем упростит.

мы не заводили отдельный хелпер под сафари, а переиспользовали уже имеющийся в репозитории getOptionScrollPosition (аналогично сделано в материал).

благодаря этому поведение в autocomplete / select / tree-select стало одинаковым (+ добавил тесты).

Вот, наглядно как было и как стало, вместо одной строки .focus(); теперь 3 реализации прокрутки:

я хочу все три варианта унифицировать в один хелпер и использовать его только для сафари, а для вэбкита оставить одну строку с вызовом фокуса

[lskramarov]

Deep pass over the scroll-into-view rewrite. Ten comments below; the first three are functional bugs, the rest are smaller.

1. select + kbqSelectSearch + *cdkVirtualFor is broken outright. viewport.getViewportSize() is a plain cache read in the CDK, and its value is measured once in a microtask after the viewport's ngOnInit — at which point the projected <cdk-virtual-scroll-viewport> is not in the document yet, so clientHeight is 0 and it stays 0 forever. Passing 0 as panelHeight makes getOptionScrollPosition scroll the active option one full row above the viewport top on every arrow key. Nothing in the repo pairs a virtual viewport with a search field, so no example and no spec covers this branch.

2. Hovering an option now scrolls the panel. KbqOption.onMouseenter() activates the option, which reaches scrollActiveOptionIntoView(). The old focus({ preventScroll: this.isFocusedByMouse }) existed precisely so that mouse-driven activation never scrolls; the manual scrollTop write has no equivalent, so the list moves under a stationary cursor.

3. if (!this.search()) is not the predicate that decides whether the search input exists. The template renders it under search() && shouldShowSearch(). A select with searchMinOptionsThreshold therefore takes the ActiveDescendant path while no input is rendered, and nothing in the panel gets DOM focus at all.

On the discussion above about extracting a single helper: the three copies have already diverged in ways that are bugs rather than style. The group-label case exists only in the autocomplete (see the comment on select.component.ts), the virtual-scroll case only in the select, and the two rect-based bodies in select/tree-select are byte-identical down to their comment. That divergence is the concrete cost of three implementations instead of one.

[lskramarov]

getViewportSize() is a bare cache read — getViewportSize() { return this._viewportSize; } in @angular/cdk fesm2022/scrolling.mjs, with _viewportSize = 0 as the initial value. It has exactly two writers: the ngOnInit microtask ngZone.runOutsideAngular(() => Promise.resolve().then(() => this._measureViewportSize())), and checkViewportSize(), whose only caller is viewportRuler.change() (window resize / orientationchange). grep -rn checkViewportSize packages/ returns nothing.

_measureViewportSize() reads viewportEl.clientHeight. The <cdk-virtual-scroll-viewport> is projected into the <ng-content /> at select.html:149, which lives inside the <ng-template cdkConnectedOverlay>, so at that microtask the element has no parent node and clientHeight is 0. _viewportSize then stays 0 for the component's lifetime, unless the user happens to resize the window while the panel is open.

With panelHeight === 0, getOptionScrollPosition always takes the second branch, because optionOffset + optionHeight > currentScrollPosition + 0 holds for every option that is not strictly above the current offset:

itemSize 32, currentOffset 0, active option fully visible at offset 64
expected: 0                              (already in view — leave it alone)
actual:   Math.max(0, 64 - 0 + 32) = 96

so scrollToOffset(96) pushes the active option one whole row above the top of the panel, on every arrow key.

A zero viewport size does not blank the list (FixedSizeVirtualScrollStrategy expands the rendered range from maxBufferPx alone), which is why the shipped virtual-scroll examples still look fine — they just never have a search field, so this branch is unreachable from them. The new spec only stubs the non-virtual optionsContainer.

Reading the live height instead — viewport.elementRef.nativeElement.clientHeight — fixes it, or call viewport.checkViewportSize() from onAttached().

[lskramarov]

This scroll now also runs on mouse hover, which it deliberately never did before.

KbqOption.onMouseenter() sets isFocusedByMouse = true and calls this.parent?.keyManager?.setActiveItem(this); KbqSelect provides KBQ_OPTION_PARENT_COMPONENT, and setActiveItem emits change on any index move, which lands in the subscription in initKeyManager(). Previously that path ended in element.focus({ preventScroll: this.isFocusedByMouse }) — the flag exists for exactly this case. Now it ends in an unconditional container.scrollTop = ....

Repro: searchable select, scrolled so the bottom row is half-clipped. Move the pointer onto that row — the list scrolls up under the stationary cursor, a different row slides under the pointer, mouseenter fires again, and it scrolls again.

Same at line 499 of autocomplete-trigger.directive.ts (KbqAutocomplete provides the same parent token). tree-select is not affected: its hover path goes through keyManager.updateActiveItem, which emits no change.

Two side effects worth noting: isFocusedByMouse is only ever cleared inside focus(), so with a search field it now latches true permanently; and the already-visible case still writes scrollTop unnecessarily — const next = getOptionScrollPosition(...); if (next !== container.scrollTop) { ... } would avoid that.

[lskramarov]

This is not the predicate that decides whether a search input exists. select.html renders it under @if (search() && shouldShowSearch()), and shouldShowSearch() returns false when searchMinOptionsThreshold is set, the search value is empty, and options.length is below the threshold.

search is a contentChild, and projected content is instantiated in the consumer's view, so the directive instance resolves even when the <ng-content> slot never renders. So for <kbq-select [searchMinOptionsThreshold]="10"> with a projected [kbqSelectSearch] and 5 options, search() is truthy and activeOption?.focus() is skipped — and handleKeydown also skips search.focus(), because it pairs both checks correctly:

if (search && this.shouldShowSearch()) {
    search.focus();
}

KbqSelect uses an ActiveDescendantKeyManager, which never focuses items itself, so after ArrowDown nothing inside the panel holds DOM focus: no focus ring, and the panel's (keydown) handler stops receiving events. Before this PR the method was an unconditional this.keyManager.activeItem?.focus(), so this is a regression for that configuration.

Line 1646 of tree-select.component.ts has the same mismatch — its own panelKeydownHandler uses search && this.shouldShowSearch() in two places.

[lskramarov]

kbq-select supports kbq-optgroup (readonly optionGroups = contentChildren(KbqOptgroup), plus grouped fixtures in select/e2e.ts, select-groups-example.ts and the SelectWithGroups spec host), but it has no counterpart to the index === 0 && labelCount === 1 case added to the autocomplete — in fact optionGroups is never read anywhere in this file.

Numbers, with --kbq-size-xxs: 4px padding on .kbq-select__content and a 32px group label:

scroll content:  padding [0,4)   label [4,36)   option 0 [36,68)
optionOffset(option 0) = 36

Scrolling back up to option 0 from anywhere below hits if (optionOffset < currentScrollPosition) return optionOffset; and sets scrollTop = 36, so the label at [4,36) ends up completely out of view — exactly what setScrollTop(0) prevents in the autocomplete.

tree-select has no optgroup support, so it is not affected. Worth noting the autocomplete's own guard is narrow too (index === 0), so the first option of the second or third group is still clipped there.

[lskramarov]

KbqAutocomplete.panel is viewChild.required<ElementRef>('panel') and #panel lives inside the component's <ng-template>, so the query is unresolved whenever the overlay is detached. Reading an unresolved required query throws unconditionally (RuntimeError(-951); only the message text is ngDevMode-gated, so production throws too).

scrollActiveOptionIntoView() is public API — it is the whole of the exported KeyboardNavigationHandler interface, and this PR removes its // (undocumented) marker in autocomplete.api.md. Sequence with the shipped default autoActiveFirstOption: true:

1. open the panel → resetActiveItem() → setFirstItemActive() → activeItemIndex === 0
2. press Escape → closePanel() clears overlayAttached and detaches, but never resets the key manager
3. call trigger.scrollActiveOptionIntoView()

index is 0, so the index < 0 guard misses; autocomplete.options is a @ContentChildren declared in the consumer's view and survives detach, so options[0] is truthy; execution reaches autocomplete.getScrollTop() on this line → const panel = this.panel(); → NG0951. The grouped index === 0 && labelCount === 1 branch throws the same way through setScrollTop(0).

The if (panel) checks inside setScrollTop/getScrollTop are dead code for a required query and cannot absorb it. MatAutocompleteTrigger._scrollToOption, which this mirrors, guards the whole block with else if (this.autocomplete.panel). The previous body (keyManager.activeItem?.focus()) was a safe no-op in every state.

The internal keyManager.change path is guarded by panelOpen, so this only affects the public call.

[lskramarov]

labelCount is read only on the next line, inside a branch already gated on index === 0 — for every other index this walk is thrown away. It runs on every arrow key, every type-ahead character and every option hover, so arrowing top-to-bottom through a 500-option grouped list costs roughly 125k discarded comparisons.

options.toArray() on the line above is a fresh _results.slice() per call. Note the signature change moved that copy out from behind the old if (optionGroups.length) guard, so an ungrouped autocomplete now allocates a full-length array per keystroke where it previously allocated nothing — and the only thing the array is used for is options[index].

element.offsetTop already accounts for every group label above the option, which is why the count is needed for nothing else. Something along these lines is O(1) in the common path:

const option = autocomplete.options.get(index);

if (index === 0 && countGroupLabelsBeforeOption(0, autocomplete.options.toArray(), autocomplete.optionGroups()) === 1) {

and it would also make the QueryList → readonly T[] signature change in core unnecessary.

Separately, autocomplete.panel().nativeElement.offsetHeight a few lines down is a per-render constant re-measured on every keystroke; caching it on attach and invalidating on options.changes removes a forced reflow per event.

[lskramarov]

Narrowing these from QueryList<T> to readonly T[] is a hard compile break for external callers, and the symbol stays // @public in core.api.md.

QueryList<T> exposes length, first, last, get, toArray, map, filter, find, reduce, forEach, some and [Symbol.iterator], but has no numeric index signature and none of concat/slice/indexOf/at/every/includes — so it is not assignable to readonly KbqOption[]. Anyone who wrote the call shape the old signature mandated:

countGroupLabelsBeforeOption(i, select.options, select.optionGroups)

(and KbqSelect.options is still published as QueryList<KbqOption>) now gets TS2345 after upgrading. There is no deprecated overload and no ng-update migration, and the PR is titled fix(...) with no BREAKING CHANGE footer.

Keeping the parameters as QueryList and calling .toArray() internally — as the function did before — avoids the break entirely; the only in-repo caller already has a QueryList in hand.

[lskramarov]

This is consumed only by the private scrollActiveOptionIntoView() and is never referenced from tree-select.html, yet it lands in tree-select.api.md as readonly optionsContainer: Signal<ElementRef<HTMLElement>>, which commits us to it through check-api. A consumer following the published surface and calling treeSelect.optionsContainer() while the panel is closed gets an NG0951 throw, since #optionsContainer lives inside the overlay <ng-template>.

Signal-based queries may be private, so private readonly optionsContainer = ... compiles identically and leaves the guard file untouched.

While this is being added: onAttached() still restores the saved scroll position onto this.panel()!.nativeElement, which has overflow: hidden and cannot scroll. The real scroller is the container this PR just made available — KbqSelect already writes to this.optionsContainer().nativeElement in the same place.

[lskramarov]

The note says the constant "no longer drives anything" because the height is now measured at runtime, but git grep AUTOCOMPLETE_PANEL_HEIGHT main -- packages apps returns only the declaration itself and the generated llms-full.txt — nothing read it before this PR either. The block comment this replaced already referred to constants that no longer existed.

So the rationale attributes the deadness to a change that did not cause it. Either leave the constant alone as out of scope, or reduce the note to the house style used elsewhere in the repo (/** @deprecated Not used. Will be removed in the next major release. */, cf. checkbox.ts, code-block.ts).

There is also an inconsistency to settle: this harmless constant is preserved forever "to avoid a breaking change", in the same commit that changes countGroupLabelsBeforeOption's public signature outright.

[lskramarov]

This case passes unchanged against the old implementation, which contradicts the comment above the describe ("Each case would fail against the old focus()-only implementation").

It starts at scrollTop = 0 and asserts toBe(0). getOptionScrollPosition(64, 32, 0, 256) takes neither branch and returns 0 — but keyManager.activeItem?.focus() never touched scrollTop in jsdom either, so the assertion holds with or without the fix. Same at autocomplete.spec.ts:955 and tree-select.component.spec.ts:3266. Seeding a non-zero scrollTop would make the already-visible case assert that the position is preserved rather than coincidentally 0.

Two related gaps in the same suites:

• the autocomplete block has no expect(document.activeElement).toBe(input) assertion, even though "focus stays on the input" is the entire rationale for the change there — its select and tree-select siblings both have one;
• no test anywhere pairs kbqSelectSearch with *cdkVirtualFor, so the virtual-scroll branch is completely uncovered (see the comment on select.component.ts).

[artembelik]

#1973