fix(select,tree-select,autocomplete): panel scrolling in Safari (#DS-3299) - #1973
Merged
Conversation
|
Visit the preview URL for this PR (updated for commit 53db3cc): https://koobiq-next--prs-1973-cv7vlr0b.web.app (expires Sat, 05 Sep 2026 14:08:04 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are internally consistent (prevent-scroll + explicit reveal), deprecations match removed usage, and the added WebKit regression tests directly cover the Safari-specific failure mode.
Pull request overview
This PR addresses Safari/WebKit-specific panel scrolling regressions for select-like overlays (select, tree-select, autocomplete, dropdown) by preventing implicit scrolling triggered by HTMLElement.focus() and instead performing an explicit “reveal” of the active item.
Changes:
- Update option-like components to always focus with
{ preventScroll: true }and reveal viascrollIntoView({ block: 'nearest', inline: 'nearest' })when appropriate. - Add WebKit-only Playwright regression tests to ensure panels keep user scroll position stable and never rely on focus-driven scrolling.
- Deprecate legacy scroll-offset helpers/constants that are no longer used now that reveal logic lives on the focused item.
File summaries
| File | Description |
|---|---|
| tools/public_api_guard/components/core.api.md | Marks getOptionScrollPosition deprecated and updates extracted API metadata. |
| tools/public_api_guard/components/autocomplete.api.md | Marks AUTOCOMPLETE_PANEL_HEIGHT deprecated and exposes scrollActiveOptionIntoView in public API snapshot. |
| packages/e2e/utils/index.ts | Re-exports new focus/scroll E2E helpers. |
| packages/e2e/utils/focus-scroll.ts | Adds Playwright helpers to record preventScroll usage and assert scroll stability across frames. |
| packages/components/tree/tree-option.component.ts | Forces preventScroll on focus and explicitly reveals focused nodes (except for mouse-origin focus). |
| packages/components/tree-select/tree-select.component.ts | Restores scroll position on the actual scrolling container and aligns docs around focus-driven reveal. |
| packages/components/tree-select/e2e.webkit.playwright-spec.ts | Adds WebKit-only scrolling regression tests for tree-select. |
| packages/components/select/select.component.ts | Aligns internal scroll behavior docs with “open at top” + focus-driven reveal. |
| packages/components/select/e2e.webkit.playwright-spec.ts | Adds WebKit-only scrolling regression tests for select. |
| packages/components/dropdown/e2e.webkit.playwright-spec.ts | Adds WebKit-only scrolling regression tests for dropdown. |
| packages/components/dropdown/dropdown-item.component.ts | Forces preventScroll on focus and explicitly reveals focused items when appropriate. |
| packages/components/core/option/option.ts | Centralizes reveal-on-focus behavior in KbqOption.focus and deprecates legacy scroll-position helper. |
| packages/components/core/option/option.spec.ts | Adds unit tests asserting preventScroll: true and reveal behavior, including pointer-activation suppression. |
| packages/components/autocomplete/e2e.webkit.playwright-spec.ts | Adds WebKit-only scrolling regression tests for autocomplete. |
| packages/components/autocomplete/autocomplete.component.ts | Adds scrollActiveOptionIntoView() to delegate scroll/reveal behavior consistently. |
| packages/components/autocomplete/autocomplete-trigger.directive.ts | Deprecates unused panel height constant and routes scroll-into-view through the autocomplete API. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🚨 E2E tests failedReview the report for details. 💡 Comment |
lskramarov
force-pushed
the
fix/DS-3299_v2
branch
from
September 2, 2026 07:13
3292e5c to
bf9cc79
Compare
artembelik
approved these changes
Sep 2, 2026
NikGurev
approved these changes
Sep 2, 2026
…3299) The panels scrolled the active option into view by relying on the scroll that HTMLElement.focus() performs implicitly. Blink runs that scroll synchronously, but WebKit defers it to a later rendering update, where it lands after — and undoes — whatever the reader scrolled in the meantime. With hover re-activating options as they passed under the pointer, the panels could not be scrolled at all in Safari. Focus now always passes preventScroll, and the panels scroll explicitly through the new KbqScrollbarViewport.scrollIntoViewNearest, which moves the viewport by the shortest distance and leaves an already visible target alone. It measures from rects rather than an offsetParent walk, because a scrollport is not necessarily a containing block: kbq-select__content is position: static, so its options report the overlay pane as their offsetParent. Nearest-edge semantics also make pointer-driven activation a no-op, so the option's mouse-origin flag is no longer needed. Measured against Playwright's WebKit build: focus() followed by scrollTop = 64 left the panel at 848 two frames later, against 64 in Chromium. Also drops the dead getOptionScrollPosition and AUTOCOMPLETE_PANEL_HEIGHT exports, superseded by the explicit scroll, and fixes tree-select restoring scrollTop on the panel instead of on the option list that actually scrolls.
Same root cause as the select family. FocusKeyManager focuses the item it activates, and focus was left to scroll that item into view — a scroll WebKit defers past the reader's own, which made a dropdown taller than the viewport unscrollable in Safari. Items now focus with preventScroll, through both the FocusMonitor and the plain branch, and the panel follows the active item with scrollIntoViewNearest driven by keyManager.change. Only setActiveItem emits there; hover reaches the key manager through updateActiveItem, which does not, and would be a no-op anyway since a hovered item is already in view. The panel was confirmed to actually overflow before changing anything — scrollHeight 1288 against clientHeight 680 on the 40-item fixture — and the regression spec asserts that premise so the rest cannot pass vacuously.
…3299)
Review of the previous two commits found that pushing "focus never scrolls"
down into KbqOption, KbqTreeOption and KbqDropdownItem while restoring the
scroll only in four panels left every other consumer of those items with no
scrolling at all, and made the panels scroll on hover.
Measured regressions, all fixed here:
- Hovering a partially clipped option scrolled select and autocomplete under a
stationary pointer (scrollTop 50 -> 36). Only setActiveItem emits change, and
KbqOption's hover path goes through it, so the deleted mouse-origin guard had
no replacement.
- Standalone kbq-tree-selection stopped scrolling entirely; the tree package has
no scroll code of its own, and only tree-select was compensated.
- KbqAppSwitcher drives KbqDropdownItem with its own FocusKeyManager inside a
kbq-scrollbar, so the compensation added to KbqDropdown never reached it.
- A virtual-scroll select whose viewport lacks kbqScrollbarViewport resolved the
scroll target to the non-scrolling wrapper and dead-ended at the buffer edge.
- Reopening a tree-select reset the list to the top without revealing the
selected node, because change does not emit when the active index is unchanged.
Each item now focuses with preventScroll and reveals itself with
scrollIntoView({ block: 'nearest' }), skipping the reveal when the pointer
activated it or when focus did not actually move. The browser resolves the
scroll container, so this works for any consumer regardless of markup, and it is
synchronous in both engines, which is what the WebKit fix needs.
That removes the need for the panel-side plumbing: KbqScrollbarViewport
.scrollIntoViewNearest and its wrapper are gone again, along with the select's
activeScrollbarViewport getter and the dropdown's keyManager.change subscription.
Dropping the rect-based measurement also drops three latent faults review found
in it: RTL offsets fed into CDK's RTL-normalizing scrollTo, fractional rects
compared against integer client metrics, and transformed rects read from the
dropdown panel while its enter animation still had scale(0.8) applied.
The four WebKit specs now share helpers from packages/e2e/utils instead of
repeating them, install the focus recorder via addInitScript so the panel's
opening focus calls are covered too, and gain a guard for the hover case.
…(#DS-3299) Removing `getOptionScrollPosition` and `AUTOCOMPLETE_PANEL_HEIGHT` outright was a breaking change for downstream consumers: both were `@public` and reachable through the package barrels, so an import of either stopped compiling on upgrade, with no migration schematic and no `BREAKING CHANGE` note to warn anyone. Both are restored unchanged and marked `@deprecated` instead, with the reason and the replacement in the doc comment. Nothing in the library calls them — an option reveals itself on focus, which lets the browser resolve the scroll container — so they stay dead code until a major release can drop them. This leaves the branch free of compile-breaking API changes: the guard now records the two symbols as `@public @deprecated` rather than absent.
The "scrolls with the wheel and stays where the reader left it" test read the offset a fixed two frames after the wheel, then asserted it was unchanged two frames later. On CI that caught the gesture mid-flight — 199 first, 200 once it settled — and failed on a one-pixel difference in tree-select and select. The baseline now waits for the offset to stop changing rather than for a frame count, and the follow-up assertion allows a pixel: a deferred focus scroll, the thing this guards, moves the panel by at least a row, so sub-pixel settling is not the movement being watched for. The gesture settles within one frame locally, so this is reasoned from the CI failure rather than a local reproduction; 100 repeated runs of the four specs pass.
…roots (#DS-3299) Review of the previous commits found two defects in the reveal itself, both measured in Chromium against the e2e fixtures. `activatedByPointer` was cleared only inside `focus()`, but hovering the option that is already active never gets there — `setActiveItem` emits `change` only when the index actually moves. One such hover armed the flag permanently and silently swallowed every later reveal: with the panel scrolled to 400, the boundary key left the active option off-screen where it otherwise revealed it at 4. The flag now clears on `mouseleave`, so it means "the pointer is on me" rather than "someone hovered me once". `ownerDocument.activeElement` never resolves to an element inside a shadow root, so the re-entrancy guard was inert under `KbqShadowDomOverlayContainer` — exactly where `KbqTreeOption`'s `(focusin)` re-entrancy would scroll a second time and jump the tree under the cursor. It now reads through shadow roots with CDK's `_getFocusedElementPierceShadowDom`, as the autocomplete trigger already does. Both live in one `kbqFocusAndReveal` in `core/utils/dom.ts`, replacing the block that had been copied into three item classes with three different notions of "the pointer did this" — which is why only `KbqOption` carried the leak. Also: the autocomplete trigger re-focused its input on every arrow key and the dropdown focused its own scrollport, both without `preventScroll`, leaving the same deferred-scroll hazard one element over; `KbqAutocomplete` .scrollActiveOptionIntoView is gone again, a public one-line hop that moved focus off the input with the repair living at the call site. Tests: the wheel guard now waits for the offset to move before waiting for it to settle, so an unapplied gesture can no longer resolve as "never scrolled" and fail as a product regression; the page-scroll assertion gets a scrollable body, without which it could not fail on any of the four routes; and the option specs now pin that focus still moves on the pointer path and that a reveal returns once the pointer leaves. Known and deliberate: the reveal aligns to the nearest edge where the browser's implicit focus scroll centred (measured 640 against 720). That is the right behaviour for step-by-step navigation but changes where a jumped-to option lands on open, type-ahead and Home/End, in every engine.
`check-e2e-types` (`tsc -p tsconfig.playwright-spec.json --noEmit`) failed the linters job: `addInitScript` resolves to a `Disposable`, so returning it from a function declared `Promise<void>` is a type error. Nothing type-checked this file before — playwright.config.ts only transpiles the specs — which is why it compiled and ran fine. Awaited rather than returned, matching how `e2eDisableResizeObserver` was fixed in the same utils folder. Behaviour is unchanged: every caller already awaited the returned promise.
lskramarov
force-pushed
the
fix/DS-3299_v2
branch
from
September 2, 2026 14:04
5e562c4 to
53db3cc
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.
No description provided.