Skip to content

fix(core): prevent race condition on programmatic scroll#2911

Closed
brianferry wants to merge 2 commits intomainfrom
fix/scroll-spy-controller-race-condition
Closed

fix(core): prevent race condition on programmatic scroll#2911
brianferry wants to merge 2 commits intomainfrom
fix/scroll-spy-controller-race-condition

Conversation

@brianferry
Copy link
Collaborator

@brianferry brianferry commented Jun 11, 2025

What I did

ScrollSpyController

  1. Added a destinationTarget property for when an activeLink is set.
  2. When the destinationTarget comes into view, it sets the #force flag to false and clears the destinationTarget.

There is a race condition that is causing this issue RedHat-UX/red-hat-design-system#2425 where the rh-jump-link component gets out of sync during a programmatic scroll event.

What we're doing here is instead looking to see if we've arrived at the destinationTarget and that the destinationTarget is intersected before we continue on with the onIo action.

Testing Instructions

This is a fix that will be felt upstream in RHDS

  1. Build the project
  2. Link the project or overwrite your scroll-spy-controller.js files in rhds-elements
  3. Confirm that [bug] <rh-jump-links> Active state not consistent RedHat-UX/red-hat-design-system#2425 is not an issue for you anymore.

Notes to Reviewers

@netlify
Copy link

netlify bot commented Jun 11, 2025

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit b7e053e
😎 Deploy Preview https://deploy-preview-2911--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 11, 2025

✅ Commitlint tests passed!

More Info
{
  "valid": true,
  "errors": [],
  "warnings": [],
  "input": "fix(core): prevent race condition on programmatic scroll"
}

@github-actions github-actions bot added the AT passed Automated testing has passed label Jun 11, 2025
@github-actions

This comment has been minimized.

@bennypowers
Copy link
Member

see also #2920

@changeset-bot
Copy link

changeset-bot bot commented Feb 10, 2026

⚠️ No Changeset found

Latest commit: 62a64cd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

SSR Test Run for b7e053e: Report

bennypowers added a commit that referenced this pull request Mar 8, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AT passed Automated testing has passed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants