Skip to content

Commit ca65338

Browse files
authored
fix(core): scroll-spy race conditions on rapid/smooth scroll (#2968)
* 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 <noreply@anthropic.com> * docs: add changeset Assisted-By: Claude Opus 4.6 <noreply@anthropic.com> * 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 <noreply@anthropic.com>
1 parent 8b88e14 commit ca65338

3 files changed

Lines changed: 401 additions & 9 deletions

File tree

.changeset/fix-scroll-spy-race.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
"@patternfly/pfe-core": patch
3+
---
4+
`ScrollSpyController`: fix race conditions on rapid and smooth scroll navigation
5+
6+
- Fix rapid clicks leaving stale force-release listeners that caused the active
7+
state to fall "one click behind"
8+
- Release force on `scrollend` instead of first IntersectionObserver callback,
9+
preventing intermediate sections from stealing active state during smooth scroll
10+
- Sort passed links by DOM order instead of Set insertion order, fixing incorrect
11+
active state with non-contiguous content sections

core/pfe-core/controllers/scroll-spy-controller.ts

Lines changed: 47 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,12 @@ export class ScrollSpyController implements ReactiveController {
6363
/** Ignore intersections? */
6464
#force = false;
6565

66+
/** AbortController to cancel previous force-release listeners */
67+
#forceAbort?: AbortController;
68+
69+
/** Timeout handle for force-release safety valve */
70+
#forceTimeout?: ReturnType<typeof setTimeout>;
71+
6672
/** Has the intersection observer found an element? */
6773
#intersected = false;
6874

@@ -144,10 +150,23 @@ export class ScrollSpyController implements ReactiveController {
144150
hostDisconnected(): void {
145151
ScrollSpyController.#instances.delete(this);
146152
this.#io?.disconnect();
153+
this.#releaseForce();
147154
}
148155

149156
#initializing = true;
150157

158+
/** Cancel force mode and clean up associated listeners */
159+
#releaseForce() {
160+
if (!this.#force) {
161+
return;
162+
}
163+
this.#force = false;
164+
this.#forceAbort?.abort();
165+
this.#forceAbort = undefined;
166+
clearTimeout(this.#forceTimeout);
167+
this.#forceTimeout = undefined;
168+
}
169+
151170
async #initIo() {
152171
const rootNode = this.#getRootNode();
153172
if (rootNode instanceof Document || rootNode instanceof ShadowRoot) {
@@ -193,25 +212,34 @@ export class ScrollSpyController implements ReactiveController {
193212

194213
async #nextIntersection() {
195214
this.#intersected = false;
196-
// safeguard the loop
197-
setTimeout(() => this.#intersected = false, 3000);
215+
// safeguard: break the loop after 3s even if no intersection fires
216+
const timer = setTimeout(() => this.#intersected = true, 3000);
198217
while (!this.#intersected) {
199218
await new Promise(requestAnimationFrame);
200219
}
220+
clearTimeout(timer);
201221
}
202222

203223
async #onIo(entries: IntersectionObserverEntry[]) {
204224
if (!this.#force) {
205-
for (const { target, boundingClientRect, intersectionRect } of entries) {
225+
for (const entry of entries) {
226+
const { target, boundingClientRect } = entry;
206227
const selector = `:is(${this.#tagNames.join(',')})[href="#${target.id}"]`;
207228
const link = this.host.querySelector(selector);
208229
if (link) {
209-
this.#markPassed(link, boundingClientRect.top < intersectionRect.top);
230+
// Mark as passed if the element's top has reached the root's top edge.
231+
// Using rootBounds (not intersectionRect) so that elements exactly AT the
232+
// viewport top are correctly considered "passed" (the current section).
233+
const rootTop = entry.rootBounds?.top ?? 0;
234+
this.#markPassed(link, boundingClientRect.top <= rootTop + 2);
210235
}
211236
}
212-
const link = [...this.#passedLinks];
213-
const last = link.at(-1);
214-
this.#setActive(last ?? this.#linkChildren.at(0));
237+
// Sort passed links by DOM order rather than Set insertion order
238+
const linkOrder = this.#linkChildren;
239+
const passed = [...this.#passedLinks]
240+
.sort((a, b) => linkOrder.indexOf(a) - linkOrder.indexOf(b));
241+
const last = passed.at(-1);
242+
this.#setActive(last ?? linkOrder.at(0));
215243
}
216244
this.#intersected = true;
217245
this.#intersectingTargets.clear();
@@ -242,16 +270,26 @@ export class ScrollSpyController implements ReactiveController {
242270
* @param link usually an `<a>`
243271
*/
244272
public async setActive(link: EventTarget | null): Promise<void> {
273+
// Cancel any previous programmatic scroll's force state
274+
this.#forceAbort?.abort();
275+
clearTimeout(this.#forceTimeout);
276+
245277
this.#force = true;
246278
this.#setActive(link);
279+
247280
let sawActive = false;
248281
for (const child of this.#linkChildren) {
249282
this.#markPassed(child, !sawActive);
250283
if (child === link) {
251284
sawActive = true;
252285
}
253286
}
254-
await this.#nextIntersection();
255-
this.#force = false;
287+
288+
// Force is released when the scroll completes (scrollend event),
289+
// or after a 3-second safety timeout
290+
this.#forceAbort = new AbortController();
291+
const { signal } = this.#forceAbort;
292+
addEventListener('scrollend', () => this.#releaseForce(), { once: true, signal });
293+
this.#forceTimeout = setTimeout(() => this.#releaseForce(), 3000);
256294
}
257295
}

0 commit comments

Comments
 (0)