From 5b46cf6b1de91201682443054b912e1a004274c8 Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Mon, 9 Mar 2026 01:35:57 +0200 Subject: [PATCH 1/3] fix(core): scroll-spy race conditions on rapid/smooth scroll navigation Fixes three bugs in ScrollSpyController: 1. Rapid clicks: setActive() now cancels previous force state via AbortController before establishing new force, preventing stale listeners from releasing force prematurely. 2. Force release timing: replaced await-first-intersection approach with scrollend event listener (+ 3s safety timeout). The old approach released force when any IO fired during smooth scroll, causing intermediate sections to steal the active state. 3. Non-contiguous sections: passedLinks are now sorted by DOM order instead of relying on Set insertion order, which was unreliable when sections had untracked content between them. Also fixes a safeguard bug where the #nextIntersection timeout set `#intersected = false` (should be `true`), which could cause an infinite rAF loop, and fixes the boundary comparison to use rootBounds.top instead of intersectionRect.top so elements exactly at the viewport top are correctly considered "passed". Closes #2911 Closes #2920 Assisted-By: Claude Opus 4.6 --- .../controllers/scroll-spy-controller.ts | 55 ++- .../test/scroll-spy-controller.spec.ts | 343 ++++++++++++++++++ 2 files changed, 389 insertions(+), 9 deletions(-) create mode 100644 core/pfe-core/test/scroll-spy-controller.spec.ts diff --git a/core/pfe-core/controllers/scroll-spy-controller.ts b/core/pfe-core/controllers/scroll-spy-controller.ts index 5edd597f7a..acd9f731ea 100644 --- a/core/pfe-core/controllers/scroll-spy-controller.ts +++ b/core/pfe-core/controllers/scroll-spy-controller.ts @@ -63,6 +63,12 @@ export class ScrollSpyController implements ReactiveController { /** Ignore intersections? */ #force = false; + /** AbortController to cancel previous force-release listeners */ + #forceAbort?: AbortController; + + /** Timeout handle for force-release safety valve */ + #forceTimeout?: ReturnType; + /** Has the intersection observer found an element? */ #intersected = false; @@ -144,10 +150,23 @@ export class ScrollSpyController implements ReactiveController { hostDisconnected(): void { ScrollSpyController.#instances.delete(this); this.#io?.disconnect(); + this.#releaseForce(); } #initializing = true; + /** Cancel force mode and clean up associated listeners */ + #releaseForce() { + if (!this.#force) { + return; + } + this.#force = false; + this.#forceAbort?.abort(); + this.#forceAbort = undefined; + clearTimeout(this.#forceTimeout); + this.#forceTimeout = undefined; + } + async #initIo() { const rootNode = this.#getRootNode(); if (rootNode instanceof Document || rootNode instanceof ShadowRoot) { @@ -193,8 +212,8 @@ export class ScrollSpyController implements ReactiveController { async #nextIntersection() { this.#intersected = false; - // safeguard the loop - setTimeout(() => this.#intersected = false, 3000); + // safeguard: break the loop after 3s even if no intersection fires + setTimeout(() => this.#intersected = true, 3000); while (!this.#intersected) { await new Promise(requestAnimationFrame); } @@ -202,16 +221,24 @@ export class ScrollSpyController implements ReactiveController { async #onIo(entries: IntersectionObserverEntry[]) { if (!this.#force) { - for (const { target, boundingClientRect, intersectionRect } of entries) { + for (const entry of entries) { + const { target, boundingClientRect } = entry; const selector = `:is(${this.#tagNames.join(',')})[href="#${target.id}"]`; const link = this.host.querySelector(selector); if (link) { - this.#markPassed(link, boundingClientRect.top < intersectionRect.top); + // Mark as passed if the element's top has reached the root's top edge. + // Using rootBounds (not intersectionRect) so that elements exactly AT the + // viewport top are correctly considered "passed" (the current section). + const rootTop = entry.rootBounds?.top ?? 0; + this.#markPassed(link, boundingClientRect.top <= rootTop + 2); } } - const link = [...this.#passedLinks]; - const last = link.at(-1); - this.#setActive(last ?? this.#linkChildren.at(0)); + // Sort passed links by DOM order rather than Set insertion order + const linkOrder = this.#linkChildren; + const passed = [...this.#passedLinks] + .sort((a, b) => linkOrder.indexOf(a) - linkOrder.indexOf(b)); + const last = passed.at(-1); + this.#setActive(last ?? linkOrder.at(0)); } this.#intersected = true; this.#intersectingTargets.clear(); @@ -242,8 +269,13 @@ export class ScrollSpyController implements ReactiveController { * @param link usually an `` */ public async setActive(link: EventTarget | null): Promise { + // Cancel any previous programmatic scroll's force state + this.#forceAbort?.abort(); + clearTimeout(this.#forceTimeout); + this.#force = true; this.#setActive(link); + let sawActive = false; for (const child of this.#linkChildren) { this.#markPassed(child, !sawActive); @@ -251,7 +283,12 @@ export class ScrollSpyController implements ReactiveController { sawActive = true; } } - await this.#nextIntersection(); - this.#force = false; + + // Force is released when the scroll completes (scrollend event), + // or after a 3-second safety timeout + this.#forceAbort = new AbortController(); + const { signal } = this.#forceAbort; + addEventListener('scrollend', () => this.#releaseForce(), { once: true, signal }); + this.#forceTimeout = setTimeout(() => this.#releaseForce(), 3000); } } diff --git a/core/pfe-core/test/scroll-spy-controller.spec.ts b/core/pfe-core/test/scroll-spy-controller.spec.ts new file mode 100644 index 0000000000..2ee7b42df7 --- /dev/null +++ b/core/pfe-core/test/scroll-spy-controller.spec.ts @@ -0,0 +1,343 @@ +import { expect, html, nextFrame, aTimeout } from '@open-wc/testing'; +import { createFixture } from '@patternfly/pfe-tools/test/create-fixture.js'; +import { setViewport } from '@web/test-runner-commands'; +import { allUpdates } from '@patternfly/pfe-tools/test/utils.js'; +import { LitElement, type TemplateResult } from 'lit'; +import { customElement } from 'lit/decorators/custom-element.js'; +import { property } from 'lit/decorators/property.js'; +import { unsafeHTML } from 'lit/directives/unsafe-html.js'; + +import { ScrollSpyController } from '@patternfly/pfe-core/controllers/scroll-spy-controller.js'; + +// Minimal host element to test ScrollSpyController in isolation +@customElement('test-scroll-spy') +class TestScrollSpy extends LitElement { + #spy = new ScrollSpyController(this, { + rootMargin: '0px 0px 0px 0px', + tagNames: ['test-scroll-spy-link'], + onIntersection: () => { + this.lastIntersection = Date.now(); + }, + }); + + lastIntersection = 0; + + override createRenderRoot() { + return this; + } + + render(): TemplateResult { + return html``; + } + + setActive(link: Element) { + this.#spy.setActive(link); + } +} + +@customElement('test-scroll-spy-link') +class TestScrollSpyLink extends LitElement { + @property({ reflect: true }) href?: string; + @property({ type: Boolean, reflect: true }) active = false; + + override createRenderRoot() { + return this; + } + + render(): TemplateResult { + return html``; + } +} + +/** + * Generate a content section with enough height to require scrolling + * @param id section element id + * @param title section heading text + * @param height section height in pixels + */ +function section(id: string, title: string, height = 1000): string { + return `

${title}

`; +} + +/** + * Wait for a scrollend event or timeout + * @param timeout max wait in ms + */ +async function waitForScrollEnd(timeout = 2000): Promise { + await new Promise(resolve => { + const timer = setTimeout(resolve, timeout); + addEventListener('scrollend', () => { + clearTimeout(timer); + resolve(); + }, { once: true }); + }); +} + +/** + * Get the currently active link text + * @param host the test scroll spy element + */ +function getActiveLink(host: TestScrollSpy): string | null { + const active = host.querySelector('test-scroll-spy-link[active]'); + return active?.textContent?.trim() ?? null; +} + +describe('ScrollSpyController', function() { + // These tests involve real scrolling and IO, give them time + // eslint-disable-next-line @typescript-eslint/no-invalid-this + this.timeout(15000); + + beforeEach(async function() { + await setViewport({ width: 1024, height: 768 }); + window.scrollTo({ top: 0, behavior: 'instant' }); + await nextFrame(); + }); + + afterEach(async function() { + window.scrollTo({ top: 0, behavior: 'instant' }); + await nextFrame(); + }); + + describe('basic scroll tracking', function() { + let host: TestScrollSpy; + + beforeEach(async function() { + const container = await createFixture(html` +
+ + Section 1 + Section 2 + Section 3 + Section 4 + Section 5 + +
+ ${unsafeHTML(section('s1', 'Section 1'))} + ${unsafeHTML(section('s2', 'Section 2'))} + ${unsafeHTML(section('s3', 'Section 3'))} + ${unsafeHTML(section('s4', 'Section 4'))} + ${unsafeHTML(section('s5', 'Section 5'))} +
+
+ `); + host = container.querySelector('test-scroll-spy')!; + await allUpdates(host); + await nextFrame(); + await nextFrame(); + }); + + it('should activate first link on initial load', async function() { + // IO needs a frame to fire + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 1'); + }); + + it('should update active link when scrolling down', async function() { + const s3 = document.getElementById('s3')!; + s3.scrollIntoView({ behavior: 'instant' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 3'); + }); + + it('should update active link when scrolling back up', async function() { + // Scroll down first + const s4 = document.getElementById('s4')!; + s4.scrollIntoView({ behavior: 'instant' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 4'); + + // Scroll back up + const s2 = document.getElementById('s2')!; + s2.scrollIntoView({ behavior: 'instant' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 2'); + }); + }); + + describe('programmatic setActive (click simulation)', function() { + let host: TestScrollSpy; + let links: TestScrollSpyLink[]; + + beforeEach(async function() { + const container = await createFixture(html` +
+ + Section 1 + Section 2 + Section 3 + Section 4 + Section 5 + +
+ ${unsafeHTML(section('s1', 'Section 1'))} + ${unsafeHTML(section('s2', 'Section 2'))} + ${unsafeHTML(section('s3', 'Section 3'))} + ${unsafeHTML(section('s4', 'Section 4'))} + ${unsafeHTML(section('s5', 'Section 5'))} +
+
+ `); + host = container.querySelector('test-scroll-spy')!; + links = Array.from(host.querySelectorAll('test-scroll-spy-link')); + await allUpdates(host); + await nextFrame(); + await nextFrame(); + }); + + // Reproduces #2425: clicking a link far down the list with smooth scroll + // should not activate an intermediate section + it('should maintain active state during smooth scroll to distant section', async function() { + // Simulate clicking section 5 (far away) + host.setActive(links[4]); + expect(getActiveLink(host)).to.equal('Section 5'); + + // Scroll to section 5 with smooth behavior (as browser would on anchor click) + document.getElementById('s5')!.scrollIntoView({ behavior: 'smooth' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + + // Active should still be section 5, NOT an intermediate section + expect(getActiveLink(host)).to.equal('Section 5'); + }); + + // Reproduces #2425: active state getting "one click behind" + it('should not fall one click behind on rapid navigation', async function() { + // Click section 3 + host.setActive(links[2]); + document.getElementById('s3')!.scrollIntoView({ behavior: 'instant' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 3'); + + // Now click section 5 + host.setActive(links[4]); + document.getElementById('s5')!.scrollIntoView({ behavior: 'instant' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + + // Should be section 5, not section 3 + expect(getActiveLink(host)).to.equal('Section 5'); + }); + + // Reproduces #2425: rapid clicks should cancel previous force state + it('should handle rapid successive clicks correctly', async function() { + // Click 2, then immediately click 4 + host.setActive(links[1]); + document.getElementById('s2')!.scrollIntoView({ behavior: 'smooth' }); + await aTimeout(50); + + // Before scroll completes, click section 4 + host.setActive(links[3]); + document.getElementById('s4')!.scrollIntoView({ behavior: 'smooth' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + + expect(getActiveLink(host)).to.equal('Section 4'); + }); + }); + + describe('non-contiguous content sections', function() { + let host: TestScrollSpy; + + // Reproduces #2474: sections with non-tracked content between them + beforeEach(async function() { + const container = await createFixture(html` +
+ + Section 1 + Section 2 + Section 3 + +
+

Section 1

+
Section 1 content
+ + +
+

Untracked sidebar content

+

This content is NOT tracked by jump links

+
+ +

Section 2

+
Section 2 content
+ + +
+

Another untracked area

+
+ +

Section 3

+
Section 3 content
+
+
+ `); + host = container.querySelector('test-scroll-spy')!; + await allUpdates(host); + await nextFrame(); + await nextFrame(); + }); + + it('should correctly track sections separated by untracked content', async function() { + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 1'); + + document.getElementById('s2')!.scrollIntoView({ behavior: 'instant' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 2'); + + document.getElementById('s3')!.scrollIntoView({ behavior: 'instant' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 3'); + }); + + it('should correctly track when scrolling back through non-contiguous sections', async function() { + // Scroll all the way down + document.getElementById('s3')!.scrollIntoView({ behavior: 'instant' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 3'); + + // Scroll back to section 1 + document.getElementById('s1')!.scrollIntoView({ behavior: 'instant' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 1'); + }); + + it('should maintain correct active link when navigating via hash to non-contiguous section', async function() { + const links = Array.from(host.querySelectorAll('test-scroll-spy-link')); + + // Programmatically activate section 3 (simulating click) + host.setActive(links[2]); + document.getElementById('s3')!.scrollIntoView({ behavior: 'instant' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 3'); + + // Jump back to section 1 + host.setActive(links[0]); + document.getElementById('s1')!.scrollIntoView({ behavior: 'instant' }); + await waitForScrollEnd(); + await nextFrame(); + await aTimeout(100); + expect(getActiveLink(host)).to.equal('Section 1'); + }); + }); +}); From 0466ae547c00036236874463bc10feedc7c1ab35 Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Mon, 9 Mar 2026 01:39:21 +0200 Subject: [PATCH 2/3] docs: add changeset Assisted-By: Claude Opus 4.6 --- .changeset/fix-scroll-spy-race.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 .changeset/fix-scroll-spy-race.md diff --git a/.changeset/fix-scroll-spy-race.md b/.changeset/fix-scroll-spy-race.md new file mode 100644 index 0000000000..9f3c8d792d --- /dev/null +++ b/.changeset/fix-scroll-spy-race.md @@ -0,0 +1,11 @@ +--- +"@patternfly/pfe-core": patch +--- +`ScrollSpyController`: fix race conditions on rapid and smooth scroll navigation + +- Fix rapid clicks leaving stale force-release listeners that caused the active + state to fall "one click behind" +- Release force on `scrollend` instead of first IntersectionObserver callback, + preventing intermediate sections from stealing active state during smooth scroll +- Sort passed links by DOM order instead of Set insertion order, fixing incorrect + active state with non-contiguous content sections From fa1ed380b14b250bac25c8fabe74439015db059e Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Mon, 9 Mar 2026 01:43:20 +0200 Subject: [PATCH 3/3] fix(core): clear #nextIntersection safeguard timeout on exit Prevents a stale 3s timeout from prematurely resolving a subsequent #nextIntersection() call. Assisted-By: Claude Opus 4.6 --- core/pfe-core/controllers/scroll-spy-controller.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/pfe-core/controllers/scroll-spy-controller.ts b/core/pfe-core/controllers/scroll-spy-controller.ts index acd9f731ea..6630cdc4e4 100644 --- a/core/pfe-core/controllers/scroll-spy-controller.ts +++ b/core/pfe-core/controllers/scroll-spy-controller.ts @@ -213,10 +213,11 @@ export class ScrollSpyController implements ReactiveController { async #nextIntersection() { this.#intersected = false; // safeguard: break the loop after 3s even if no intersection fires - setTimeout(() => this.#intersected = true, 3000); + const timer = setTimeout(() => this.#intersected = true, 3000); while (!this.#intersected) { await new Promise(requestAnimationFrame); } + clearTimeout(timer); } async #onIo(entries: IntersectionObserverEntry[]) {