From 59dc3865532e717e487ae086e70f92f27f9f42ae Mon Sep 17 00:00:00 2001 From: lskramarov Date: Tue, 1 Sep 2026 21:49:42 +0300 Subject: [PATCH 1/7] fix(select,tree-select,autocomplete): panel scrolling in Safari (#DS-3299) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../autocomplete-trigger.directive.ts | 11 +- .../autocomplete/autocomplete.component.ts | 17 +++ .../e2e.webkit.playwright-spec.ts | 92 ++++++++++++ .../components/core/option/option.spec.ts | 22 +++ packages/components/core/option/option.ts | 48 +------ .../components/scrollbar/scrollbar.spec.ts | 116 +++++++++++++++ packages/components/scrollbar/scrollbar.ts | 52 +++++++ .../select/e2e.webkit.playwright-spec.ts | 135 ++++++++++++++++++ .../components/select/select.component.ts | 23 ++- .../tree-select/e2e.webkit.playwright-spec.ts | 93 ++++++++++++ .../tree-select/tree-select.component.ts | 20 ++- .../components/tree/tree-option.component.ts | 4 +- packages/e2e/utils/focus-scroll.ts | 32 +++++ packages/e2e/utils/index.ts | 1 + .../components/autocomplete.api.md | 4 +- tools/public_api_guard/components/core.api.md | 4 - .../components/scrollbar.api.md | 2 + 17 files changed, 610 insertions(+), 66 deletions(-) create mode 100644 packages/components/autocomplete/e2e.webkit.playwright-spec.ts create mode 100644 packages/components/select/e2e.webkit.playwright-spec.ts create mode 100644 packages/components/tree-select/e2e.webkit.playwright-spec.ts create mode 100644 packages/e2e/utils/focus-scroll.ts diff --git a/packages/components/autocomplete/autocomplete-trigger.directive.ts b/packages/components/autocomplete/autocomplete-trigger.directive.ts index 25d912ae38..4fdc7d9899 100644 --- a/packages/components/autocomplete/autocomplete-trigger.directive.ts +++ b/packages/components/autocomplete/autocomplete-trigger.directive.ts @@ -58,15 +58,6 @@ import { delay, filter, map, switchMap, take, tap } from 'rxjs/operators'; import { KbqAutocompleteOrigin } from './autocomplete-origin.directive'; import { KbqAutocomplete } from './autocomplete.component'; -/** - * The following style constants are necessary to save here in order - * to properly calculate the scrollTop of the panel. Because we are not - * actually focusing the active item, scroll must be handled manually. - */ - -/** The total height of the autocomplete panel. */ -export const AUTOCOMPLETE_PANEL_HEIGHT = 256; - /** * Injection token that determines the scroll handling while the autocomplete panel is open. The root default * keeps the trigger usable outside `KbqAutocompleteModule`'s injector; providing the token anywhere still wins @@ -468,7 +459,7 @@ export class KbqAutocompleteTrigger } scrollActiveOptionIntoView(): void { - this.autocomplete().keyManager.activeItem?.focus(); + this.autocomplete().scrollActiveOptionIntoView(); } /** Stream of clicks outside of the autocomplete panel. */ diff --git a/packages/components/autocomplete/autocomplete.component.ts b/packages/components/autocomplete/autocomplete.component.ts index 67cfbf5fc1..d061318d41 100644 --- a/packages/components/autocomplete/autocomplete.component.ts +++ b/packages/components/autocomplete/autocomplete.component.ts @@ -244,6 +244,23 @@ export class KbqAutocomplete implements AfterContentInit { }); } + /** + * Focuses the active option and scrolls the panel by as little as it takes to reveal it. + * + * The scroll is explicit on purpose. Focus performs one implicitly, 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 pass under the pointer, that made the panel unscrollable. + */ + scrollActiveOptionIntoView(): void { + const activeItem = this.keyManager.activeItem; + + if (!activeItem) return; + + activeItem.focus(); + + this.scrollbarViewport()?.scrollIntoViewNearest(activeItem.getHostElement()); + } + setScrollTop(scrollTop: number): void { const panel = this.panel(); diff --git a/packages/components/autocomplete/e2e.webkit.playwright-spec.ts b/packages/components/autocomplete/e2e.webkit.playwright-spec.ts new file mode 100644 index 0000000000..7c1de72cd4 --- /dev/null +++ b/packages/components/autocomplete/e2e.webkit.playwright-spec.ts @@ -0,0 +1,92 @@ +import { expect, Locator, Page, test } from '@playwright/test'; +import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions } from '../../e2e/utils'; + +/* -------------------------------------------------------------------------- */ +/* WebKit-only regression guard for panel scrolling (DS-3299). */ +/* */ +/* The panel used to scroll its 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. */ +/* */ +/* These assert on scroll offsets rather than screenshots, so they need no */ +/* baselines and no Docker. */ +/* -------------------------------------------------------------------------- */ + +test.use({ browserName: 'webkit' }); + +/** Waits two frames — where WebKit's deferred focus scroll used to land. */ +const settle = (page: Page) => + page.evaluate( + () => new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))) + ); + +const scrollTopOf = (locator: Locator) => locator.evaluate((el) => el.scrollTop); + +test.describe('KbqAutocomplete panel scrolling', () => { + const getContent = (page: Page) => page.locator('.kbq-autocomplete-panel__content'); + + test.beforeEach(async ({ page }) => { + await page.goto('/E2eAutocompleteScrollbar'); + await page.getByTestId('e2eAutocompleteInput').focus(); + await expect(getContent(page)).toBeVisible(); + }); + + test('never asks the browser to scroll an option into view on focus', async ({ page }) => { + await e2eRecordOptionFocusOptions(page); + + for (let i = 0; i < 5; i++) { + await page.keyboard.press('ArrowDown'); + } + + const preventScrollFlags = await e2eReadOptionFocusOptions(page); + + // A non-empty recording proves keyboard navigation really went through focus, so the + // assertion below cannot pass by never having been exercised. + expect(preventScrollFlags.length).toBeGreaterThan(0); + expect(preventScrollFlags).not.toContain(false); + }); + + test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { + const content = getContent(page); + + await content.hover(); + await page.mouse.wheel(0, 200); + await settle(page); + + const scrolled = await scrollTopOf(content); + + expect(scrolled).toBeGreaterThan(0); + + await settle(page); + + expect(await scrollTopOf(content)).toBe(scrolled); + }); + + test('brings the active option into view on keyboard navigation without scrolling the page', async ({ page }) => { + const content = getContent(page); + + for (let i = 0; i < 15; i++) { + await page.keyboard.press('ArrowDown'); + } + + await settle(page); + + expect(await scrollTopOf(content)).toBeGreaterThan(0); + + const activeIsVisible = await content.evaluate((el) => { + const active = el.querySelector('.kbq-option.kbq-active'); + + if (!active) return null; + + const panel = el.getBoundingClientRect(); + const option = active.getBoundingClientRect(); + + return option.top >= panel.top - 1 && option.bottom <= panel.bottom + 1; + }); + + expect(activeIsVisible).toBe(true); + expect(await page.evaluate(() => window.scrollY)).toBe(0); + }); +}); diff --git a/packages/components/core/option/option.spec.ts b/packages/components/core/option/option.spec.ts index ba07b1c5d7..5e69b8f9b3 100644 --- a/packages/components/core/option/option.spec.ts +++ b/packages/components/core/option/option.spec.ts @@ -34,6 +34,28 @@ describe('KbqOption component', () => { subscription.unsubscribe(); }); + it('should never let focus scroll the option into view, however it was activated', () => { + const fixture = TestBed.createComponent(OptionWithDisable); + + fixture.detectChanges(); + + const option: KbqOption = fixture.debugElement.query(By.directive(KbqOption)).componentInstance; + const host = option.getHostElement(); + const focusSpy = jest.spyOn(host, 'focus'); + + option.focus(); + + expect(focusSpy).toHaveBeenNthCalledWith(1, { preventScroll: true }); + + // Hovering once suppressed the implicit scroll for the next focus only. The panel scrolls + // explicitly now, so focus must never ask for a scroll — WebKit defers that scroll past the + // reader's own, which left the panel unscrollable while the pointer sat over the list. + host.dispatchEvent(new MouseEvent('mouseenter')); + option.focus(); + + expect(focusSpy).toHaveBeenNthCalledWith(2, { preventScroll: true }); + }); + it('should not emit to `onSelectionChange` if selecting an already-selected option', () => { const fixture = TestBed.createComponent(OptionWithDisable); diff --git a/packages/components/core/option/option.ts b/packages/components/core/option/option.ts index 274c00cd2e..06fde065be 100644 --- a/packages/components/core/option/option.ts +++ b/packages/components/core/option/option.ts @@ -258,15 +258,6 @@ export class KbqOption extends KbqOptionBase implements AfterViewChecked, OnDest private mostRecentViewValue = ''; - /** - * Flag that indicates whether the component is currently focused by a mouse interaction. - * - * When set to `true`, the component has focus resulting from a mouse click or - * other pointer event. It is automatically cleared when the component loses - * focus or if focus is obtained through keyboard navigation or programmatic means. - */ - private isFocusedByMouse: boolean = false; - ngAfterViewChecked() { // Since parent components could be using the option's label to display the selected values // (e.g. `kbq-select`) and they don't have a way of knowing if the option's label has changed @@ -310,13 +301,17 @@ export class KbqOption extends KbqOptionBase implements AfterViewChecked, OnDest } } + /** + * Moves keyboard focus to this option without scrolling it into view. The owning panel scrolls + * explicitly instead — see `KbqScrollbarViewport.scrollIntoViewNearest`. Relying on the implicit + * scroll that focus performs is not portable: WebKit defers it to a later rendering update, where + * it lands after — and undoes — any scrolling the reader did in the meantime. + */ focus(): void { const element = this.getHostElement(); if (typeof element.focus === 'function') { - element.focus({ preventScroll: this.isFocusedByMouse }); - - this.isFocusedByMouse = false; + element.focus({ preventScroll: true }); } } @@ -400,8 +395,6 @@ export class KbqOption extends KbqOptionBase implements AfterViewChecked, OnDest protected onMouseenter() { if (this.disabled) return; - this.isFocusedByMouse = true; - this.parent?.keyManager?.setActiveItem(this); } } @@ -435,30 +428,3 @@ export function countGroupLabelsBeforeOption( return 0; } - -/** - * Determines the position to which to scroll a panel in order for an option to be into view. - * @param optionIndex Index of the option to be scrolled into the view. - * @param optionHeight Height of the options. - * @param currentScrollPosition Current scroll position of the panel. - * @param panelHeight Height of the panel. - * @docs-private - */ -export function getOptionScrollPosition( - optionIndex: number, - optionHeight: number, - currentScrollPosition: number, - panelHeight: number -): number { - const optionOffset = optionIndex * optionHeight; - - if (optionOffset < currentScrollPosition) { - return optionOffset; - } - - if (optionOffset + optionHeight > currentScrollPosition + panelHeight) { - return Math.max(0, optionOffset - panelHeight + optionHeight); - } - - return currentScrollPosition; -} diff --git a/packages/components/scrollbar/scrollbar.spec.ts b/packages/components/scrollbar/scrollbar.spec.ts index bd3f5c2ba5..3ee3ced3a9 100644 --- a/packages/components/scrollbar/scrollbar.spec.ts +++ b/packages/components/scrollbar/scrollbar.spec.ts @@ -1136,6 +1136,122 @@ describe(KbqScrollbar.name, () => { expect(scrollToSpy).toHaveBeenCalledWith({ top: 175, left: 175 }); }); + + /** + * Places the scrollport and the target in one coordinate space, the way `scrollIntoViewNearest` + * reads them: the scrollport's rect sits at y=0, and the target's rect is the position it would + * paint at for the given `scrollTop` — i.e. its offset in the content, minus how far it is scrolled. + */ + const placeTarget = ( + viewport: HTMLElement, + target: HTMLElement, + content: { offsetTop: number; height: number; offsetLeft: number; width: number }, + scroll: { top: number; left: number } + ) => { + setRect(viewport, { top: 0, left: 0, width: 100, height: 100 }); + setRect(target, { + top: content.offsetTop - scroll.top, + left: content.offsetLeft - scroll.left, + width: content.width, + height: content.height + }); + }; + + it('scrollIntoViewNearest aligns a target above the scrollport to its start edge', () => { + const fixture = createComponent(TestScrollbarScrollTo); + const target = fixture.componentInstance.target().nativeElement; + + setMetrics(fixture.componentInstance.scrollbarEl().nativeElement, { + clientHeight: 100, + clientWidth: 100, + scrollTop: 200, + scrollLeft: 0 + }); + placeTarget( + fixture.componentInstance.scrollbarEl().nativeElement, + target, + { offsetTop: 50, height: 50, offsetLeft: 0, width: 50 }, + { top: 200, left: 0 } + ); + + const scrollToSpy = spyOnScrollTo(fixture); + + fixture.componentInstance.scrollbar().scrollIntoViewNearest(target); + + expect(scrollToSpy).toHaveBeenCalledWith({ top: 50, left: 0 }); + }); + + it('scrollIntoViewNearest aligns a target below the scrollport to its end edge', () => { + const fixture = createComponent(TestScrollbarScrollTo); + const target = fixture.componentInstance.target().nativeElement; + + setMetrics(fixture.componentInstance.scrollbarEl().nativeElement, { + clientHeight: 100, + clientWidth: 100, + scrollTop: 0, + scrollLeft: 0 + }); + placeTarget( + fixture.componentInstance.scrollbarEl().nativeElement, + target, + { offsetTop: 200, height: 50, offsetLeft: 0, width: 50 }, + { top: 0, left: 0 } + ); + + const scrollToSpy = spyOnScrollTo(fixture); + + fixture.componentInstance.scrollbar().scrollIntoViewNearest(target); + + expect(scrollToSpy).toHaveBeenCalledWith({ top: 150, left: 0 }); + }); + + it('scrollIntoViewNearest leaves a fully visible target where it is', () => { + const fixture = createComponent(TestScrollbarScrollTo); + const target = fixture.componentInstance.target().nativeElement; + + setMetrics(fixture.componentInstance.scrollbarEl().nativeElement, { + clientHeight: 100, + clientWidth: 100, + scrollTop: 100, + scrollLeft: 0 + }); + placeTarget( + fixture.componentInstance.scrollbarEl().nativeElement, + target, + { offsetTop: 120, height: 20, offsetLeft: 0, width: 50 }, + { top: 100, left: 0 } + ); + + const scrollToSpy = spyOnScrollTo(fixture); + + fixture.componentInstance.scrollbar().scrollIntoViewNearest(target); + + expect(scrollToSpy).not.toHaveBeenCalled(); + }); + + it('scrollIntoViewNearest brings the inline axis into view as well', () => { + const fixture = createComponent(TestScrollbarScrollTo); + const target = fixture.componentInstance.target().nativeElement; + + setMetrics(fixture.componentInstance.scrollbarEl().nativeElement, { + clientHeight: 100, + clientWidth: 100, + scrollTop: 0, + scrollLeft: 0 + }); + placeTarget( + fixture.componentInstance.scrollbarEl().nativeElement, + target, + { offsetTop: 0, height: 20, offsetLeft: 200, width: 50 }, + { top: 0, left: 0 } + ); + + const scrollToSpy = spyOnScrollTo(fixture); + + fixture.componentInstance.scrollbar().scrollIntoViewNearest(target); + + expect(scrollToSpy).toHaveBeenCalledWith({ top: 0, left: 150 }); + }); }); describe('scrollChanges', () => { diff --git a/packages/components/scrollbar/scrollbar.ts b/packages/components/scrollbar/scrollbar.ts index 09b65c0c97..575ae48343 100644 --- a/packages/components/scrollbar/scrollbar.ts +++ b/packages/components/scrollbar/scrollbar.ts @@ -127,6 +127,21 @@ function getElementOffset(ancestor: HTMLElement, element: HTMLElement): { offset return { offsetTop, offsetLeft }; } +// The offset that brings `[offset, offset + size]` inside `[current, current + viewport]` while moving as +// little as possible: past the start edge it aligns to the start, past the end edge to the end, and an +// already visible range is left where it is. +function nearestEdgeScrollOffset(offset: number, size: number, current: number, viewport: number): number { + if (offset < current) { + return offset; + } + + if (offset + size > current + viewport) { + return Math.max(0, offset + size - viewport); + } + + return current; +} + /** * How the scrollbar is presented: * - `hover` — track appears on pointer hover or while scrolling (default); @@ -372,6 +387,38 @@ export class KbqScrollbarViewport { }); } + /** + * Scrolls `target` just far enough to bring it inside the viewport, leaving an already visible target + * where it is. Unlike {@link scrollIntoView}, which centers its target, this keeps a list from jumping + * under the reader on every step of keyboard navigation. + * + * Scrolls this viewport only — an ancestor scroll container is never moved, which + * `Element.scrollIntoView({ block: 'nearest' })` does not promise inside an overlay. + */ + scrollIntoViewNearest(target: HTMLElement, behavior?: ScrollBehavior): void { + const element = this.getNativeElement(); + const { scrollTop, scrollLeft, clientHeight, clientWidth, clientTop, clientLeft } = element; + + // Measured from rects rather than `getElementOffset`, 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` and an `offsetParent` walk never reaches the scrollport. + // Rects are unaffected by that, and subtracting the border edge puts them in scroll coordinates. + const viewportRect = element.getBoundingClientRect(); + const targetRect = target.getBoundingClientRect(); + + const offsetTop = scrollTop + targetRect.top - viewportRect.top - clientTop; + const offsetLeft = scrollLeft + targetRect.left - viewportRect.left - clientLeft; + + const top = nearestEdgeScrollOffset(offsetTop, targetRect.height, scrollTop, clientHeight); + const left = nearestEdgeScrollOffset(offsetLeft, targetRect.width, scrollLeft, clientWidth); + + if (top === scrollTop && left === scrollLeft) { + return; + } + + this.scrollTo({ top, left, behavior }); + } + /** Scrolls `target` to the center of the viewport. */ scrollIntoView(target: HTMLElement, behavior?: ScrollBehavior): void { const { offsetHeight, offsetWidth } = target; @@ -803,6 +850,11 @@ export class KbqScrollbar { this.viewport.scrollToElement(target, options); } + /** Scrolls `target` into view by the shortest distance — see {@link KbqScrollbarViewport.scrollIntoViewNearest}. */ + scrollIntoViewNearest(target: HTMLElement, behavior?: ScrollBehavior): void { + this.viewport.scrollIntoViewNearest(target, behavior); + } + /** Scrolls `target` to the center of the viewport. */ scrollIntoView(target: HTMLElement, behavior?: ScrollBehavior): void { this.viewport.scrollIntoView(target, behavior); diff --git a/packages/components/select/e2e.webkit.playwright-spec.ts b/packages/components/select/e2e.webkit.playwright-spec.ts new file mode 100644 index 0000000000..3ddbcfb719 --- /dev/null +++ b/packages/components/select/e2e.webkit.playwright-spec.ts @@ -0,0 +1,135 @@ +import { expect, Locator, Page, test } from '@playwright/test'; +import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions } from '../../e2e/utils'; + +/* -------------------------------------------------------------------------- */ +/* WebKit-only regression guard for panel scrolling (DS-3299). */ +/* */ +/* The panel used to scroll its 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 panel could not be scrolled at all in Safari. */ +/* */ +/* These assert on scroll offsets rather than screenshots, so they need no */ +/* baselines and no Docker. */ +/* -------------------------------------------------------------------------- */ + +test.use({ browserName: 'webkit' }); + +/** Waits two frames — where WebKit's deferred focus scroll used to land. */ +const settle = (page: Page) => + page.evaluate( + () => new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))) + ); + +const scrollTopOf = (locator: Locator) => locator.evaluate((el) => el.scrollTop); + +test.describe('KbqSelect panel scrolling', () => { + const getSelect = (page: Page) => page.getByTestId('e2eSelect'); + const getContent = (page: Page) => page.locator('.kbq-select__content'); + + test.beforeEach(async ({ page }) => { + await page.goto('/E2eSelectScrollbar'); + await getSelect(page).click(); + await expect(getContent(page)).toBeVisible(); + }); + + test('never asks the browser to scroll an option into view on focus', async ({ page }) => { + await e2eRecordOptionFocusOptions(page); + + for (let i = 0; i < 5; i++) { + await page.keyboard.press('ArrowDown'); + } + + const preventScrollFlags = await e2eReadOptionFocusOptions(page); + + // A non-empty recording proves keyboard navigation really went through focus, so the + // assertion below cannot pass by never having been exercised. + expect(preventScrollFlags.length).toBeGreaterThan(0); + expect(preventScrollFlags).not.toContain(false); + }); + + test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { + const content = getContent(page); + + await content.hover(); + await page.mouse.wheel(0, 200); + await settle(page); + + const scrolled = await scrollTopOf(content); + + expect(scrolled).toBeGreaterThan(0); + + await settle(page); + + expect(await scrollTopOf(content)).toBe(scrolled); + }); + + test('brings the active option into view on keyboard navigation without scrolling the page', async ({ page }) => { + const content = getContent(page); + + for (let i = 0; i < 15; i++) { + await page.keyboard.press('ArrowDown'); + } + + await settle(page); + + expect(await scrollTopOf(content)).toBeGreaterThan(0); + + // The active option is fully inside the scrollport, not merely somewhere in the list. + const activeIsVisible = await content.evaluate((el) => { + const active = el.querySelector('.kbq-option.kbq-active'); + + if (!active) return null; + + const panel = el.getBoundingClientRect(); + const option = active.getBoundingClientRect(); + + return option.top >= panel.top - 1 && option.bottom <= panel.bottom + 1; + }); + + expect(activeIsVisible).toBe(true); + expect(await page.evaluate(() => window.scrollY)).toBe(0); + }); +}); + +test.describe('KbqSelect panel scrolling under virtual scroll', () => { + // Virtual scroll swaps in a CDK viewport that translates its content instead of laying it out at the + // scroll offset, so the panel drives it through the CDK's own API rather than by element offsets. + const getViewport = (page: Page) => page.locator('cdk-virtual-scroll-viewport'); + + test.beforeEach(async ({ page }) => { + await page.goto('/E2eVirtualScrollSelectScrollbar'); + await page.getByTestId('e2eSelect').click(); + await expect(getViewport(page)).toBeVisible(); + }); + + test('follows the active option with the keyboard', async ({ page }) => { + const viewport = getViewport(page); + const initial = await scrollTopOf(viewport); + + for (let i = 0; i < 20; i++) { + await page.keyboard.press('ArrowDown'); + } + + await settle(page); + + // Twenty options is past the panel's cap, so the viewport has to have followed the active one. + expect(await scrollTopOf(viewport)).toBeGreaterThan(initial); + expect(await page.evaluate(() => window.scrollY)).toBe(0); + }); + + test('never asks the browser to scroll an option into view on focus', async ({ page }) => { + await e2eRecordOptionFocusOptions(page); + + for (let i = 0; i < 5; i++) { + await page.keyboard.press('ArrowDown'); + } + + const preventScrollFlags = await e2eReadOptionFocusOptions(page); + + expect(preventScrollFlags.length).toBeGreaterThan(0); + expect(preventScrollFlags).not.toContain(false); + }); +}); diff --git a/packages/components/select/select.component.ts b/packages/components/select/select.component.ts index 4d3af3eb7f..3e47569c4a 100644 --- a/packages/components/select/select.component.ts +++ b/packages/components/select/select.component.ts @@ -1075,7 +1075,7 @@ export class KbqSelect /** Subscription to the options panel scroll event used by `scrolledToBottom`. */ private scrollSubscription = Subscription.EMPTY; - /** The scroll position of the overlay panel, calculated to center the selected option. */ + /** The scroll offset the panel is restored to when it attaches — the list always opens at the top. */ private scrollTop = 0; /** Unique id for this input. Auto-incremented for each instance. */ @@ -2251,9 +2251,26 @@ export class KbqSelect } } - /** Scrolls the active option into view. */ + /** + * Focuses the active option and scrolls the panel by as little as it takes to reveal it. + * + * The scroll is explicit on purpose. Focus performs one implicitly, 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 pass under the pointer, that made the panel unscrollable. + */ private scrollActiveOptionIntoView(): void { - this.keyManager.activeItem?.focus(); + const activeItem = this.keyManager.activeItem; + + if (!activeItem) return; + + activeItem.focus(); + + this.activeScrollbarViewport?.scrollIntoViewNearest(activeItem.getHostElement()); + } + + /** The panel's real scrolling element: virtual scroll projects its own viewport in place of ours. */ + private get activeScrollbarViewport(): KbqScrollbarViewport | undefined { + return this.projectedScrollbarViewport() ?? this.scrollbarViewport(); } /** Comparison function to specify which option is displayed. Defaults to object equality. */ diff --git a/packages/components/tree-select/e2e.webkit.playwright-spec.ts b/packages/components/tree-select/e2e.webkit.playwright-spec.ts new file mode 100644 index 0000000000..0ca2fbae95 --- /dev/null +++ b/packages/components/tree-select/e2e.webkit.playwright-spec.ts @@ -0,0 +1,93 @@ +import { expect, Locator, Page, test } from '@playwright/test'; +import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions } from '../../e2e/utils'; + +/* -------------------------------------------------------------------------- */ +/* WebKit-only regression guard for panel scrolling (DS-3299). */ +/* */ +/* The panel used to scroll its active node 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. Tree-select was the worst affected: it focused with no origin */ +/* at all, so even hovering a node queued one. */ +/* */ +/* These assert on scroll offsets rather than screenshots, so they need no */ +/* baselines and no Docker. */ +/* -------------------------------------------------------------------------- */ + +test.use({ browserName: 'webkit' }); + +/** Waits two frames — where WebKit's deferred focus scroll used to land. */ +const settle = (page: Page) => + page.evaluate( + () => new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))) + ); + +const scrollTopOf = (locator: Locator) => locator.evaluate((el) => el.scrollTop); + +test.describe('KbqTreeSelect panel scrolling', () => { + const getContent = (page: Page) => page.locator('.kbq-tree-select__content'); + + test.beforeEach(async ({ page }) => { + await page.goto('/E2eTreeSelectScrollbar'); + await page.getByTestId('e2eTreeSelect').click(); + await expect(getContent(page)).toBeVisible(); + }); + + test('never asks the browser to scroll a node into view on focus', async ({ page }) => { + await e2eRecordOptionFocusOptions(page); + + for (let i = 0; i < 5; i++) { + await page.keyboard.press('ArrowDown'); + } + + const preventScrollFlags = await e2eReadOptionFocusOptions(page); + + // A non-empty recording proves keyboard navigation really went through focus, so the + // assertion below cannot pass by never having been exercised. + expect(preventScrollFlags.length).toBeGreaterThan(0); + expect(preventScrollFlags).not.toContain(false); + }); + + test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { + const content = getContent(page); + + await content.hover(); + await page.mouse.wheel(0, 200); + await settle(page); + + const scrolled = await scrollTopOf(content); + + expect(scrolled).toBeGreaterThan(0); + + await settle(page); + + expect(await scrollTopOf(content)).toBe(scrolled); + }); + + test('brings the active node into view on keyboard navigation without scrolling the page', async ({ page }) => { + const content = getContent(page); + + for (let i = 0; i < 15; i++) { + await page.keyboard.press('ArrowDown'); + } + + await settle(page); + + expect(await scrollTopOf(content)).toBeGreaterThan(0); + + const activeIsVisible = await content.evaluate((el) => { + const active = el.querySelector('.kbq-tree-option.kbq-active, .kbq-tree-option:focus'); + + if (!active) return null; + + const panel = el.getBoundingClientRect(); + const option = active.getBoundingClientRect(); + + return option.top >= panel.top - 1 && option.bottom <= panel.bottom + 1; + }); + + expect(activeIsVisible).toBe(true); + expect(await page.evaluate(() => window.scrollY)).toBe(0); + }); +}); diff --git a/packages/components/tree-select/tree-select.component.ts b/packages/components/tree-select/tree-select.component.ts index fd9dfc6892..51cf964da8 100644 --- a/packages/components/tree-select/tree-select.component.ts +++ b/packages/components/tree-select/tree-select.component.ts @@ -1233,8 +1233,8 @@ export class KbqTreeSelect this.overlayDir.positionChange.pipe(take(1)).subscribe(() => { this.changeDetectorRef.detectChanges(); this.setOverlayPosition(); - // `panel` is guaranteed to exist here: this callback only fires once the overlay has attached. - this.panel()!.nativeElement.scrollTop = this.scrollTop; + // The panel itself is an `overflow: hidden` box; the option list is what scrolls. + this.optionsContainer()!.nativeElement.scrollTop = this.scrollTop; this.tree()!.updateScrollSize(); // Deliberately out of this frame — see `reanchorPanel`. A microtask still lands before paint. @@ -1654,9 +1654,21 @@ export class KbqTreeSelect } } - /** Scrolls the active option into view. */ + /** + * Focuses the active option and scrolls the panel by as little as it takes to reveal it. + * + * The scroll is explicit on purpose. Focus performs one implicitly, 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 pass under the pointer, that made the panel unscrollable. + */ private scrollActiveOptionIntoView() { - this.tree()!.keyManager.activeItem?.focus(); + const activeItem = this.tree()!.keyManager.activeItem; + + if (!activeItem) return; + + activeItem.focus(); + + this.scrollbarViewport()?.scrollIntoViewNearest(activeItem.getHostElement()); } private subscribeOnSearchChanges() { diff --git a/packages/components/tree/tree-option.component.ts b/packages/components/tree/tree-option.component.ts index 5970ea0310..3e1d22af9d 100644 --- a/packages/components/tree/tree-option.component.ts +++ b/packages/components/tree/tree-option.component.ts @@ -323,7 +323,9 @@ export class KbqTreeOption extends KbqTreeNode implements AfterCo focus(focusOrigin?: FocusOrigin) { if (focusOrigin === 'program' || this.disabled || this.actionButton()?.hasFocus) return; - this.elementRef.nativeElement.focus({ preventScroll: focusOrigin === 'mouse' }); + // Never scroll from focus: WebKit defers that scroll to a later rendering update, where it lands + // after — and undoes — the reader's own scrolling. Panels scroll the active node explicitly. + this.elementRef.nativeElement.focus({ preventScroll: true }); if (!this.hasFocus) { this.onFocus.next({ option: this }); diff --git a/packages/e2e/utils/focus-scroll.ts b/packages/e2e/utils/focus-scroll.ts new file mode 100644 index 0000000000..edad4332bf --- /dev/null +++ b/packages/e2e/utils/focus-scroll.ts @@ -0,0 +1,32 @@ +import { Page } from '@playwright/test'; + +/** + * Records the `FocusOptions` every option-like element is focused with, so a spec can assert that a panel + * never leans on the scroll `HTMLElement.focus()` performs implicitly. + * + * That implicit scroll is not portable: Blink runs it synchronously, while WebKit defers it to a later + * rendering update, where it lands after — and undoes — whatever the reader scrolled in the meantime. + * Panels are expected to focus with `preventScroll: true` and scroll explicitly instead. + * + * Call before the interaction, then read the flags back with {@link e2eReadOptionFocusOptions}. + */ +export const e2eRecordOptionFocusOptions = (page: Page): Promise => + page.evaluate(() => { + const recorded: boolean[] = []; + + (window as unknown as { __kbqOptionFocusOptions: boolean[] }).__kbqOptionFocusOptions = recorded; + + const originalFocus = HTMLElement.prototype.focus; + + HTMLElement.prototype.focus = function (this: HTMLElement, options?: FocusOptions) { + if (this.matches('.kbq-option, .kbq-tree-option')) { + recorded.push(options?.preventScroll === true); + } + + return originalFocus.call(this, options); + }; + }); + +/** Reads back what {@link e2eRecordOptionFocusOptions} captured: one `preventScroll` flag per focus call. */ +export const e2eReadOptionFocusOptions = (page: Page): Promise => + page.evaluate(() => (window as unknown as { __kbqOptionFocusOptions?: boolean[] }).__kbqOptionFocusOptions ?? []); diff --git a/packages/e2e/utils/index.ts b/packages/e2e/utils/index.ts index a794db1c18..9b7ab721e0 100644 --- a/packages/e2e/utils/index.ts +++ b/packages/e2e/utils/index.ts @@ -1,4 +1,5 @@ export * from './autofill'; +export * from './focus-scroll'; export * from './overflow-shadow'; export * from './scrollbar'; export * from './theme'; diff --git a/tools/public_api_guard/components/autocomplete.api.md b/tools/public_api_guard/components/autocomplete.api.md index 78d2eee0c1..ae2acce86f 100644 --- a/tools/public_api_guard/components/autocomplete.api.md +++ b/tools/public_api_guard/components/autocomplete.api.md @@ -28,9 +28,6 @@ import { ScrollDispatcher } from '@angular/cdk/overlay'; import { ScrollStrategy } from '@angular/cdk/overlay'; import { TemplateRef } from '@angular/core'; -// @public -export const AUTOCOMPLETE_PANEL_HEIGHT = 256; - // @public export function getKbqAutocompleteMissingPanelError(): Error; @@ -92,6 +89,7 @@ export class KbqAutocomplete implements AfterContentInit { readonly panelMaxWidth: _angular_core.InputSignalWithTransform; readonly panelMinWidth: _angular_core.InputSignalWithTransform; readonly panelWidth: _angular_core.InputSignal; + scrollActiveOptionIntoView(): void; // (undocumented) setScrollTop(scrollTop: number): void; // (undocumented) diff --git a/tools/public_api_guard/components/core.api.md b/tools/public_api_guard/components/core.api.md index 7f587f1a9f..c1fb108c54 100644 --- a/tools/public_api_guard/components/core.api.md +++ b/tools/public_api_guard/components/core.api.md @@ -1030,9 +1030,6 @@ export function getKbqSelectNonFunctionValueError(): Error; // @public (undocumented) export const getNodesWithoutComments: (nodes: NodeList) => Node[]; -// @public -export function getOptionScrollPosition(optionIndex: number, optionHeight: number, currentScrollPosition: number, panelHeight: number): number; - // @public export function getSafeTriangleVertices(origin: KbqPoint, targetRect: DOMRect): KbqTriangle; @@ -3464,7 +3461,6 @@ export class KbqOption extends KbqOptionBase implements AfterViewChecked, OnDest // (undocumented) get disabled(): any; set disabled(value: any); - // (undocumented) focus(): void; getHeight(): number; // (undocumented) diff --git a/tools/public_api_guard/components/scrollbar.api.md b/tools/public_api_guard/components/scrollbar.api.md index cb7291a5a8..63ae6f2f17 100644 --- a/tools/public_api_guard/components/scrollbar.api.md +++ b/tools/public_api_guard/components/scrollbar.api.md @@ -33,6 +33,7 @@ export class KbqScrollbar { readonly scrollChanges: Observable; scrollEnd(behavior?: ScrollBehavior): void; scrollIntoView(target: HTMLElement, behavior?: ScrollBehavior): void; + scrollIntoViewNearest(target: HTMLElement, behavior?: ScrollBehavior): void; scrollStart(behavior?: ScrollBehavior): void; scrollTo(options: KbqScrollbarScrollToOptions): void; scrollToBottom(behavior?: ScrollBehavior): void; @@ -79,6 +80,7 @@ export class KbqScrollbarViewport { readonly scrollChanges: Observable; scrollEnd(behavior?: ScrollBehavior): void; scrollIntoView(target: HTMLElement, behavior?: ScrollBehavior): void; + scrollIntoViewNearest(target: HTMLElement, behavior?: ScrollBehavior): void; scrollStart(behavior?: ScrollBehavior): void; scrollTo(options: KbqScrollbarScrollToOptions): void; scrollToBottom(behavior?: ScrollBehavior): void; From 9a59bf892e3a579d66f6cd3df455b34b1a57e341 Mon Sep 17 00:00:00 2001 From: lskramarov Date: Tue, 1 Sep 2026 21:50:21 +0300 Subject: [PATCH 2/7] fix(dropdown): panel scrolling in Safari (#DS-3299) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../dropdown/dropdown-item.component.ts | 13 ++- .../components/dropdown/dropdown.component.ts | 19 ++++ .../dropdown/e2e.webkit.playwright-spec.ts | 103 ++++++++++++++++++ packages/e2e/utils/focus-scroll.ts | 2 +- 4 files changed, 133 insertions(+), 4 deletions(-) create mode 100644 packages/components/dropdown/e2e.webkit.playwright-spec.ts diff --git a/packages/components/dropdown/dropdown-item.component.ts b/packages/components/dropdown/dropdown-item.component.ts index 751f19e3b3..feae43c7f9 100644 --- a/packages/components/dropdown/dropdown-item.component.ts +++ b/packages/components/dropdown/dropdown-item.component.ts @@ -131,14 +131,21 @@ export class KbqDropdownItem implements KbqTitleTextRef, KbqDropdownItemActionHo this.getHostElement().classList.remove('cdk-keyboard-focused'); } - /** Focuses the dropdown item. */ + /** + * Focuses the dropdown item without scrolling it into view. The panel scrolls the active item + * explicitly instead — see `KbqDropdown`. Relying on the implicit scroll that focus performs is not + * portable: WebKit defers it to a later rendering update, where it lands after — and undoes — any + * scrolling the reader did in the meantime. + */ focus(origin?: FocusOrigin, options?: FocusOptions): void { if (this.disabled) return; + const focusOptions: FocusOptions = { ...options, preventScroll: true }; + if (this.focusMonitor && origin) { - this.focusMonitor.focusVia(this.getHostElement(), origin, options); + this.focusMonitor.focusVia(this.getHostElement(), origin, focusOptions); } else { - this.getHostElement().focus(options); + this.getHostElement().focus(focusOptions); } this.focused.next(this); diff --git a/packages/components/dropdown/dropdown.component.ts b/packages/components/dropdown/dropdown.component.ts index 4af9d733d1..3eeca2eb0a 100644 --- a/packages/components/dropdown/dropdown.component.ts +++ b/packages/components/dropdown/dropdown.component.ts @@ -330,6 +330,9 @@ export class KbqDropdown implements AfterContentInit, KbqDropdownPanel, OnInit, /** Subscription to tab events on the dropdown panel */ private tabSubscription = Subscription.EMPTY; + /** Subscription keeping the panel scrolled to the active item. */ + private activeItemSubscription = Subscription.EMPTY; + /** Cleans up the safe-area `mousemove` listener. `null` when no safe area is being tracked. */ private safeAreaCleanup: (() => void) | null = null; @@ -367,6 +370,12 @@ export class KbqDropdown implements AfterContentInit, KbqDropdownPanel, OnInit, this.tabSubscription = this.keyManager.tabOut.subscribe(() => this.closed.emit('tab')); + // `FocusKeyManager` focuses the item it activates, but focus is not allowed to scroll (see + // `KbqDropdownItem.focus`), so the panel follows the active item itself. Only `setActiveItem` + // emits here — 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. + this.activeItemSubscription = this.keyManager.change.subscribe(() => this.scrollActiveItemIntoView()); + // If a user manually (programmatically) focuses a menu item, we need to reflect that focus // change back to the key manager. Note that we don't need to unsubscribe here because focused // is internal and we know that it gets completed on destroy. @@ -386,6 +395,7 @@ export class KbqDropdown implements AfterContentInit, KbqDropdownPanel, OnInit, ngOnDestroy() { this.directDescendantItems.destroy(); this.tabSubscription.unsubscribe(); + this.activeItemSubscription.unsubscribe(); this.closed.complete(); this.deactivateSafeArea(); this.panelReached.complete(); @@ -585,6 +595,15 @@ export class KbqDropdown implements AfterContentInit, KbqDropdownPanel, OnInit, } } + /** Scrolls the panel by as little as it takes to reveal the active item, leaving a visible one alone. */ + private scrollActiveItemIntoView(): void { + const activeItem = this.keyManager.activeItem; + + if (!activeItem) return; + + this.scrollbarViewport()?.scrollIntoViewNearest(activeItem.getHostElement()); + } + /** Moves DOM focus onto the dropdown panel so that keydown events keep being handled. */ private focusPanel(): void { // The panel is rendered into the overlay through a `TemplatePortal`, so it can't be diff --git a/packages/components/dropdown/e2e.webkit.playwright-spec.ts b/packages/components/dropdown/e2e.webkit.playwright-spec.ts new file mode 100644 index 0000000000..9f30f75125 --- /dev/null +++ b/packages/components/dropdown/e2e.webkit.playwright-spec.ts @@ -0,0 +1,103 @@ +import { expect, Locator, Page, test } from '@playwright/test'; +import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions } from '../../e2e/utils'; + +/* -------------------------------------------------------------------------- */ +/* WebKit-only regression guard for panel scrolling (DS-3299). */ +/* */ +/* `FocusKeyManager` focuses the item it activates, and the panel used to */ +/* lean 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. Hovering re-focuses items as they pass under the */ +/* pointer, so the panel could not be scrolled at all in Safari. */ +/* */ +/* These assert on scroll offsets rather than screenshots, so they need no */ +/* baselines and no Docker. */ +/* -------------------------------------------------------------------------- */ + +test.use({ browserName: 'webkit' }); + +/** Waits two frames — where WebKit's deferred focus scroll used to land. */ +const settle = (page: Page) => + page.evaluate( + () => new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))) + ); + +const scrollTopOf = (locator: Locator) => locator.evaluate((el) => el.scrollTop); + +test.describe('KbqDropdown panel scrolling', () => { + const getPanel = (page: Page) => page.locator('.kbq-dropdown__panel'); + + test.beforeEach(async ({ page }) => { + await page.goto('/E2eDropdownScrollbar'); + await page.getByTestId('e2eDropdownScrollbarTrigger').click(); + await expect(getPanel(page)).toBeVisible(); + // The panel is measured below, so wait for its items rather than just for the box itself. + await expect(getPanel(page).locator('.kbq-dropdown-item').first()).toBeVisible(); + }); + + test('has a panel that actually overflows', async ({ page }) => { + // Guards the premise of every other test here: without overflow they would all pass vacuously. + await expect.poll(() => getPanel(page).evaluate((el) => el.scrollHeight > el.clientHeight)).toBe(true); + }); + + test('never asks the browser to scroll an item into view on focus', async ({ page }) => { + await e2eRecordOptionFocusOptions(page); + + for (let i = 0; i < 5; i++) { + await page.keyboard.press('ArrowDown'); + } + + const preventScrollFlags = await e2eReadOptionFocusOptions(page); + + // A non-empty recording proves keyboard navigation really went through focus, so the + // assertion below cannot pass by never having been exercised. + expect(preventScrollFlags.length).toBeGreaterThan(0); + expect(preventScrollFlags).not.toContain(false); + }); + + test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { + const panel = getPanel(page); + + await panel.hover(); + await page.mouse.wheel(0, 200); + await settle(page); + + const scrolled = await scrollTopOf(panel); + + expect(scrolled).toBeGreaterThan(0); + + await settle(page); + + expect(await scrollTopOf(panel)).toBe(scrolled); + }); + + test('brings the active item into view on keyboard navigation without scrolling the page', async ({ page }) => { + const panel = getPanel(page); + const itemCount = await panel.locator('.kbq-dropdown-item').count(); + + // Roughly twenty items fit, so walking to the last one is what forces the panel to scroll. + // Navigation does not wrap here (`navigationWithWrap` defaults to false), so this lands on the end. + for (let i = 0; i < itemCount; i++) { + await page.keyboard.press('ArrowDown'); + } + + await settle(page); + + expect(await scrollTopOf(panel)).toBeGreaterThan(0); + + const activeIsVisible = await panel.evaluate((el) => { + const active = el.querySelector('.kbq-dropdown-item:focus'); + + if (!active) return null; + + const panelRect = el.getBoundingClientRect(); + const itemRect = active.getBoundingClientRect(); + + return itemRect.top >= panelRect.top - 1 && itemRect.bottom <= panelRect.bottom + 1; + }); + + expect(activeIsVisible).toBe(true); + expect(await page.evaluate(() => window.scrollY)).toBe(0); + }); +}); diff --git a/packages/e2e/utils/focus-scroll.ts b/packages/e2e/utils/focus-scroll.ts index edad4332bf..1d00e524d0 100644 --- a/packages/e2e/utils/focus-scroll.ts +++ b/packages/e2e/utils/focus-scroll.ts @@ -19,7 +19,7 @@ export const e2eRecordOptionFocusOptions = (page: Page): Promise => const originalFocus = HTMLElement.prototype.focus; HTMLElement.prototype.focus = function (this: HTMLElement, options?: FocusOptions) { - if (this.matches('.kbq-option, .kbq-tree-option')) { + if (this.matches('.kbq-option, .kbq-tree-option, .kbq-dropdown-item')) { recorded.push(options?.preventScroll === true); } From 46e416177433250dfdfbced2d43914e3cb80573a Mon Sep 17 00:00:00 2001 From: lskramarov Date: Tue, 1 Sep 2026 22:20:09 +0300 Subject: [PATCH 3/7] fix(components): reveal the focused option from the item itself (#DS-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. --- .../autocomplete/autocomplete.component.ts | 16 +- .../e2e.webkit.playwright-spec.ts | 117 ++++++++------ .../components/core/option/option.spec.ts | 22 ++- packages/components/core/option/option.ts | 32 +++- .../dropdown/dropdown-item.component.ts | 21 ++- .../components/dropdown/dropdown.component.ts | 19 --- .../dropdown/e2e.webkit.playwright-spec.ts | 124 ++++++++------- .../components/scrollbar/scrollbar.spec.ts | 116 -------------- packages/components/scrollbar/scrollbar.ts | 52 ------ .../select/e2e.webkit.playwright-spec.ts | 150 ++++++++---------- .../components/select/select.component.ts | 21 +-- .../tree-select/e2e.webkit.playwright-spec.ts | 118 ++++++++------ .../tree-select/tree-select.component.ts | 16 +- .../components/tree/tree-option.component.ts | 15 +- packages/e2e/utils/focus-scroll.ts | 52 +++++- .../components/scrollbar.api.md | 2 - 16 files changed, 379 insertions(+), 514 deletions(-) diff --git a/packages/components/autocomplete/autocomplete.component.ts b/packages/components/autocomplete/autocomplete.component.ts index d061318d41..417d26044e 100644 --- a/packages/components/autocomplete/autocomplete.component.ts +++ b/packages/components/autocomplete/autocomplete.component.ts @@ -244,21 +244,9 @@ export class KbqAutocomplete implements AfterContentInit { }); } - /** - * Focuses the active option and scrolls the panel by as little as it takes to reveal it. - * - * The scroll is explicit on purpose. Focus performs one implicitly, 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 pass under the pointer, that made the panel unscrollable. - */ + /** Focuses the active option, which reveals it — see `KbqOption.focus`. */ scrollActiveOptionIntoView(): void { - const activeItem = this.keyManager.activeItem; - - if (!activeItem) return; - - activeItem.focus(); - - this.scrollbarViewport()?.scrollIntoViewNearest(activeItem.getHostElement()); + this.keyManager.activeItem?.focus(); } setScrollTop(scrollTop: number): void { diff --git a/packages/components/autocomplete/e2e.webkit.playwright-spec.ts b/packages/components/autocomplete/e2e.webkit.playwright-spec.ts index 7c1de72cd4..718c3b0655 100644 --- a/packages/components/autocomplete/e2e.webkit.playwright-spec.ts +++ b/packages/components/autocomplete/e2e.webkit.playwright-spec.ts @@ -1,92 +1,109 @@ -import { expect, Locator, Page, test } from '@playwright/test'; -import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions } from '../../e2e/utils'; - -/* -------------------------------------------------------------------------- */ -/* WebKit-only regression guard for panel scrolling (DS-3299). */ -/* */ -/* The panel used to scroll its 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. */ -/* */ -/* These assert on scroll offsets rather than screenshots, so they need no */ -/* baselines and no Docker. */ -/* -------------------------------------------------------------------------- */ +import { expect, Page, test } from '@playwright/test'; +import { + e2eIsFullyInView, + e2eReadOptionFocusOptions, + e2eRecordOptionFocusOptions, + e2eScrollTopOf, + e2eSettleFrames +} from '../../e2e/utils'; + +/* + * WebKit-only guard for panel scrolling (DS-3299). + * + * Focus scrolls its target into view implicitly. Blink does that synchronously; WebKit defers it to a + * later rendering update, where it lands after — and undoes — whatever the reader scrolled in between. + * Components therefore focus with `preventScroll` and reveal the option themselves. + * + * These assert on scroll offsets rather than screenshots, so they need no baselines and no Docker. + */ test.use({ browserName: 'webkit' }); -/** Waits two frames — where WebKit's deferred focus scroll used to land. */ -const settle = (page: Page) => - page.evaluate( - () => new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))) - ); - -const scrollTopOf = (locator: Locator) => locator.evaluate((el) => el.scrollTop); - test.describe('KbqAutocomplete panel scrolling', () => { - const getContent = (page: Page) => page.locator('.kbq-autocomplete-panel__content'); + const getPort = (page: Page) => page.locator('.kbq-autocomplete-panel__content'); test.beforeEach(async ({ page }) => { + // Installed before navigation so the focus calls the panel makes while opening are recorded too. + await e2eRecordOptionFocusOptions(page); await page.goto('/E2eAutocompleteScrollbar'); await page.getByTestId('e2eAutocompleteInput').focus(); - await expect(getContent(page)).toBeVisible(); + await expect(getPort(page)).toBeVisible(); + await expect(getPort(page).locator('.kbq-option').first()).toBeVisible(); }); - test('never asks the browser to scroll an option into view on focus', async ({ page }) => { - await e2eRecordOptionFocusOptions(page); + test('has a panel that actually overflows', async ({ page }) => { + // Guards the premise of the other tests: without overflow they would all pass vacuously. + await expect.poll(() => getPort(page).evaluate((el) => el.scrollHeight > el.clientHeight)).toBe(true); + }); + test('never asks the browser to scroll on focus, including while opening', async ({ page }) => { for (let i = 0; i < 5; i++) { await page.keyboard.press('ArrowDown'); } const preventScrollFlags = await e2eReadOptionFocusOptions(page); - // A non-empty recording proves keyboard navigation really went through focus, so the - // assertion below cannot pass by never having been exercised. + // A non-empty recording proves focus was exercised, so the assertion cannot pass vacuously. expect(preventScrollFlags.length).toBeGreaterThan(0); expect(preventScrollFlags).not.toContain(false); }); test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { - const content = getContent(page); + const port = getPort(page); - await content.hover(); + await port.hover(); await page.mouse.wheel(0, 200); - await settle(page); + await e2eSettleFrames(page); - const scrolled = await scrollTopOf(content); + const scrolled = await e2eScrollTopOf(port); expect(scrolled).toBeGreaterThan(0); - await settle(page); + await e2eSettleFrames(page); - expect(await scrollTopOf(content)).toBe(scrolled); + expect(await e2eScrollTopOf(port)).toBe(scrolled); }); - test('brings the active option into view on keyboard navigation without scrolling the page', async ({ page }) => { - const content = getContent(page); + test('does not move the list when the pointer lands on a partially clipped option', async ({ page }) => { + const port = getPort(page); - for (let i = 0; i < 15; i++) { - await page.keyboard.press('ArrowDown'); - } + const offsets = await port.evaluate(async (element, sel) => { + // Land on a fractional offset so a row straddles the top edge of the scrollport. + element.scrollTop = 50; + await new Promise((resolve) => requestAnimationFrame(resolve)); - await settle(page); + const before = element.scrollTop; + const port = element.getBoundingClientRect(); + const clipped = [...element.querySelectorAll(sel)].find((item) => { + const box = item.getBoundingClientRect(); - expect(await scrollTopOf(content)).toBeGreaterThan(0); + return box.top < port.top && box.bottom > port.top; + }); - const activeIsVisible = await content.evaluate((el) => { - const active = el.querySelector('.kbq-option.kbq-active'); + if (!clipped) return null; - if (!active) return null; + clipped.dispatchEvent(new MouseEvent('mouseenter')); + await new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(resolve))); - const panel = el.getBoundingClientRect(); - const option = active.getBoundingClientRect(); + return { before, after: element.scrollTop }; + }, '.kbq-option'); + + expect(offsets).not.toBeNull(); + expect(offsets!.after).toBe(offsets!.before); + }); + + test('brings the active option into view on keyboard navigation without scrolling the page', async ({ page }) => { + const port = getPort(page); + const itemCount = await port.locator('.kbq-option').count(); + + for (let i = 0; i < itemCount; i++) { + await page.keyboard.press('ArrowDown'); + } - return option.top >= panel.top - 1 && option.bottom <= panel.bottom + 1; - }); + await e2eSettleFrames(page); - expect(activeIsVisible).toBe(true); + expect(await e2eScrollTopOf(port)).toBeGreaterThan(0); + expect(await e2eIsFullyInView(port, '.kbq-option.kbq-active')).toBe(true); expect(await page.evaluate(() => window.scrollY)).toBe(0); }); }); diff --git a/packages/components/core/option/option.spec.ts b/packages/components/core/option/option.spec.ts index 5e69b8f9b3..009cad7095 100644 --- a/packages/components/core/option/option.spec.ts +++ b/packages/components/core/option/option.spec.ts @@ -34,7 +34,7 @@ describe('KbqOption component', () => { subscription.unsubscribe(); }); - it('should never let focus scroll the option into view, however it was activated', () => { + it('should reveal the option on focus without letting the browser scroll from focus itself', () => { const fixture = TestBed.createComponent(OptionWithDisable); fixture.detectChanges(); @@ -42,18 +42,28 @@ describe('KbqOption component', () => { const option: KbqOption = fixture.debugElement.query(By.directive(KbqOption)).componentInstance; const host = option.getHostElement(); const focusSpy = jest.spyOn(host, 'focus'); + const scrollSpy = jest.spyOn(host, 'scrollIntoView'); option.focus(); - expect(focusSpy).toHaveBeenNthCalledWith(1, { preventScroll: true }); + expect(focusSpy).toHaveBeenCalledWith({ preventScroll: true }); + expect(scrollSpy).toHaveBeenCalledWith({ block: 'nearest', inline: 'nearest' }); + }); + + it('should not scroll the list when the option is activated by the pointer', () => { + const fixture = TestBed.createComponent(OptionWithDisable); + + fixture.detectChanges(); + + const option: KbqOption = fixture.debugElement.query(By.directive(KbqOption)).componentInstance; + const host = option.getHostElement(); + const scrollSpy = jest.spyOn(host, 'scrollIntoView'); - // Hovering once suppressed the implicit scroll for the next focus only. The panel scrolls - // explicitly now, so focus must never ask for a scroll — WebKit defers that scroll past the - // reader's own, which left the panel unscrollable while the pointer sat over the list. host.dispatchEvent(new MouseEvent('mouseenter')); option.focus(); - expect(focusSpy).toHaveBeenNthCalledWith(2, { preventScroll: true }); + // The option is already under the cursor; revealing it would shift the list out from under it. + expect(scrollSpy).not.toHaveBeenCalled(); }); it('should not emit to `onSelectionChange` if selecting an already-selected option', () => { diff --git a/packages/components/core/option/option.ts b/packages/components/core/option/option.ts index 06fde065be..0ea1b24643 100644 --- a/packages/components/core/option/option.ts +++ b/packages/components/core/option/option.ts @@ -258,6 +258,9 @@ export class KbqOption extends KbqOptionBase implements AfterViewChecked, OnDest private mostRecentViewValue = ''; + /** Set while the pointer activates this option, so that focusing it does not scroll the list. */ + private activatedByPointer = false; + ngAfterViewChecked() { // Since parent components could be using the option's label to display the selected values // (e.g. `kbq-select`) and they don't have a way of knowing if the option's label has changed @@ -302,17 +305,30 @@ export class KbqOption extends KbqOptionBase implements AfterViewChecked, OnDest } /** - * Moves keyboard focus to this option without scrolling it into view. The owning panel scrolls - * explicitly instead — see `KbqScrollbarViewport.scrollIntoViewNearest`. Relying on the implicit - * scroll that focus performs is not portable: WebKit defers it to a later rendering update, where - * it lands after — and undoes — any scrolling the reader did in the meantime. + * Moves keyboard focus to this option and reveals it. + * + * The reveal is explicit because the one `focus()` performs implicitly is not portable: WebKit defers + * it to a later rendering update, where it lands after — and undoes — any scrolling the reader did in + * the meantime. Doing it here rather than in the panel keeps every consumer of `KbqOption` working, + * whatever it uses as a scroll container. */ focus(): void { const element = this.getHostElement(); - if (typeof element.focus === 'function') { - element.focus({ preventScroll: true }); - } + if (typeof element.focus !== 'function') return; + + // Re-entrant calls (a focus listener calling back into `focus`) must not scroll a second time. + const wasFocused = element.ownerDocument.activeElement === element; + const activatedByPointer = this.activatedByPointer; + + this.activatedByPointer = false; + + element.focus({ preventScroll: true }); + + // The pointer is already on this option; revealing it would shift the list out from under it. + if (activatedByPointer || wasFocused) return; + + element.scrollIntoView?.({ block: 'nearest', inline: 'nearest' }); } /** @@ -395,6 +411,8 @@ export class KbqOption extends KbqOptionBase implements AfterViewChecked, OnDest protected onMouseenter() { if (this.disabled) return; + this.activatedByPointer = true; + this.parent?.keyManager?.setActiveItem(this); } } diff --git a/packages/components/dropdown/dropdown-item.component.ts b/packages/components/dropdown/dropdown-item.component.ts index feae43c7f9..755ca1b344 100644 --- a/packages/components/dropdown/dropdown-item.component.ts +++ b/packages/components/dropdown/dropdown-item.component.ts @@ -132,20 +132,29 @@ export class KbqDropdownItem implements KbqTitleTextRef, KbqDropdownItemActionHo } /** - * Focuses the dropdown item without scrolling it into view. The panel scrolls the active item - * explicitly instead — see `KbqDropdown`. Relying on the implicit scroll that focus performs is not - * portable: WebKit defers it to a later rendering update, where it lands after — and undoes — any - * scrolling the reader did in the meantime. + * Focuses the dropdown item and reveals it. + * + * The reveal is explicit because the one `focus()` performs implicitly is not portable: WebKit defers + * it to a later rendering update, where it lands after — and undoes — any scrolling the reader did in + * the meantime. `preventScroll` is therefore always forced on, overriding `options`. */ focus(origin?: FocusOrigin, options?: FocusOptions): void { if (this.disabled) return; + const element = this.getHostElement(); + // A focus listener calling back into `focus` must not scroll a second time. + const wasFocused = element.ownerDocument.activeElement === element; const focusOptions: FocusOptions = { ...options, preventScroll: true }; if (this.focusMonitor && origin) { - this.focusMonitor.focusVia(this.getHostElement(), origin, focusOptions); + this.focusMonitor.focusVia(element, origin, focusOptions); } else { - this.getHostElement().focus(focusOptions); + element.focus(focusOptions); + } + + // With the pointer already on this item, revealing it would shift the list out from under it. + if (origin !== 'mouse' && !wasFocused) { + element.scrollIntoView?.({ block: 'nearest', inline: 'nearest' }); } this.focused.next(this); diff --git a/packages/components/dropdown/dropdown.component.ts b/packages/components/dropdown/dropdown.component.ts index 3eeca2eb0a..4af9d733d1 100644 --- a/packages/components/dropdown/dropdown.component.ts +++ b/packages/components/dropdown/dropdown.component.ts @@ -330,9 +330,6 @@ export class KbqDropdown implements AfterContentInit, KbqDropdownPanel, OnInit, /** Subscription to tab events on the dropdown panel */ private tabSubscription = Subscription.EMPTY; - /** Subscription keeping the panel scrolled to the active item. */ - private activeItemSubscription = Subscription.EMPTY; - /** Cleans up the safe-area `mousemove` listener. `null` when no safe area is being tracked. */ private safeAreaCleanup: (() => void) | null = null; @@ -370,12 +367,6 @@ export class KbqDropdown implements AfterContentInit, KbqDropdownPanel, OnInit, this.tabSubscription = this.keyManager.tabOut.subscribe(() => this.closed.emit('tab')); - // `FocusKeyManager` focuses the item it activates, but focus is not allowed to scroll (see - // `KbqDropdownItem.focus`), so the panel follows the active item itself. Only `setActiveItem` - // emits here — 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. - this.activeItemSubscription = this.keyManager.change.subscribe(() => this.scrollActiveItemIntoView()); - // If a user manually (programmatically) focuses a menu item, we need to reflect that focus // change back to the key manager. Note that we don't need to unsubscribe here because focused // is internal and we know that it gets completed on destroy. @@ -395,7 +386,6 @@ export class KbqDropdown implements AfterContentInit, KbqDropdownPanel, OnInit, ngOnDestroy() { this.directDescendantItems.destroy(); this.tabSubscription.unsubscribe(); - this.activeItemSubscription.unsubscribe(); this.closed.complete(); this.deactivateSafeArea(); this.panelReached.complete(); @@ -595,15 +585,6 @@ export class KbqDropdown implements AfterContentInit, KbqDropdownPanel, OnInit, } } - /** Scrolls the panel by as little as it takes to reveal the active item, leaving a visible one alone. */ - private scrollActiveItemIntoView(): void { - const activeItem = this.keyManager.activeItem; - - if (!activeItem) return; - - this.scrollbarViewport()?.scrollIntoViewNearest(activeItem.getHostElement()); - } - /** Moves DOM focus onto the dropdown panel so that keydown events keep being handled. */ private focusPanel(): void { // The panel is rendered into the overlay through a `TemplatePortal`, so it can't be diff --git a/packages/components/dropdown/e2e.webkit.playwright-spec.ts b/packages/components/dropdown/e2e.webkit.playwright-spec.ts index 9f30f75125..16f894e321 100644 --- a/packages/components/dropdown/e2e.webkit.playwright-spec.ts +++ b/packages/components/dropdown/e2e.webkit.playwright-spec.ts @@ -1,103 +1,109 @@ -import { expect, Locator, Page, test } from '@playwright/test'; -import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions } from '../../e2e/utils'; - -/* -------------------------------------------------------------------------- */ -/* WebKit-only regression guard for panel scrolling (DS-3299). */ -/* */ -/* `FocusKeyManager` focuses the item it activates, and the panel used to */ -/* lean 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. Hovering re-focuses items as they pass under the */ -/* pointer, so the panel could not be scrolled at all in Safari. */ -/* */ -/* These assert on scroll offsets rather than screenshots, so they need no */ -/* baselines and no Docker. */ -/* -------------------------------------------------------------------------- */ +import { expect, Page, test } from '@playwright/test'; +import { + e2eIsFullyInView, + e2eReadOptionFocusOptions, + e2eRecordOptionFocusOptions, + e2eScrollTopOf, + e2eSettleFrames +} from '../../e2e/utils'; + +/* + * WebKit-only guard for panel scrolling (DS-3299). + * + * Focus scrolls its target into view implicitly. Blink does that synchronously; WebKit defers it to a + * later rendering update, where it lands after — and undoes — whatever the reader scrolled in between. + * Components therefore focus with `preventScroll` and reveal the item themselves. + * + * These assert on scroll offsets rather than screenshots, so they need no baselines and no Docker. + */ test.use({ browserName: 'webkit' }); -/** Waits two frames — where WebKit's deferred focus scroll used to land. */ -const settle = (page: Page) => - page.evaluate( - () => new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))) - ); - -const scrollTopOf = (locator: Locator) => locator.evaluate((el) => el.scrollTop); - test.describe('KbqDropdown panel scrolling', () => { - const getPanel = (page: Page) => page.locator('.kbq-dropdown__panel'); + const getPort = (page: Page) => page.locator('.kbq-dropdown__panel'); test.beforeEach(async ({ page }) => { + // Installed before navigation so the focus calls the panel makes while opening are recorded too. + await e2eRecordOptionFocusOptions(page); await page.goto('/E2eDropdownScrollbar'); await page.getByTestId('e2eDropdownScrollbarTrigger').click(); - await expect(getPanel(page)).toBeVisible(); - // The panel is measured below, so wait for its items rather than just for the box itself. - await expect(getPanel(page).locator('.kbq-dropdown-item').first()).toBeVisible(); + await expect(getPort(page)).toBeVisible(); + await expect(getPort(page).locator('.kbq-dropdown-item').first()).toBeVisible(); }); test('has a panel that actually overflows', async ({ page }) => { - // Guards the premise of every other test here: without overflow they would all pass vacuously. - await expect.poll(() => getPanel(page).evaluate((el) => el.scrollHeight > el.clientHeight)).toBe(true); + // Guards the premise of the other tests: without overflow they would all pass vacuously. + await expect.poll(() => getPort(page).evaluate((el) => el.scrollHeight > el.clientHeight)).toBe(true); }); - test('never asks the browser to scroll an item into view on focus', async ({ page }) => { - await e2eRecordOptionFocusOptions(page); - + test('never asks the browser to scroll on focus, including while opening', async ({ page }) => { for (let i = 0; i < 5; i++) { await page.keyboard.press('ArrowDown'); } const preventScrollFlags = await e2eReadOptionFocusOptions(page); - // A non-empty recording proves keyboard navigation really went through focus, so the - // assertion below cannot pass by never having been exercised. + // A non-empty recording proves focus was exercised, so the assertion cannot pass vacuously. expect(preventScrollFlags.length).toBeGreaterThan(0); expect(preventScrollFlags).not.toContain(false); }); test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { - const panel = getPanel(page); + const port = getPort(page); - await panel.hover(); + await port.hover(); await page.mouse.wheel(0, 200); - await settle(page); + await e2eSettleFrames(page); - const scrolled = await scrollTopOf(panel); + const scrolled = await e2eScrollTopOf(port); expect(scrolled).toBeGreaterThan(0); - await settle(page); + await e2eSettleFrames(page); - expect(await scrollTopOf(panel)).toBe(scrolled); + expect(await e2eScrollTopOf(port)).toBe(scrolled); }); - test('brings the active item into view on keyboard navigation without scrolling the page', async ({ page }) => { - const panel = getPanel(page); - const itemCount = await panel.locator('.kbq-dropdown-item').count(); + test('does not move the list when the pointer lands on a partially clipped item', async ({ page }) => { + const port = getPort(page); - // Roughly twenty items fit, so walking to the last one is what forces the panel to scroll. - // Navigation does not wrap here (`navigationWithWrap` defaults to false), so this lands on the end. - for (let i = 0; i < itemCount; i++) { - await page.keyboard.press('ArrowDown'); - } + const offsets = await port.evaluate(async (element, sel) => { + // Land on a fractional offset so a row straddles the top edge of the scrollport. + element.scrollTop = 50; + await new Promise((resolve) => requestAnimationFrame(resolve)); - await settle(page); + const before = element.scrollTop; + const port = element.getBoundingClientRect(); + const clipped = [...element.querySelectorAll(sel)].find((item) => { + const box = item.getBoundingClientRect(); - expect(await scrollTopOf(panel)).toBeGreaterThan(0); + return box.top < port.top && box.bottom > port.top; + }); - const activeIsVisible = await panel.evaluate((el) => { - const active = el.querySelector('.kbq-dropdown-item:focus'); + if (!clipped) return null; - if (!active) return null; + clipped.dispatchEvent(new MouseEvent('mouseenter')); + await new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(resolve))); - const panelRect = el.getBoundingClientRect(); - const itemRect = active.getBoundingClientRect(); + return { before, after: element.scrollTop }; + }, '.kbq-dropdown-item'); + + expect(offsets).not.toBeNull(); + expect(offsets!.after).toBe(offsets!.before); + }); + + test('brings the active item into view on keyboard navigation without scrolling the page', async ({ page }) => { + const port = getPort(page); + const itemCount = await port.locator('.kbq-dropdown-item').count(); + + for (let i = 0; i < itemCount; i++) { + await page.keyboard.press('ArrowDown'); + } - return itemRect.top >= panelRect.top - 1 && itemRect.bottom <= panelRect.bottom + 1; - }); + await e2eSettleFrames(page); - expect(activeIsVisible).toBe(true); + expect(await e2eScrollTopOf(port)).toBeGreaterThan(0); + expect(await e2eIsFullyInView(port, '.kbq-dropdown-item:focus')).toBe(true); expect(await page.evaluate(() => window.scrollY)).toBe(0); }); }); diff --git a/packages/components/scrollbar/scrollbar.spec.ts b/packages/components/scrollbar/scrollbar.spec.ts index 3ee3ced3a9..bd3f5c2ba5 100644 --- a/packages/components/scrollbar/scrollbar.spec.ts +++ b/packages/components/scrollbar/scrollbar.spec.ts @@ -1136,122 +1136,6 @@ describe(KbqScrollbar.name, () => { expect(scrollToSpy).toHaveBeenCalledWith({ top: 175, left: 175 }); }); - - /** - * Places the scrollport and the target in one coordinate space, the way `scrollIntoViewNearest` - * reads them: the scrollport's rect sits at y=0, and the target's rect is the position it would - * paint at for the given `scrollTop` — i.e. its offset in the content, minus how far it is scrolled. - */ - const placeTarget = ( - viewport: HTMLElement, - target: HTMLElement, - content: { offsetTop: number; height: number; offsetLeft: number; width: number }, - scroll: { top: number; left: number } - ) => { - setRect(viewport, { top: 0, left: 0, width: 100, height: 100 }); - setRect(target, { - top: content.offsetTop - scroll.top, - left: content.offsetLeft - scroll.left, - width: content.width, - height: content.height - }); - }; - - it('scrollIntoViewNearest aligns a target above the scrollport to its start edge', () => { - const fixture = createComponent(TestScrollbarScrollTo); - const target = fixture.componentInstance.target().nativeElement; - - setMetrics(fixture.componentInstance.scrollbarEl().nativeElement, { - clientHeight: 100, - clientWidth: 100, - scrollTop: 200, - scrollLeft: 0 - }); - placeTarget( - fixture.componentInstance.scrollbarEl().nativeElement, - target, - { offsetTop: 50, height: 50, offsetLeft: 0, width: 50 }, - { top: 200, left: 0 } - ); - - const scrollToSpy = spyOnScrollTo(fixture); - - fixture.componentInstance.scrollbar().scrollIntoViewNearest(target); - - expect(scrollToSpy).toHaveBeenCalledWith({ top: 50, left: 0 }); - }); - - it('scrollIntoViewNearest aligns a target below the scrollport to its end edge', () => { - const fixture = createComponent(TestScrollbarScrollTo); - const target = fixture.componentInstance.target().nativeElement; - - setMetrics(fixture.componentInstance.scrollbarEl().nativeElement, { - clientHeight: 100, - clientWidth: 100, - scrollTop: 0, - scrollLeft: 0 - }); - placeTarget( - fixture.componentInstance.scrollbarEl().nativeElement, - target, - { offsetTop: 200, height: 50, offsetLeft: 0, width: 50 }, - { top: 0, left: 0 } - ); - - const scrollToSpy = spyOnScrollTo(fixture); - - fixture.componentInstance.scrollbar().scrollIntoViewNearest(target); - - expect(scrollToSpy).toHaveBeenCalledWith({ top: 150, left: 0 }); - }); - - it('scrollIntoViewNearest leaves a fully visible target where it is', () => { - const fixture = createComponent(TestScrollbarScrollTo); - const target = fixture.componentInstance.target().nativeElement; - - setMetrics(fixture.componentInstance.scrollbarEl().nativeElement, { - clientHeight: 100, - clientWidth: 100, - scrollTop: 100, - scrollLeft: 0 - }); - placeTarget( - fixture.componentInstance.scrollbarEl().nativeElement, - target, - { offsetTop: 120, height: 20, offsetLeft: 0, width: 50 }, - { top: 100, left: 0 } - ); - - const scrollToSpy = spyOnScrollTo(fixture); - - fixture.componentInstance.scrollbar().scrollIntoViewNearest(target); - - expect(scrollToSpy).not.toHaveBeenCalled(); - }); - - it('scrollIntoViewNearest brings the inline axis into view as well', () => { - const fixture = createComponent(TestScrollbarScrollTo); - const target = fixture.componentInstance.target().nativeElement; - - setMetrics(fixture.componentInstance.scrollbarEl().nativeElement, { - clientHeight: 100, - clientWidth: 100, - scrollTop: 0, - scrollLeft: 0 - }); - placeTarget( - fixture.componentInstance.scrollbarEl().nativeElement, - target, - { offsetTop: 0, height: 20, offsetLeft: 200, width: 50 }, - { top: 0, left: 0 } - ); - - const scrollToSpy = spyOnScrollTo(fixture); - - fixture.componentInstance.scrollbar().scrollIntoViewNearest(target); - - expect(scrollToSpy).toHaveBeenCalledWith({ top: 0, left: 150 }); - }); }); describe('scrollChanges', () => { diff --git a/packages/components/scrollbar/scrollbar.ts b/packages/components/scrollbar/scrollbar.ts index 575ae48343..09b65c0c97 100644 --- a/packages/components/scrollbar/scrollbar.ts +++ b/packages/components/scrollbar/scrollbar.ts @@ -127,21 +127,6 @@ function getElementOffset(ancestor: HTMLElement, element: HTMLElement): { offset return { offsetTop, offsetLeft }; } -// The offset that brings `[offset, offset + size]` inside `[current, current + viewport]` while moving as -// little as possible: past the start edge it aligns to the start, past the end edge to the end, and an -// already visible range is left where it is. -function nearestEdgeScrollOffset(offset: number, size: number, current: number, viewport: number): number { - if (offset < current) { - return offset; - } - - if (offset + size > current + viewport) { - return Math.max(0, offset + size - viewport); - } - - return current; -} - /** * How the scrollbar is presented: * - `hover` — track appears on pointer hover or while scrolling (default); @@ -387,38 +372,6 @@ export class KbqScrollbarViewport { }); } - /** - * Scrolls `target` just far enough to bring it inside the viewport, leaving an already visible target - * where it is. Unlike {@link scrollIntoView}, which centers its target, this keeps a list from jumping - * under the reader on every step of keyboard navigation. - * - * Scrolls this viewport only — an ancestor scroll container is never moved, which - * `Element.scrollIntoView({ block: 'nearest' })` does not promise inside an overlay. - */ - scrollIntoViewNearest(target: HTMLElement, behavior?: ScrollBehavior): void { - const element = this.getNativeElement(); - const { scrollTop, scrollLeft, clientHeight, clientWidth, clientTop, clientLeft } = element; - - // Measured from rects rather than `getElementOffset`, 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` and an `offsetParent` walk never reaches the scrollport. - // Rects are unaffected by that, and subtracting the border edge puts them in scroll coordinates. - const viewportRect = element.getBoundingClientRect(); - const targetRect = target.getBoundingClientRect(); - - const offsetTop = scrollTop + targetRect.top - viewportRect.top - clientTop; - const offsetLeft = scrollLeft + targetRect.left - viewportRect.left - clientLeft; - - const top = nearestEdgeScrollOffset(offsetTop, targetRect.height, scrollTop, clientHeight); - const left = nearestEdgeScrollOffset(offsetLeft, targetRect.width, scrollLeft, clientWidth); - - if (top === scrollTop && left === scrollLeft) { - return; - } - - this.scrollTo({ top, left, behavior }); - } - /** Scrolls `target` to the center of the viewport. */ scrollIntoView(target: HTMLElement, behavior?: ScrollBehavior): void { const { offsetHeight, offsetWidth } = target; @@ -850,11 +803,6 @@ export class KbqScrollbar { this.viewport.scrollToElement(target, options); } - /** Scrolls `target` into view by the shortest distance — see {@link KbqScrollbarViewport.scrollIntoViewNearest}. */ - scrollIntoViewNearest(target: HTMLElement, behavior?: ScrollBehavior): void { - this.viewport.scrollIntoViewNearest(target, behavior); - } - /** Scrolls `target` to the center of the viewport. */ scrollIntoView(target: HTMLElement, behavior?: ScrollBehavior): void { this.viewport.scrollIntoView(target, behavior); diff --git a/packages/components/select/e2e.webkit.playwright-spec.ts b/packages/components/select/e2e.webkit.playwright-spec.ts index 3ddbcfb719..f69dd90aab 100644 --- a/packages/components/select/e2e.webkit.playwright-spec.ts +++ b/packages/components/select/e2e.webkit.playwright-spec.ts @@ -1,135 +1,109 @@ -import { expect, Locator, Page, test } from '@playwright/test'; -import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions } from '../../e2e/utils'; - -/* -------------------------------------------------------------------------- */ -/* WebKit-only regression guard for panel scrolling (DS-3299). */ -/* */ -/* The panel used to scroll its 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 panel could not be scrolled at all in Safari. */ -/* */ -/* These assert on scroll offsets rather than screenshots, so they need no */ -/* baselines and no Docker. */ -/* -------------------------------------------------------------------------- */ +import { expect, Page, test } from '@playwright/test'; +import { + e2eIsFullyInView, + e2eReadOptionFocusOptions, + e2eRecordOptionFocusOptions, + e2eScrollTopOf, + e2eSettleFrames +} from '../../e2e/utils'; + +/* + * WebKit-only guard for panel scrolling (DS-3299). + * + * Focus scrolls its target into view implicitly. Blink does that synchronously; WebKit defers it to a + * later rendering update, where it lands after — and undoes — whatever the reader scrolled in between. + * Components therefore focus with `preventScroll` and reveal the option themselves. + * + * These assert on scroll offsets rather than screenshots, so they need no baselines and no Docker. + */ test.use({ browserName: 'webkit' }); -/** Waits two frames — where WebKit's deferred focus scroll used to land. */ -const settle = (page: Page) => - page.evaluate( - () => new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))) - ); - -const scrollTopOf = (locator: Locator) => locator.evaluate((el) => el.scrollTop); - test.describe('KbqSelect panel scrolling', () => { - const getSelect = (page: Page) => page.getByTestId('e2eSelect'); - const getContent = (page: Page) => page.locator('.kbq-select__content'); + const getPort = (page: Page) => page.locator('.kbq-select__content'); test.beforeEach(async ({ page }) => { + // Installed before navigation so the focus calls the panel makes while opening are recorded too. + await e2eRecordOptionFocusOptions(page); await page.goto('/E2eSelectScrollbar'); - await getSelect(page).click(); - await expect(getContent(page)).toBeVisible(); + await page.getByTestId('e2eSelect').click(); + await expect(getPort(page)).toBeVisible(); + await expect(getPort(page).locator('.kbq-option').first()).toBeVisible(); }); - test('never asks the browser to scroll an option into view on focus', async ({ page }) => { - await e2eRecordOptionFocusOptions(page); + test('has a panel that actually overflows', async ({ page }) => { + // Guards the premise of the other tests: without overflow they would all pass vacuously. + await expect.poll(() => getPort(page).evaluate((el) => el.scrollHeight > el.clientHeight)).toBe(true); + }); + test('never asks the browser to scroll on focus, including while opening', async ({ page }) => { for (let i = 0; i < 5; i++) { await page.keyboard.press('ArrowDown'); } const preventScrollFlags = await e2eReadOptionFocusOptions(page); - // A non-empty recording proves keyboard navigation really went through focus, so the - // assertion below cannot pass by never having been exercised. + // A non-empty recording proves focus was exercised, so the assertion cannot pass vacuously. expect(preventScrollFlags.length).toBeGreaterThan(0); expect(preventScrollFlags).not.toContain(false); }); test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { - const content = getContent(page); + const port = getPort(page); - await content.hover(); + await port.hover(); await page.mouse.wheel(0, 200); - await settle(page); + await e2eSettleFrames(page); - const scrolled = await scrollTopOf(content); + const scrolled = await e2eScrollTopOf(port); expect(scrolled).toBeGreaterThan(0); - await settle(page); + await e2eSettleFrames(page); - expect(await scrollTopOf(content)).toBe(scrolled); + expect(await e2eScrollTopOf(port)).toBe(scrolled); }); - test('brings the active option into view on keyboard navigation without scrolling the page', async ({ page }) => { - const content = getContent(page); + test('does not move the list when the pointer lands on a partially clipped option', async ({ page }) => { + const port = getPort(page); - for (let i = 0; i < 15; i++) { - await page.keyboard.press('ArrowDown'); - } - - await settle(page); - - expect(await scrollTopOf(content)).toBeGreaterThan(0); - - // The active option is fully inside the scrollport, not merely somewhere in the list. - const activeIsVisible = await content.evaluate((el) => { - const active = el.querySelector('.kbq-option.kbq-active'); + const offsets = await port.evaluate(async (element, sel) => { + // Land on a fractional offset so a row straddles the top edge of the scrollport. + element.scrollTop = 50; + await new Promise((resolve) => requestAnimationFrame(resolve)); - if (!active) return null; + const before = element.scrollTop; + const port = element.getBoundingClientRect(); + const clipped = [...element.querySelectorAll(sel)].find((item) => { + const box = item.getBoundingClientRect(); - const panel = el.getBoundingClientRect(); - const option = active.getBoundingClientRect(); + return box.top < port.top && box.bottom > port.top; + }); - return option.top >= panel.top - 1 && option.bottom <= panel.bottom + 1; - }); + if (!clipped) return null; - expect(activeIsVisible).toBe(true); - expect(await page.evaluate(() => window.scrollY)).toBe(0); - }); -}); + clipped.dispatchEvent(new MouseEvent('mouseenter')); + await new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(resolve))); -test.describe('KbqSelect panel scrolling under virtual scroll', () => { - // Virtual scroll swaps in a CDK viewport that translates its content instead of laying it out at the - // scroll offset, so the panel drives it through the CDK's own API rather than by element offsets. - const getViewport = (page: Page) => page.locator('cdk-virtual-scroll-viewport'); + return { before, after: element.scrollTop }; + }, '.kbq-option'); - test.beforeEach(async ({ page }) => { - await page.goto('/E2eVirtualScrollSelectScrollbar'); - await page.getByTestId('e2eSelect').click(); - await expect(getViewport(page)).toBeVisible(); + expect(offsets).not.toBeNull(); + expect(offsets!.after).toBe(offsets!.before); }); - test('follows the active option with the keyboard', async ({ page }) => { - const viewport = getViewport(page); - const initial = await scrollTopOf(viewport); + test('brings the active option into view on keyboard navigation without scrolling the page', async ({ page }) => { + const port = getPort(page); + const itemCount = await port.locator('.kbq-option').count(); - for (let i = 0; i < 20; i++) { + for (let i = 0; i < itemCount; i++) { await page.keyboard.press('ArrowDown'); } - await settle(page); + await e2eSettleFrames(page); - // Twenty options is past the panel's cap, so the viewport has to have followed the active one. - expect(await scrollTopOf(viewport)).toBeGreaterThan(initial); + expect(await e2eScrollTopOf(port)).toBeGreaterThan(0); + expect(await e2eIsFullyInView(port, '.kbq-option.kbq-active')).toBe(true); expect(await page.evaluate(() => window.scrollY)).toBe(0); }); - - test('never asks the browser to scroll an option into view on focus', async ({ page }) => { - await e2eRecordOptionFocusOptions(page); - - for (let i = 0; i < 5; i++) { - await page.keyboard.press('ArrowDown'); - } - - const preventScrollFlags = await e2eReadOptionFocusOptions(page); - - expect(preventScrollFlags.length).toBeGreaterThan(0); - expect(preventScrollFlags).not.toContain(false); - }); }); diff --git a/packages/components/select/select.component.ts b/packages/components/select/select.component.ts index 3e47569c4a..f6ef8dd5af 100644 --- a/packages/components/select/select.component.ts +++ b/packages/components/select/select.component.ts @@ -2251,26 +2251,9 @@ export class KbqSelect } } - /** - * Focuses the active option and scrolls the panel by as little as it takes to reveal it. - * - * The scroll is explicit on purpose. Focus performs one implicitly, 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 pass under the pointer, that made the panel unscrollable. - */ + /** Focuses the active option, which reveals it — see `KbqOption.focus`. */ private scrollActiveOptionIntoView(): void { - const activeItem = this.keyManager.activeItem; - - if (!activeItem) return; - - activeItem.focus(); - - this.activeScrollbarViewport?.scrollIntoViewNearest(activeItem.getHostElement()); - } - - /** The panel's real scrolling element: virtual scroll projects its own viewport in place of ours. */ - private get activeScrollbarViewport(): KbqScrollbarViewport | undefined { - return this.projectedScrollbarViewport() ?? this.scrollbarViewport(); + this.keyManager.activeItem?.focus(); } /** Comparison function to specify which option is displayed. Defaults to object equality. */ diff --git a/packages/components/tree-select/e2e.webkit.playwright-spec.ts b/packages/components/tree-select/e2e.webkit.playwright-spec.ts index 0ca2fbae95..e6f81157f5 100644 --- a/packages/components/tree-select/e2e.webkit.playwright-spec.ts +++ b/packages/components/tree-select/e2e.webkit.playwright-spec.ts @@ -1,93 +1,109 @@ -import { expect, Locator, Page, test } from '@playwright/test'; -import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions } from '../../e2e/utils'; - -/* -------------------------------------------------------------------------- */ -/* WebKit-only regression guard for panel scrolling (DS-3299). */ -/* */ -/* The panel used to scroll its active node 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. Tree-select was the worst affected: it focused with no origin */ -/* at all, so even hovering a node queued one. */ -/* */ -/* These assert on scroll offsets rather than screenshots, so they need no */ -/* baselines and no Docker. */ -/* -------------------------------------------------------------------------- */ +import { expect, Page, test } from '@playwright/test'; +import { + e2eIsFullyInView, + e2eReadOptionFocusOptions, + e2eRecordOptionFocusOptions, + e2eScrollTopOf, + e2eSettleFrames +} from '../../e2e/utils'; + +/* + * WebKit-only guard for panel scrolling (DS-3299). + * + * Focus scrolls its target into view implicitly. Blink does that synchronously; WebKit defers it to a + * later rendering update, where it lands after — and undoes — whatever the reader scrolled in between. + * Components therefore focus with `preventScroll` and reveal the node themselves. + * + * These assert on scroll offsets rather than screenshots, so they need no baselines and no Docker. + */ test.use({ browserName: 'webkit' }); -/** Waits two frames — where WebKit's deferred focus scroll used to land. */ -const settle = (page: Page) => - page.evaluate( - () => new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))) - ); - -const scrollTopOf = (locator: Locator) => locator.evaluate((el) => el.scrollTop); - test.describe('KbqTreeSelect panel scrolling', () => { - const getContent = (page: Page) => page.locator('.kbq-tree-select__content'); + const getPort = (page: Page) => page.locator('.kbq-tree-select__content'); test.beforeEach(async ({ page }) => { + // Installed before navigation so the focus calls the panel makes while opening are recorded too. + await e2eRecordOptionFocusOptions(page); await page.goto('/E2eTreeSelectScrollbar'); await page.getByTestId('e2eTreeSelect').click(); - await expect(getContent(page)).toBeVisible(); + await expect(getPort(page)).toBeVisible(); + await expect(getPort(page).locator('.kbq-tree-option').first()).toBeVisible(); }); - test('never asks the browser to scroll a node into view on focus', async ({ page }) => { - await e2eRecordOptionFocusOptions(page); + test('has a panel that actually overflows', async ({ page }) => { + // Guards the premise of the other tests: without overflow they would all pass vacuously. + await expect.poll(() => getPort(page).evaluate((el) => el.scrollHeight > el.clientHeight)).toBe(true); + }); + test('never asks the browser to scroll on focus, including while opening', async ({ page }) => { for (let i = 0; i < 5; i++) { await page.keyboard.press('ArrowDown'); } const preventScrollFlags = await e2eReadOptionFocusOptions(page); - // A non-empty recording proves keyboard navigation really went through focus, so the - // assertion below cannot pass by never having been exercised. + // A non-empty recording proves focus was exercised, so the assertion cannot pass vacuously. expect(preventScrollFlags.length).toBeGreaterThan(0); expect(preventScrollFlags).not.toContain(false); }); test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { - const content = getContent(page); + const port = getPort(page); - await content.hover(); + await port.hover(); await page.mouse.wheel(0, 200); - await settle(page); + await e2eSettleFrames(page); - const scrolled = await scrollTopOf(content); + const scrolled = await e2eScrollTopOf(port); expect(scrolled).toBeGreaterThan(0); - await settle(page); + await e2eSettleFrames(page); - expect(await scrollTopOf(content)).toBe(scrolled); + expect(await e2eScrollTopOf(port)).toBe(scrolled); }); - test('brings the active node into view on keyboard navigation without scrolling the page', async ({ page }) => { - const content = getContent(page); + test('does not move the list when the pointer lands on a partially clipped node', async ({ page }) => { + const port = getPort(page); - for (let i = 0; i < 15; i++) { - await page.keyboard.press('ArrowDown'); - } + const offsets = await port.evaluate(async (element, sel) => { + // Land on a fractional offset so a row straddles the top edge of the scrollport. + element.scrollTop = 50; + await new Promise((resolve) => requestAnimationFrame(resolve)); - await settle(page); + const before = element.scrollTop; + const port = element.getBoundingClientRect(); + const clipped = [...element.querySelectorAll(sel)].find((item) => { + const box = item.getBoundingClientRect(); - expect(await scrollTopOf(content)).toBeGreaterThan(0); + return box.top < port.top && box.bottom > port.top; + }); - const activeIsVisible = await content.evaluate((el) => { - const active = el.querySelector('.kbq-tree-option.kbq-active, .kbq-tree-option:focus'); + if (!clipped) return null; - if (!active) return null; + clipped.dispatchEvent(new MouseEvent('mouseenter')); + await new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(resolve))); - const panel = el.getBoundingClientRect(); - const option = active.getBoundingClientRect(); + return { before, after: element.scrollTop }; + }, '.kbq-tree-option'); + + expect(offsets).not.toBeNull(); + expect(offsets!.after).toBe(offsets!.before); + }); + + test('brings the active node into view on keyboard navigation without scrolling the page', async ({ page }) => { + const port = getPort(page); + const itemCount = await port.locator('.kbq-tree-option').count(); + + for (let i = 0; i < itemCount; i++) { + await page.keyboard.press('ArrowDown'); + } - return option.top >= panel.top - 1 && option.bottom <= panel.bottom + 1; - }); + await e2eSettleFrames(page); - expect(activeIsVisible).toBe(true); + expect(await e2eScrollTopOf(port)).toBeGreaterThan(0); + expect(await e2eIsFullyInView(port, '.kbq-tree-option:focus')).toBe(true); expect(await page.evaluate(() => window.scrollY)).toBe(0); }); }); diff --git a/packages/components/tree-select/tree-select.component.ts b/packages/components/tree-select/tree-select.component.ts index 51cf964da8..87c766d427 100644 --- a/packages/components/tree-select/tree-select.component.ts +++ b/packages/components/tree-select/tree-select.component.ts @@ -1654,21 +1654,9 @@ export class KbqTreeSelect } } - /** - * Focuses the active option and scrolls the panel by as little as it takes to reveal it. - * - * The scroll is explicit on purpose. Focus performs one implicitly, 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 pass under the pointer, that made the panel unscrollable. - */ + /** Focuses the active node, which reveals it — see `KbqTreeOption.focus`. */ private scrollActiveOptionIntoView() { - const activeItem = this.tree()!.keyManager.activeItem; - - if (!activeItem) return; - - activeItem.focus(); - - this.scrollbarViewport()?.scrollIntoViewNearest(activeItem.getHostElement()); + this.tree()!.keyManager.activeItem?.focus(); } private subscribeOnSearchChanges() { diff --git a/packages/components/tree/tree-option.component.ts b/packages/components/tree/tree-option.component.ts index 3e1d22af9d..51e6efe1b4 100644 --- a/packages/components/tree/tree-option.component.ts +++ b/packages/components/tree/tree-option.component.ts @@ -323,9 +323,18 @@ export class KbqTreeOption extends KbqTreeNode implements AfterCo focus(focusOrigin?: FocusOrigin) { if (focusOrigin === 'program' || this.disabled || this.actionButton()?.hasFocus) return; - // Never scroll from focus: WebKit defers that scroll to a later rendering update, where it lands - // after — and undoes — the reader's own scrolling. Panels scroll the active node explicitly. - this.elementRef.nativeElement.focus({ preventScroll: true }); + const element = this.elementRef.nativeElement; + // A focus listener calling back into `focus` must not scroll a second time. + const wasFocused = element.ownerDocument.activeElement === element; + + // The reveal is explicit because the one focus performs implicitly is not portable: WebKit defers + // it to a later rendering update, where it lands after — and undoes — the reader's own scrolling. + element.focus({ preventScroll: true }); + + // With the pointer already on this node, revealing it would shift the tree out from under it. + if (focusOrigin !== 'mouse' && !wasFocused) { + element.scrollIntoView?.({ block: 'nearest', inline: 'nearest' }); + } if (!this.hasFocus) { this.onFocus.next({ option: this }); diff --git a/packages/e2e/utils/focus-scroll.ts b/packages/e2e/utils/focus-scroll.ts index 1d00e524d0..3833dc03b3 100644 --- a/packages/e2e/utils/focus-scroll.ts +++ b/packages/e2e/utils/focus-scroll.ts @@ -1,4 +1,9 @@ -import { Page } from '@playwright/test'; +import { Locator, Page } from '@playwright/test'; + +type FocusScrollWindow = Window & { + __kbqOptionFocusOptions?: boolean[]; + __kbqOptionFocusPatched?: boolean; +}; /** * Records the `FocusOptions` every option-like element is focused with, so a spec can assert that a panel @@ -6,21 +11,28 @@ import { Page } from '@playwright/test'; * * That implicit scroll is not portable: Blink runs it synchronously, while WebKit defers it to a later * rendering update, where it lands after — and undoes — whatever the reader scrolled in the meantime. - * Panels are expected to focus with `preventScroll: true` and scroll explicitly instead. + * Components are expected to focus with `preventScroll: true` and reveal the element explicitly. * - * Call before the interaction, then read the flags back with {@link e2eReadOptionFocusOptions}. + * Installed with `addInitScript` so the recording covers the focus calls a panel makes as it opens, not + * just the ones a spec triggers afterwards. Call before `page.goto`, then read the flags back with + * {@link e2eReadOptionFocusOptions}. */ export const e2eRecordOptionFocusOptions = (page: Page): Promise => - page.evaluate(() => { - const recorded: boolean[] = []; + page.addInitScript(() => { + const target = window as FocusScrollWindow; + + target.__kbqOptionFocusOptions = []; + + // A page may be navigated more than once per spec; patch the prototype only once. + if (target.__kbqOptionFocusPatched) return; - (window as unknown as { __kbqOptionFocusOptions: boolean[] }).__kbqOptionFocusOptions = recorded; + target.__kbqOptionFocusPatched = true; const originalFocus = HTMLElement.prototype.focus; HTMLElement.prototype.focus = function (this: HTMLElement, options?: FocusOptions) { if (this.matches('.kbq-option, .kbq-tree-option, .kbq-dropdown-item')) { - recorded.push(options?.preventScroll === true); + (window as FocusScrollWindow).__kbqOptionFocusOptions?.push(options?.preventScroll === true); } return originalFocus.call(this, options); @@ -29,4 +41,28 @@ export const e2eRecordOptionFocusOptions = (page: Page): Promise => /** Reads back what {@link e2eRecordOptionFocusOptions} captured: one `preventScroll` flag per focus call. */ export const e2eReadOptionFocusOptions = (page: Page): Promise => - page.evaluate(() => (window as unknown as { __kbqOptionFocusOptions?: boolean[] }).__kbqOptionFocusOptions ?? []); + page.evaluate(() => (window as FocusScrollWindow).__kbqOptionFocusOptions ?? []); + +/** Waits two animation frames — long enough for a scroll a browser deferred past the current task to land. */ +export const e2eSettleFrames = (page: Page): Promise => + page.evaluate( + () => new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))) + ); + +/** Reads a scrollport's current vertical offset. */ +export const e2eScrollTopOf = (scrollport: Locator): Promise => + scrollport.evaluate((element) => element.scrollTop); + +/** Whether `selector`'s match inside `scrollport` is fully within it. Null when nothing matches. */ +export const e2eIsFullyInView = (scrollport: Locator, selector: string): Promise => + scrollport.evaluate((element, itemSelector) => { + const item = element.querySelector(itemSelector); + + if (!item) return null; + + const port = element.getBoundingClientRect(); + const box = item.getBoundingClientRect(); + + // A one-pixel tolerance absorbs sub-pixel rounding at fractional zoom levels. + return box.top >= port.top - 1 && box.bottom <= port.bottom + 1; + }, selector); diff --git a/tools/public_api_guard/components/scrollbar.api.md b/tools/public_api_guard/components/scrollbar.api.md index 63ae6f2f17..cb7291a5a8 100644 --- a/tools/public_api_guard/components/scrollbar.api.md +++ b/tools/public_api_guard/components/scrollbar.api.md @@ -33,7 +33,6 @@ export class KbqScrollbar { readonly scrollChanges: Observable; scrollEnd(behavior?: ScrollBehavior): void; scrollIntoView(target: HTMLElement, behavior?: ScrollBehavior): void; - scrollIntoViewNearest(target: HTMLElement, behavior?: ScrollBehavior): void; scrollStart(behavior?: ScrollBehavior): void; scrollTo(options: KbqScrollbarScrollToOptions): void; scrollToBottom(behavior?: ScrollBehavior): void; @@ -80,7 +79,6 @@ export class KbqScrollbarViewport { readonly scrollChanges: Observable; scrollEnd(behavior?: ScrollBehavior): void; scrollIntoView(target: HTMLElement, behavior?: ScrollBehavior): void; - scrollIntoViewNearest(target: HTMLElement, behavior?: ScrollBehavior): void; scrollStart(behavior?: ScrollBehavior): void; scrollTo(options: KbqScrollbarScrollToOptions): void; scrollToBottom(behavior?: ScrollBehavior): void; From 21f5fa045970e8edbaeb35f1053a94228c916214 Mon Sep 17 00:00:00 2001 From: lskramarov Date: Wed, 2 Sep 2026 10:02:59 +0300 Subject: [PATCH 4/7] fix(core,autocomplete): keep the unused scroll helpers as deprecated (#DS-3299) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../autocomplete-trigger.directive.ts | 9 ++++++ packages/components/core/option/option.ts | 30 +++++++++++++++++++ .../components/autocomplete.api.md | 3 ++ tools/public_api_guard/components/core.api.md | 3 ++ 4 files changed, 45 insertions(+) diff --git a/packages/components/autocomplete/autocomplete-trigger.directive.ts b/packages/components/autocomplete/autocomplete-trigger.directive.ts index 4fdc7d9899..3e4bbd0f52 100644 --- a/packages/components/autocomplete/autocomplete-trigger.directive.ts +++ b/packages/components/autocomplete/autocomplete-trigger.directive.ts @@ -58,6 +58,15 @@ import { delay, filter, map, switchMap, take, tap } from 'rxjs/operators'; import { KbqAutocompleteOrigin } from './autocomplete-origin.directive'; import { KbqAutocomplete } from './autocomplete.component'; +/** + * The total height of the autocomplete panel. + * + * @deprecated Unused — the panel is capped by `--kbq-autocomplete-size-panel-max-height` and reveals its + * active option through `KbqOption.focus`, so nothing computes a scroll offset from this. Will be removed + * in the next major release. + */ +export const AUTOCOMPLETE_PANEL_HEIGHT = 256; + /** * Injection token that determines the scroll handling while the autocomplete panel is open. The root default * keeps the trigger usable outside `KbqAutocompleteModule`'s injector; providing the token anywhere still wins diff --git a/packages/components/core/option/option.ts b/packages/components/core/option/option.ts index 0ea1b24643..70615fd1ce 100644 --- a/packages/components/core/option/option.ts +++ b/packages/components/core/option/option.ts @@ -446,3 +446,33 @@ export function countGroupLabelsBeforeOption( return 0; } + +/** + * Determines the position to which to scroll a panel in order for an option to be into view. + * @param optionIndex Index of the option to be scrolled into the view. + * @param optionHeight Height of the options. + * @param currentScrollPosition Current scroll position of the panel. + * @param panelHeight Height of the panel. + * @docs-private + * @deprecated Unused — an option reveals itself on focus through `KbqOption.focus`, which lets the + * browser resolve the scroll container instead of computing an offset from a uniform row height. Will be + * removed in the next major release. + */ +export function getOptionScrollPosition( + optionIndex: number, + optionHeight: number, + currentScrollPosition: number, + panelHeight: number +): number { + const optionOffset = optionIndex * optionHeight; + + if (optionOffset < currentScrollPosition) { + return optionOffset; + } + + if (optionOffset + optionHeight > currentScrollPosition + panelHeight) { + return Math.max(0, optionOffset - panelHeight + optionHeight); + } + + return currentScrollPosition; +} diff --git a/tools/public_api_guard/components/autocomplete.api.md b/tools/public_api_guard/components/autocomplete.api.md index ae2acce86f..962dfc8967 100644 --- a/tools/public_api_guard/components/autocomplete.api.md +++ b/tools/public_api_guard/components/autocomplete.api.md @@ -28,6 +28,9 @@ import { ScrollDispatcher } from '@angular/cdk/overlay'; import { ScrollStrategy } from '@angular/cdk/overlay'; import { TemplateRef } from '@angular/core'; +// @public @deprecated +export const AUTOCOMPLETE_PANEL_HEIGHT = 256; + // @public export function getKbqAutocompleteMissingPanelError(): Error; diff --git a/tools/public_api_guard/components/core.api.md b/tools/public_api_guard/components/core.api.md index c1fb108c54..39d3d40328 100644 --- a/tools/public_api_guard/components/core.api.md +++ b/tools/public_api_guard/components/core.api.md @@ -1030,6 +1030,9 @@ export function getKbqSelectNonFunctionValueError(): Error; // @public (undocumented) export const getNodesWithoutComments: (nodes: NodeList) => Node[]; +// @public @deprecated +export function getOptionScrollPosition(optionIndex: number, optionHeight: number, currentScrollPosition: number, panelHeight: number): number; + // @public export function getSafeTriangleVertices(origin: KbqPoint, targetRect: DOMRect): KbqTriangle; From 311f4fe091803e70d82de4fc9e6d3b15246fb1cd Mon Sep 17 00:00:00 2001 From: lskramarov Date: Wed, 2 Sep 2026 10:03:18 +0300 Subject: [PATCH 5/7] test(e2e): stop the wheel-scroll guard racing the gesture (#DS-3299) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../e2e.webkit.playwright-spec.ts | 11 ++++-- .../dropdown/e2e.webkit.playwright-spec.ts | 11 ++++-- .../select/e2e.webkit.playwright-spec.ts | 11 ++++-- .../tree-select/e2e.webkit.playwright-spec.ts | 11 ++++-- packages/e2e/utils/focus-scroll.ts | 39 +++++++++++++++++++ 5 files changed, 67 insertions(+), 16 deletions(-) diff --git a/packages/components/autocomplete/e2e.webkit.playwright-spec.ts b/packages/components/autocomplete/e2e.webkit.playwright-spec.ts index 718c3b0655..efd5d77f14 100644 --- a/packages/components/autocomplete/e2e.webkit.playwright-spec.ts +++ b/packages/components/autocomplete/e2e.webkit.playwright-spec.ts @@ -4,7 +4,8 @@ import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions, e2eScrollTopOf, - e2eSettleFrames + e2eSettleFrames, + e2eWaitForScrollEnd } from '../../e2e/utils'; /* @@ -53,15 +54,17 @@ test.describe('KbqAutocomplete panel scrolling', () => { await port.hover(); await page.mouse.wheel(0, 200); - await e2eSettleFrames(page); - const scrolled = await e2eScrollTopOf(port); + // The gesture is applied over several frames; wait for the offset itself rather than a frame count. + const scrolled = await e2eWaitForScrollEnd(port); expect(scrolled).toBeGreaterThan(0); + // Nothing may move the panel afterwards — a deferred focus scroll used to land right here. await e2eSettleFrames(page); - expect(await e2eScrollTopOf(port)).toBe(scrolled); + // A deferred focus scroll moves the panel by at least a row; a pixel of settling is not movement. + expect(Math.abs((await e2eScrollTopOf(port)) - scrolled)).toBeLessThanOrEqual(1); }); test('does not move the list when the pointer lands on a partially clipped option', async ({ page }) => { diff --git a/packages/components/dropdown/e2e.webkit.playwright-spec.ts b/packages/components/dropdown/e2e.webkit.playwright-spec.ts index 16f894e321..743695548c 100644 --- a/packages/components/dropdown/e2e.webkit.playwright-spec.ts +++ b/packages/components/dropdown/e2e.webkit.playwright-spec.ts @@ -4,7 +4,8 @@ import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions, e2eScrollTopOf, - e2eSettleFrames + e2eSettleFrames, + e2eWaitForScrollEnd } from '../../e2e/utils'; /* @@ -53,15 +54,17 @@ test.describe('KbqDropdown panel scrolling', () => { await port.hover(); await page.mouse.wheel(0, 200); - await e2eSettleFrames(page); - const scrolled = await e2eScrollTopOf(port); + // The gesture is applied over several frames; wait for the offset itself rather than a frame count. + const scrolled = await e2eWaitForScrollEnd(port); expect(scrolled).toBeGreaterThan(0); + // Nothing may move the panel afterwards — a deferred focus scroll used to land right here. await e2eSettleFrames(page); - expect(await e2eScrollTopOf(port)).toBe(scrolled); + // A deferred focus scroll moves the panel by at least a row; a pixel of settling is not movement. + expect(Math.abs((await e2eScrollTopOf(port)) - scrolled)).toBeLessThanOrEqual(1); }); test('does not move the list when the pointer lands on a partially clipped item', async ({ page }) => { diff --git a/packages/components/select/e2e.webkit.playwright-spec.ts b/packages/components/select/e2e.webkit.playwright-spec.ts index f69dd90aab..96ab6d2b44 100644 --- a/packages/components/select/e2e.webkit.playwright-spec.ts +++ b/packages/components/select/e2e.webkit.playwright-spec.ts @@ -4,7 +4,8 @@ import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions, e2eScrollTopOf, - e2eSettleFrames + e2eSettleFrames, + e2eWaitForScrollEnd } from '../../e2e/utils'; /* @@ -53,15 +54,17 @@ test.describe('KbqSelect panel scrolling', () => { await port.hover(); await page.mouse.wheel(0, 200); - await e2eSettleFrames(page); - const scrolled = await e2eScrollTopOf(port); + // The gesture is applied over several frames; wait for the offset itself rather than a frame count. + const scrolled = await e2eWaitForScrollEnd(port); expect(scrolled).toBeGreaterThan(0); + // Nothing may move the panel afterwards — a deferred focus scroll used to land right here. await e2eSettleFrames(page); - expect(await e2eScrollTopOf(port)).toBe(scrolled); + // A deferred focus scroll moves the panel by at least a row; a pixel of settling is not movement. + expect(Math.abs((await e2eScrollTopOf(port)) - scrolled)).toBeLessThanOrEqual(1); }); test('does not move the list when the pointer lands on a partially clipped option', async ({ page }) => { diff --git a/packages/components/tree-select/e2e.webkit.playwright-spec.ts b/packages/components/tree-select/e2e.webkit.playwright-spec.ts index e6f81157f5..55fcf3ed8e 100644 --- a/packages/components/tree-select/e2e.webkit.playwright-spec.ts +++ b/packages/components/tree-select/e2e.webkit.playwright-spec.ts @@ -4,7 +4,8 @@ import { e2eReadOptionFocusOptions, e2eRecordOptionFocusOptions, e2eScrollTopOf, - e2eSettleFrames + e2eSettleFrames, + e2eWaitForScrollEnd } from '../../e2e/utils'; /* @@ -53,15 +54,17 @@ test.describe('KbqTreeSelect panel scrolling', () => { await port.hover(); await page.mouse.wheel(0, 200); - await e2eSettleFrames(page); - const scrolled = await e2eScrollTopOf(port); + // The gesture is applied over several frames; wait for the offset itself rather than a frame count. + const scrolled = await e2eWaitForScrollEnd(port); expect(scrolled).toBeGreaterThan(0); + // Nothing may move the panel afterwards — a deferred focus scroll used to land right here. await e2eSettleFrames(page); - expect(await e2eScrollTopOf(port)).toBe(scrolled); + // A deferred focus scroll moves the panel by at least a row; a pixel of settling is not movement. + expect(Math.abs((await e2eScrollTopOf(port)) - scrolled)).toBeLessThanOrEqual(1); }); test('does not move the list when the pointer lands on a partially clipped node', async ({ page }) => { diff --git a/packages/e2e/utils/focus-scroll.ts b/packages/e2e/utils/focus-scroll.ts index 3833dc03b3..ed9a8b5290 100644 --- a/packages/e2e/utils/focus-scroll.ts +++ b/packages/e2e/utils/focus-scroll.ts @@ -53,6 +53,45 @@ export const e2eSettleFrames = (page: Page): Promise => export const e2eScrollTopOf = (scrollport: Locator): Promise => scrollport.evaluate((element) => element.scrollTop); +/** + * Waits until `scrollport`'s offset stops changing and returns it. + * + * A wheel gesture is applied over several frames, so reading the offset a fixed number of frames later + * catches an intermediate value — the reason to wait for the value itself rather than for a frame count. + */ +export const e2eWaitForScrollEnd = (scrollport: Locator): Promise => + scrollport.evaluate( + (element) => + new Promise((resolve) => { + const stableFramesNeeded = 3; + // Bounds the wait so a continuously animating scrollport fails on the assertion, not a hang. + const maxFrames = 120; + + let previous = element.scrollTop; + let stableFrames = 0; + let frames = 0; + + const check = () => { + if (element.scrollTop === previous) { + stableFrames++; + } else { + stableFrames = 0; + previous = element.scrollTop; + } + + if (stableFrames >= stableFramesNeeded || ++frames >= maxFrames) { + resolve(previous); + + return; + } + + requestAnimationFrame(check); + }; + + requestAnimationFrame(check); + }) + ); + /** Whether `selector`'s match inside `scrollport` is fully within it. Null when nothing matches. */ export const e2eIsFullyInView = (scrollport: Locator, selector: string): Promise => scrollport.evaluate((element, itemSelector) => { From 22eb0da6f3017297bcfed26b4b49074c8c35e9a8 Mon Sep 17 00:00:00 2001 From: lskramarov Date: Wed, 2 Sep 2026 16:30:39 +0300 Subject: [PATCH 6/7] fix(components): clear the pointer flag on mouseleave, pierce shadow roots (#DS-3299) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../autocomplete-trigger.directive.ts | 5 ++- .../autocomplete/autocomplete.component.ts | 5 --- .../e2e.webkit.playwright-spec.ts | 9 +++- .../components/core/option/option.spec.ts | 23 +++++++++- packages/components/core/option/option.ts | 44 +++++++------------ packages/components/core/utils/dom.ts | 26 +++++++++++ .../dropdown/dropdown-item.component.ts | 4 +- .../components/dropdown/dropdown.component.ts | 2 +- .../dropdown/e2e.webkit.playwright-spec.ts | 9 +++- .../select/e2e.webkit.playwright-spec.ts | 9 +++- .../components/select/select.component.ts | 1 - .../tree-select/e2e.webkit.playwright-spec.ts | 9 +++- .../tree-select/tree-select.component.ts | 3 +- .../components/tree/tree-option.component.ts | 13 +----- packages/e2e/utils/focus-scroll.ts | 18 ++++---- .../components/autocomplete.api.md | 1 - tools/public_api_guard/components/core.api.md | 4 ++ 17 files changed, 114 insertions(+), 71 deletions(-) diff --git a/packages/components/autocomplete/autocomplete-trigger.directive.ts b/packages/components/autocomplete/autocomplete-trigger.directive.ts index 3e4bbd0f52..dd7bd3d179 100644 --- a/packages/components/autocomplete/autocomplete-trigger.directive.ts +++ b/packages/components/autocomplete/autocomplete-trigger.directive.ts @@ -289,7 +289,8 @@ export class KbqAutocompleteTrigger if (this.panelOpen) { this.scrollActiveOptionIntoView(); - this.elementRef.nativeElement.focus(); + // Focus returns to the input on every arrow key; it must not drag the scroll with it. + this.elementRef.nativeElement.focus({ preventScroll: true }); } else if (!this.panelOpen && autocompleteValue.keyManager.activeItem) { autocompleteValue.keyManager.activeItem?.selectViaInteraction(); } @@ -468,7 +469,7 @@ export class KbqAutocompleteTrigger } scrollActiveOptionIntoView(): void { - this.autocomplete().scrollActiveOptionIntoView(); + this.autocomplete().keyManager.activeItem?.focus(); } /** Stream of clicks outside of the autocomplete panel. */ diff --git a/packages/components/autocomplete/autocomplete.component.ts b/packages/components/autocomplete/autocomplete.component.ts index 417d26044e..67cfbf5fc1 100644 --- a/packages/components/autocomplete/autocomplete.component.ts +++ b/packages/components/autocomplete/autocomplete.component.ts @@ -244,11 +244,6 @@ export class KbqAutocomplete implements AfterContentInit { }); } - /** Focuses the active option, which reveals it — see `KbqOption.focus`. */ - scrollActiveOptionIntoView(): void { - this.keyManager.activeItem?.focus(); - } - setScrollTop(scrollTop: number): void { const panel = this.panel(); diff --git a/packages/components/autocomplete/e2e.webkit.playwright-spec.ts b/packages/components/autocomplete/e2e.webkit.playwright-spec.ts index efd5d77f14..330b7815f2 100644 --- a/packages/components/autocomplete/e2e.webkit.playwright-spec.ts +++ b/packages/components/autocomplete/e2e.webkit.playwright-spec.ts @@ -28,6 +28,8 @@ test.describe('KbqAutocomplete panel scrolling', () => { await e2eRecordOptionFocusOptions(page); await page.goto('/E2eAutocompleteScrollbar'); await page.getByTestId('e2eAutocompleteInput').focus(); + // The routes fit the viewport, so without this the page-scroll assertion below could never fail. + await page.addStyleTag({ content: 'body { min-height: 3000px; }' }); await expect(getPort(page)).toBeVisible(); await expect(getPort(page).locator('.kbq-option').first()).toBeVisible(); }); @@ -52,11 +54,13 @@ test.describe('KbqAutocomplete panel scrolling', () => { test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { const port = getPort(page); + const start = await e2eScrollTopOf(port); + await port.hover(); await page.mouse.wheel(0, 200); - // The gesture is applied over several frames; wait for the offset itself rather than a frame count. - const scrolled = await e2eWaitForScrollEnd(port); + // The gesture is applied over several frames; wait for the offset to move and then settle. + const scrolled = await e2eWaitForScrollEnd(port, start); expect(scrolled).toBeGreaterThan(0); @@ -107,6 +111,7 @@ test.describe('KbqAutocomplete panel scrolling', () => { expect(await e2eScrollTopOf(port)).toBeGreaterThan(0); expect(await e2eIsFullyInView(port, '.kbq-option.kbq-active')).toBe(true); + // The page is deliberately made scrollable in beforeEach, so this can actually fail. expect(await page.evaluate(() => window.scrollY)).toBe(0); }); }); diff --git a/packages/components/core/option/option.spec.ts b/packages/components/core/option/option.spec.ts index 009cad7095..43ed64daf3 100644 --- a/packages/components/core/option/option.spec.ts +++ b/packages/components/core/option/option.spec.ts @@ -50,22 +50,43 @@ describe('KbqOption component', () => { expect(scrollSpy).toHaveBeenCalledWith({ block: 'nearest', inline: 'nearest' }); }); - it('should not scroll the list when the option is activated by the pointer', () => { + it('should focus but not reveal while the pointer is over the option', () => { const fixture = TestBed.createComponent(OptionWithDisable); fixture.detectChanges(); const option: KbqOption = fixture.debugElement.query(By.directive(KbqOption)).componentInstance; const host = option.getHostElement(); + const focusSpy = jest.spyOn(host, 'focus'); const scrollSpy = jest.spyOn(host, 'scrollIntoView'); host.dispatchEvent(new MouseEvent('mouseenter')); option.focus(); + // Focus must still move, otherwise type-ahead and aria-activedescendant break. + expect(focusSpy).toHaveBeenCalledWith({ preventScroll: true }); // The option is already under the cursor; revealing it would shift the list out from under it. expect(scrollSpy).not.toHaveBeenCalled(); }); + it('should reveal again once the pointer has left, even if no focus happened in between', () => { + const fixture = TestBed.createComponent(OptionWithDisable); + + fixture.detectChanges(); + + const option: KbqOption = fixture.debugElement.query(By.directive(KbqOption)).componentInstance; + const host = option.getHostElement(); + const scrollSpy = jest.spyOn(host, 'scrollIntoView'); + + // Hovering the option that is already active never reaches focus(), so a flag cleared only + // there would stay armed and silently swallow every later reveal. + host.dispatchEvent(new MouseEvent('mouseenter')); + host.dispatchEvent(new MouseEvent('mouseleave')); + option.focus(); + + expect(scrollSpy).toHaveBeenCalledWith({ block: 'nearest', inline: 'nearest' }); + }); + it('should not emit to `onSelectionChange` if selecting an already-selected option', () => { const fixture = TestBed.createComponent(OptionWithDisable); diff --git a/packages/components/core/option/option.ts b/packages/components/core/option/option.ts index 70615fd1ce..6636357068 100644 --- a/packages/components/core/option/option.ts +++ b/packages/components/core/option/option.ts @@ -22,6 +22,7 @@ import { ActiveDescendantKeyManager } from '../a11y'; import { ENTER, hasModifierKey, SPACE } from '../keycodes'; import { KbqPseudoCheckboxModule } from '../selection'; import { KBQ_TITLE_TEXT_REF, KbqTitleTextRef } from '../title'; +import { kbqFocusAndReveal } from '../utils'; import { KbqOptgroup } from './optgroup'; /** @@ -157,6 +158,7 @@ export class KbqVirtualOption extends KbqOptionBase { '(click)': 'handleClick($event)', '(mouseenter)': 'onMouseenter()', + '(mouseleave)': 'onMouseleave()', '(keydown)': 'handleKeydown($event)' }, exportAs: 'kbqOption' @@ -258,9 +260,12 @@ export class KbqOption extends KbqOptionBase implements AfterViewChecked, OnDest private mostRecentViewValue = ''; - /** Set while the pointer activates this option, so that focusing it does not scroll the list. */ - private activatedByPointer = false; - + /** + * Whether the pointer is currently over this option. Cleared on `mouseleave` rather than when the + * option is focused: hovering the option that is already active does not move the key manager's + * index, so no focus follows and a flag cleared only there would stay armed indefinitely. + */ + private hoveredByPointer = false; ngAfterViewChecked() { // Since parent components could be using the option's label to display the selected values // (e.g. `kbq-select`) and they don't have a way of knowing if the option's label has changed @@ -304,31 +309,9 @@ export class KbqOption extends KbqOptionBase implements AfterViewChecked, OnDest } } - /** - * Moves keyboard focus to this option and reveals it. - * - * The reveal is explicit because the one `focus()` performs implicitly is not portable: WebKit defers - * it to a later rendering update, where it lands after — and undoes — any scrolling the reader did in - * the meantime. Doing it here rather than in the panel keeps every consumer of `KbqOption` working, - * whatever it uses as a scroll container. - */ + /** Moves keyboard focus to this option and reveals it — see {@link kbqFocusAndReveal}. */ focus(): void { - const element = this.getHostElement(); - - if (typeof element.focus !== 'function') return; - - // Re-entrant calls (a focus listener calling back into `focus`) must not scroll a second time. - const wasFocused = element.ownerDocument.activeElement === element; - const activatedByPointer = this.activatedByPointer; - - this.activatedByPointer = false; - - element.focus({ preventScroll: true }); - - // The pointer is already on this option; revealing it would shift the list out from under it. - if (activatedByPointer || wasFocused) return; - - element.scrollIntoView?.({ block: 'nearest', inline: 'nearest' }); + kbqFocusAndReveal(this.getHostElement(), this.hoveredByPointer); } /** @@ -411,10 +394,15 @@ export class KbqOption extends KbqOptionBase implements AfterViewChecked, OnDest protected onMouseenter() { if (this.disabled) return; - this.activatedByPointer = true; + this.hoveredByPointer = true; this.parent?.keyManager?.setActiveItem(this); } + + /** @docs-private */ + protected onMouseleave() { + this.hoveredByPointer = false; + } } /** diff --git a/packages/components/core/utils/dom.ts b/packages/components/core/utils/dom.ts index 65bc4e9dea..82708ebad1 100644 --- a/packages/components/core/utils/dom.ts +++ b/packages/components/core/utils/dom.ts @@ -1,3 +1,4 @@ +import { _getFocusedElementPierceShadowDom } from '@angular/cdk/platform'; import { ElementRef, inject } from '@angular/core'; /** @@ -6,3 +7,28 @@ import { ElementRef, inject } from '@angular/core'; export const kbqInjectNativeElement = (): T => { return inject>(ElementRef).nativeElement; }; + +/** + * Focuses `element` and scrolls it into view by the shortest distance, leaving a visible one alone. + * + * The reveal is explicit because the one `focus()` performs implicitly is not portable: WebKit defers it + * to a later rendering update, where it lands after — and undoes — any scrolling the reader did in the + * meantime. Letting the element reveal itself keeps every consumer working, whatever it uses as a scroll + * container, and the browser resolves that container rather than the caller guessing at it. + * + * Pass `skipReveal` when the pointer caused the focus: the element is already under the cursor, so + * revealing it would shift the list out from under it. + */ +export const kbqFocusAndReveal = (element: HTMLElement, skipReveal = false): void => { + if (typeof element.focus !== 'function') return; + + // A focus listener calling back into `focus` must not scroll a second time. Read through shadow + // roots, where `document.activeElement` reports the host instead of the element that holds focus. + const wasFocused = _getFocusedElementPierceShadowDom() === element; + + element.focus({ preventScroll: true }); + + if (skipReveal || wasFocused) return; + + element.scrollIntoView?.({ block: 'nearest', inline: 'nearest' }); +}; diff --git a/packages/components/dropdown/dropdown-item.component.ts b/packages/components/dropdown/dropdown-item.component.ts index 755ca1b344..5bb618a657 100644 --- a/packages/components/dropdown/dropdown-item.component.ts +++ b/packages/components/dropdown/dropdown-item.component.ts @@ -142,8 +142,6 @@ export class KbqDropdownItem implements KbqTitleTextRef, KbqDropdownItemActionHo if (this.disabled) return; const element = this.getHostElement(); - // A focus listener calling back into `focus` must not scroll a second time. - const wasFocused = element.ownerDocument.activeElement === element; const focusOptions: FocusOptions = { ...options, preventScroll: true }; if (this.focusMonitor && origin) { @@ -153,7 +151,7 @@ export class KbqDropdownItem implements KbqTitleTextRef, KbqDropdownItemActionHo } // With the pointer already on this item, revealing it would shift the list out from under it. - if (origin !== 'mouse' && !wasFocused) { + if (origin !== 'mouse') { element.scrollIntoView?.({ block: 'nearest', inline: 'nearest' }); } diff --git a/packages/components/dropdown/dropdown.component.ts b/packages/components/dropdown/dropdown.component.ts index 4af9d733d1..c68f9ef6c1 100644 --- a/packages/components/dropdown/dropdown.component.ts +++ b/packages/components/dropdown/dropdown.component.ts @@ -592,7 +592,7 @@ export class KbqDropdown implements AfterContentInit, KbqDropdownPanel, OnInit, // same as `setFirstItemActive` no-ops in that case. const panel = this.directDescendantItems.first?.getHostElement().closest('.kbq-dropdown__panel'); - panel?.focus(); + panel?.focus({ preventScroll: true }); } /** diff --git a/packages/components/dropdown/e2e.webkit.playwright-spec.ts b/packages/components/dropdown/e2e.webkit.playwright-spec.ts index 743695548c..4d9abdf43f 100644 --- a/packages/components/dropdown/e2e.webkit.playwright-spec.ts +++ b/packages/components/dropdown/e2e.webkit.playwright-spec.ts @@ -28,6 +28,8 @@ test.describe('KbqDropdown panel scrolling', () => { await e2eRecordOptionFocusOptions(page); await page.goto('/E2eDropdownScrollbar'); await page.getByTestId('e2eDropdownScrollbarTrigger').click(); + // The routes fit the viewport, so without this the page-scroll assertion below could never fail. + await page.addStyleTag({ content: 'body { min-height: 3000px; }' }); await expect(getPort(page)).toBeVisible(); await expect(getPort(page).locator('.kbq-dropdown-item').first()).toBeVisible(); }); @@ -52,11 +54,13 @@ test.describe('KbqDropdown panel scrolling', () => { test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { const port = getPort(page); + const start = await e2eScrollTopOf(port); + await port.hover(); await page.mouse.wheel(0, 200); - // The gesture is applied over several frames; wait for the offset itself rather than a frame count. - const scrolled = await e2eWaitForScrollEnd(port); + // The gesture is applied over several frames; wait for the offset to move and then settle. + const scrolled = await e2eWaitForScrollEnd(port, start); expect(scrolled).toBeGreaterThan(0); @@ -107,6 +111,7 @@ test.describe('KbqDropdown panel scrolling', () => { expect(await e2eScrollTopOf(port)).toBeGreaterThan(0); expect(await e2eIsFullyInView(port, '.kbq-dropdown-item:focus')).toBe(true); + // The page is deliberately made scrollable in beforeEach, so this can actually fail. expect(await page.evaluate(() => window.scrollY)).toBe(0); }); }); diff --git a/packages/components/select/e2e.webkit.playwright-spec.ts b/packages/components/select/e2e.webkit.playwright-spec.ts index 96ab6d2b44..19eed325af 100644 --- a/packages/components/select/e2e.webkit.playwright-spec.ts +++ b/packages/components/select/e2e.webkit.playwright-spec.ts @@ -28,6 +28,8 @@ test.describe('KbqSelect panel scrolling', () => { await e2eRecordOptionFocusOptions(page); await page.goto('/E2eSelectScrollbar'); await page.getByTestId('e2eSelect').click(); + // The routes fit the viewport, so without this the page-scroll assertion below could never fail. + await page.addStyleTag({ content: 'body { min-height: 3000px; }' }); await expect(getPort(page)).toBeVisible(); await expect(getPort(page).locator('.kbq-option').first()).toBeVisible(); }); @@ -52,11 +54,13 @@ test.describe('KbqSelect panel scrolling', () => { test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { const port = getPort(page); + const start = await e2eScrollTopOf(port); + await port.hover(); await page.mouse.wheel(0, 200); - // The gesture is applied over several frames; wait for the offset itself rather than a frame count. - const scrolled = await e2eWaitForScrollEnd(port); + // The gesture is applied over several frames; wait for the offset to move and then settle. + const scrolled = await e2eWaitForScrollEnd(port, start); expect(scrolled).toBeGreaterThan(0); @@ -107,6 +111,7 @@ test.describe('KbqSelect panel scrolling', () => { expect(await e2eScrollTopOf(port)).toBeGreaterThan(0); expect(await e2eIsFullyInView(port, '.kbq-option.kbq-active')).toBe(true); + // The page is deliberately made scrollable in beforeEach, so this can actually fail. expect(await page.evaluate(() => window.scrollY)).toBe(0); }); }); diff --git a/packages/components/select/select.component.ts b/packages/components/select/select.component.ts index f6ef8dd5af..46c6cfebdf 100644 --- a/packages/components/select/select.component.ts +++ b/packages/components/select/select.component.ts @@ -2251,7 +2251,6 @@ export class KbqSelect } } - /** Focuses the active option, which reveals it — see `KbqOption.focus`. */ private scrollActiveOptionIntoView(): void { this.keyManager.activeItem?.focus(); } diff --git a/packages/components/tree-select/e2e.webkit.playwright-spec.ts b/packages/components/tree-select/e2e.webkit.playwright-spec.ts index 55fcf3ed8e..7617e689fa 100644 --- a/packages/components/tree-select/e2e.webkit.playwright-spec.ts +++ b/packages/components/tree-select/e2e.webkit.playwright-spec.ts @@ -28,6 +28,8 @@ test.describe('KbqTreeSelect panel scrolling', () => { await e2eRecordOptionFocusOptions(page); await page.goto('/E2eTreeSelectScrollbar'); await page.getByTestId('e2eTreeSelect').click(); + // The routes fit the viewport, so without this the page-scroll assertion below could never fail. + await page.addStyleTag({ content: 'body { min-height: 3000px; }' }); await expect(getPort(page)).toBeVisible(); await expect(getPort(page).locator('.kbq-tree-option').first()).toBeVisible(); }); @@ -52,11 +54,13 @@ test.describe('KbqTreeSelect panel scrolling', () => { test('scrolls with the wheel and stays where the reader left it', async ({ page }) => { const port = getPort(page); + const start = await e2eScrollTopOf(port); + await port.hover(); await page.mouse.wheel(0, 200); - // The gesture is applied over several frames; wait for the offset itself rather than a frame count. - const scrolled = await e2eWaitForScrollEnd(port); + // The gesture is applied over several frames; wait for the offset to move and then settle. + const scrolled = await e2eWaitForScrollEnd(port, start); expect(scrolled).toBeGreaterThan(0); @@ -107,6 +111,7 @@ test.describe('KbqTreeSelect panel scrolling', () => { expect(await e2eScrollTopOf(port)).toBeGreaterThan(0); expect(await e2eIsFullyInView(port, '.kbq-tree-option:focus')).toBe(true); + // The page is deliberately made scrollable in beforeEach, so this can actually fail. expect(await page.evaluate(() => window.scrollY)).toBe(0); }); }); diff --git a/packages/components/tree-select/tree-select.component.ts b/packages/components/tree-select/tree-select.component.ts index 87c766d427..b6d9310f82 100644 --- a/packages/components/tree-select/tree-select.component.ts +++ b/packages/components/tree-select/tree-select.component.ts @@ -783,7 +783,7 @@ export class KbqTreeSelect private originalOnKeyDown: (event: KeyboardEvent) => void; - /** The scroll position of the overlay panel, calculated to center the selected option. */ + /** The scroll offset the panel is restored to when it attaches — the list always opens at the top. */ private scrollTop = 0; /** Unique id for this input. */ @@ -1654,7 +1654,6 @@ export class KbqTreeSelect } } - /** Focuses the active node, which reveals it — see `KbqTreeOption.focus`. */ private scrollActiveOptionIntoView() { this.tree()!.keyManager.activeItem?.focus(); } diff --git a/packages/components/tree/tree-option.component.ts b/packages/components/tree/tree-option.component.ts index 51e6efe1b4..2d4c5f5cad 100644 --- a/packages/components/tree/tree-option.component.ts +++ b/packages/components/tree/tree-option.component.ts @@ -24,6 +24,7 @@ import { KBQ_OPTION_ACTION_PARENT, KBQ_TITLE_TEXT_REF, KbqActionContainer, + kbqFocusAndReveal, kbqFocusOptionActionOnTab, KbqOptionActionComponent, KbqPseudoCheckbox, @@ -323,18 +324,8 @@ export class KbqTreeOption extends KbqTreeNode implements AfterCo focus(focusOrigin?: FocusOrigin) { if (focusOrigin === 'program' || this.disabled || this.actionButton()?.hasFocus) return; - const element = this.elementRef.nativeElement; - // A focus listener calling back into `focus` must not scroll a second time. - const wasFocused = element.ownerDocument.activeElement === element; - - // The reveal is explicit because the one focus performs implicitly is not portable: WebKit defers - // it to a later rendering update, where it lands after — and undoes — the reader's own scrolling. - element.focus({ preventScroll: true }); - // With the pointer already on this node, revealing it would shift the tree out from under it. - if (focusOrigin !== 'mouse' && !wasFocused) { - element.scrollIntoView?.({ block: 'nearest', inline: 'nearest' }); - } + kbqFocusAndReveal(this.elementRef.nativeElement, focusOrigin === 'mouse'); if (!this.hasFocus) { this.onFocus.next({ option: this }); diff --git a/packages/e2e/utils/focus-scroll.ts b/packages/e2e/utils/focus-scroll.ts index ed9a8b5290..6b324954a1 100644 --- a/packages/e2e/utils/focus-scroll.ts +++ b/packages/e2e/utils/focus-scroll.ts @@ -59,37 +59,39 @@ export const e2eScrollTopOf = (scrollport: Locator): Promise => * A wheel gesture is applied over several frames, so reading the offset a fixed number of frames later * catches an intermediate value — the reason to wait for the value itself rather than for a frame count. */ -export const e2eWaitForScrollEnd = (scrollport: Locator): Promise => +export const e2eWaitForScrollEnd = (scrollport: Locator, from: number): Promise => scrollport.evaluate( - (element) => + (element, start) => new Promise((resolve) => { const stableFramesNeeded = 3; - // Bounds the wait so a continuously animating scrollport fails on the assertion, not a hang. - const maxFrames = 120; let previous = element.scrollTop; let stableFrames = 0; - let frames = 0; const check = () => { - if (element.scrollTop === previous) { + // Waiting only for stability would resolve `start` unchanged when the gesture has not + // been applied yet — `mouse.wheel` does not wait for that — and report "never scrolled". + if (element.scrollTop !== start && element.scrollTop === previous) { stableFrames++; } else { stableFrames = 0; previous = element.scrollTop; } - if (stableFrames >= stableFramesNeeded || ++frames >= maxFrames) { + if (stableFrames >= stableFramesNeeded) { resolve(previous); return; } + // No frame cap: Playwright's own test timeout reports a stuck scrollport with a far + // clearer message than a mid-flight offset that silently satisfies the assertion. requestAnimationFrame(check); }; requestAnimationFrame(check); - }) + }), + from ); /** Whether `selector`'s match inside `scrollport` is fully within it. Null when nothing matches. */ diff --git a/tools/public_api_guard/components/autocomplete.api.md b/tools/public_api_guard/components/autocomplete.api.md index 962dfc8967..2ee5b4947a 100644 --- a/tools/public_api_guard/components/autocomplete.api.md +++ b/tools/public_api_guard/components/autocomplete.api.md @@ -92,7 +92,6 @@ export class KbqAutocomplete implements AfterContentInit { readonly panelMaxWidth: _angular_core.InputSignalWithTransform; readonly panelMinWidth: _angular_core.InputSignalWithTransform; readonly panelWidth: _angular_core.InputSignal; - scrollActiveOptionIntoView(): void; // (undocumented) setScrollTop(scrollTop: number): void; // (undocumented) diff --git a/tools/public_api_guard/components/core.api.md b/tools/public_api_guard/components/core.api.md index 39d3d40328..26b1471f16 100644 --- a/tools/public_api_guard/components/core.api.md +++ b/tools/public_api_guard/components/core.api.md @@ -3020,6 +3020,9 @@ export type KbqFlexDirection = 'row' | 'column'; // @public export type KbqFlexWrap = 'nowrap' | 'wrap'; +// @public +export const kbqFocusAndReveal: (element: HTMLElement, skipReveal?: boolean) => void; + // @public export function kbqFocusOptionActionOnTab($event: KeyboardEvent, actionButton: KbqOptionActionComponent | undefined): void; @@ -3483,6 +3486,7 @@ export class KbqOption extends KbqOptionBase implements AfterViewChecked, OnDest // (undocumented) ngOnDestroy(): void; protected onMouseenter(): void; + protected onMouseleave(): void; readonly onSelectionChange: EventEmitter>; // (undocumented) protected readonly parent: KbqOptionParentComponent; From 53db3cc8e4c353ae7ace98926d7c92196cb2eafb Mon Sep 17 00:00:00 2001 From: lskramarov Date: Wed, 2 Sep 2026 17:03:30 +0300 Subject: [PATCH 7/7] fix(e2e): await addInitScript so the shared utils type-check (#DS-3299) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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` 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. --- packages/e2e/utils/focus-scroll.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/e2e/utils/focus-scroll.ts b/packages/e2e/utils/focus-scroll.ts index 6b324954a1..bbda4fcbb5 100644 --- a/packages/e2e/utils/focus-scroll.ts +++ b/packages/e2e/utils/focus-scroll.ts @@ -17,8 +17,9 @@ type FocusScrollWindow = Window & { * just the ones a spec triggers afterwards. Call before `page.goto`, then read the flags back with * {@link e2eReadOptionFocusOptions}. */ -export const e2eRecordOptionFocusOptions = (page: Page): Promise => - page.addInitScript(() => { +export const e2eRecordOptionFocusOptions = async (page: Page): Promise => { + // Awaited rather than returned: `addInitScript` resolves to a Disposable — see `e2eDisableResizeObserver`. + await page.addInitScript(() => { const target = window as FocusScrollWindow; target.__kbqOptionFocusOptions = []; @@ -38,6 +39,7 @@ export const e2eRecordOptionFocusOptions = (page: Page): Promise => return originalFocus.call(this, options); }; }); +}; /** Reads back what {@link e2eRecordOptionFocusOptions} captured: one `preventScroll` flag per focus call. */ export const e2eReadOptionFocusOptions = (page: Page): Promise =>