Skip to content

fix: keep anchor links aligned after layout changes#2731

Open
apples-kksk wants to merge 10 commits into
docsifyjs:developfrom
apples-kksk:fix-anchor-scroll-after-layout
Open

fix: keep anchor links aligned after layout changes#2731
apples-kksk wants to merge 10 commits into
docsifyjs:developfrom
apples-kksk:fix-anchor-scroll-after-layout

Conversation

@apples-kksk
Copy link
Copy Markdown

@apples-kksk apples-kksk commented May 9, 2026

Summary

Direct links like #/?id=target can land too early when images or other content above the heading are still changing the page height. This keeps the requested heading aligned for a short settling window after the first scroll, and stops doing that as soon as the user starts interacting with the page.

I also added a Playwright regression that delays an image above the target heading and checks that the heading still ends up near the top of the viewport.

Related issue, if any:

Fixes #351

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

For any code change,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed

Does this PR introduce a breaking change?

  • Yes
  • No

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge

Checked locally with npm run lint, npm run build:js, npm run test:e2e:chromium -- anchor-scroll.test.js, and git diff --check.

@vercel
Copy link
Copy Markdown

vercel Bot commented May 9, 2026

@apples-kksk is attempting to deploy a commit to the Docsify Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes anchor deep-link scrolling (#/?id=...) landing too early when late-loading content (e.g., images) above the target heading changes layout after the initial scroll, by re-aligning the target for a short settling window and stopping once the user interacts.

Changes:

  • Refactors anchor navigation scrolling into a dedicated #scrollToHeading() helper that re-syncs scroll position during a brief post-scroll window using ResizeObserver.
  • Adds a Playwright E2E regression test that simulates a delayed image above the anchor target.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
test/e2e/anchor-scroll.test.js Adds an E2E regression test for anchor alignment when an image above the target loads late.
src/core/event/index.js Implements post-scroll re-alignment logic for anchor targets during late layout changes and cancels on user interaction.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/e2e/anchor-scroll.test.js Outdated
Comment thread src/core/event/index.js Outdated
Comment thread src/core/event/index.js Outdated
@apples-kksk
Copy link
Copy Markdown
Author

Addressed the review feedback in cf22daf: the regression test now waits for the delayed image to finish loading before checking the target, anchor resync stops after cancellation, and instant resyncs no longer register extra scroll watchers.\n\nVerified with:\n- npx prettier --check src/core/event/index.js test/e2e/anchor-scroll.test.js\n- npx eslint src/core/event/index.js test/e2e/anchor-scroll.test.js\n- npm run build:js\n- npm run test:e2e:chromium -- anchor-scroll.test.js

@vercel
Copy link
Copy Markdown

vercel Bot commented May 13, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docsify-preview Ready Ready Preview, Comment May 15, 2026 8:20am

@paulhibbitts
Copy link
Copy Markdown
Member

paulhibbitts commented May 13, 2026

Thanks for the PR @apples-kksk ! To serve as a first test, I created two CodeSandboxes with a page containing lots of images and anchor links - one using the latest RC and one using the preview build from this PR. Browser DevTools are open in both so cache is disabled.

Docsify RC CodeSandbox:
https://codesandbox.io/p/sandbox/docsify-v5-page-anchor-scroll-test-rc-vhzr7s
https://vhzr7s.csb.app/#/anchors-page?id=target-theta

Docsify PR Build CodeSandbox:
https://codesandbox.io/p/sandbox/docsify-v5-page-anchor-scroll-test-pr-qrfqnc
https://qrfqnc.csb.app/#/anchors-page?id=target-theta

Initial tests look promising - the correct visual position is reached for anchors deep in the page with the PR preview build. However, the smooth scroll animation is no longer present when clicking anchor links on the same page. Was this intentional? The animation provides a useful visual cue that the user is staying on the same page rather than navigating away.

@sy-records - I wonder if perhaps before addressing this somewhat larger PR a release of v5.0 is done first, then loop through PRs like this one for a subsequent release? Or are you thinking perhaps of a v5 release after this PR?

@apples-kksk
Copy link
Copy Markdown
Author

Thanks for testing this. The missing smooth scroll was not intentional. I updated the resync logic so the initial same-page anchor scroll remains smooth, and delayed the instant correction until after the smooth scroll settles or a short fallback window expires. I also added a regression test for same-page anchor clicks.

@paulhibbitts
Copy link
Copy Markdown
Member

Thanks @apples-kksk , I will re-test once a new Preview Build is available.

@sy-records
Copy link
Copy Markdown
Member

@paulhibbitts I’ll release v5 first.

@apples-kksk
Copy link
Copy Markdown
Author

apples-kksk commented May 14, 2026

I pushed a small CI-only diagnostic commit to investigate the Windows-only test:consume-types failure.

The anchor-scroll code itself was not changed. The Windows consumption test is now split into install/typecheck steps and will print npm debug logs, the installed local docsify package info, and an --ignore-scripts comparison if the install step fails.

Because this commit modifies .github/workflows/test.yml from a fork PR, the new GitHub Actions run is currently marked action_required and may need maintainer approval before it can run.

@sy-records
Copy link
Copy Markdown
Member

You don't need to submit it; you can test it in your own repository.

@apples-kksk
Copy link
Copy Markdown
Author

Understood. I reverted the CI-only diagnostic commit from this PR and will keep that investigation in my own fork/local environment instead.

@apples-kksk
Copy link
Copy Markdown
Author

Thanks, I kept the CI diagnostics out of this PR and ran them in my fork instead:
https://github.com/apples-kksk/docsify/actions/runs/25841605949

That run passed, including test-jest on lts/* / windows-latest. The split Windows test:consume-types steps also passed for both npm clean-install --install-links and npm run typecheck.

I reverted the CI-only diagnostic workflow change from this PR, so the PR diff is focused on the anchor-scroll fix and regression tests again.

@paulhibbitts
Copy link
Copy Markdown
Member

That sounds great @sy-records , please let me know if I can do anything else to help. I will continue to test PRs as things proceed.

@paulhibbitts
Copy link
Copy Markdown
Member

paulhibbitts commented May 14, 2026

Thanks for the updated PR @apples-kksk ! I've updated the PR build sandbox and the scroll behavior has returned.

The trade-off as currently shown is that after the initial smooth scroll there is a "jump", which can be seen with the below demo link 'target-theta'. Browser DevTools should be open in both so cache is disabled.

Docsify RC CodeSandbox:
https://codesandbox.io/p/sandbox/docsify-v5-page-anchor-scroll-test-rc-vhzr7s
https://vhzr7s.csb.app/#/anchors-page?id=target-theta

Docsify PR Build CodeSandbox:
https://codesandbox.io/p/sandbox/docsify-v5-page-anchor-scroll-test-pr-qrfqnc
https://qrfqnc.csb.app/#/anchors-page?id=target-theta

With Browser cache disabled, go to the provided target-theta link and then do a hard-refresh in the Browser.

Is this an issue that can also be addressed @apples-kksk ? If not, accuracy of the target link overweighs this visual issue to me - what do other folks think? @apples-kksk @sy-records @Koooooo-7 @trusktr ?

@apples-kksk
Copy link
Copy Markdown
Author

Thanks for the detailed repro. I can reproduce the hard-refresh jump.

The target alignment fix is working, but the delayed correction after late layout changes is currently using an instant scroll, which causes the visible jump after the initial smooth scroll. I tested switching that delayed correction to smooth scrolling: the anchor still ends aligned, and the large post-load jump disappears.

I’ll add/adjust e2e coverage for this hard-refresh path and update the PR.

@apples-kksk
Copy link
Copy Markdown
Author

Thanks for checking this and for the sandbox links.

I pushed another update that addresses the visual jump after the initial smooth scroll. The delayed anchor realignment now keeps using smooth scrolling instead of switching to an instant correction, so late layout changes above the target should preserve the smoother behavior while still ending aligned to the requested heading.

I also added/updated e2e coverage for the cases that caused regressions here:

  • direct hard-refresh anchor loads with delayed images above the target
  • spaced late layout shifts after the first corrective realignment
  • avoiding an instant delayed scrollIntoView correction
  • not interrupting an in-flight smooth correction during observer cleanup
  • stopping corrective scrolling after user input

I verified the final patch with the full fork CI, including Playwright on Chromium/Firefox/WebKit: https://github.com/apples-kksk/docsify/actions/runs/25899870161

@paulhibbitts
Copy link
Copy Markdown
Member

paulhibbitts commented May 15, 2026

Thanks for the updated PR @apples-kksk . After comparing the updated scroll version the best path forward is not as clear, as while the latest version does scroll as compared to jump you can see the first scroll, a pause, and then a second scroll.

I've made another codesandbox, so we can now compare the two:

Jump:
https://qrfqnc.csb.app/#/anchors-page?id=target-iota

Smooth scroll:
https://vfrj9p.csb.app/#/anchors-page?id=target-iota

As this behaviour is a key aspect of Docsify, we need to strive to choose the one with the best user experience. Is it possible to minimize/remove this pause between the two scrolls @apples-kksk ? If not, considering all the factors present, I am now tending to lean towards the scroll/jump behavior vs. the scroll/pause/scroll behavior (overall visual / time delay) - thoughts?

@apples-kksk
Copy link
Copy Markdown
Author

Thanks, I see what you mean. I'll look into whether the second adjustment can be handled within the initial scroll, or otherwise minimized, without bringing back the visible jump.

@apples-kksk
Copy link
Copy Markdown
Author

Paul, I pushed 6ebd086 with a different approach for the hard-refresh/direct hash path.

Instead of starting the anchor scroll and then correcting after images load, it now waits briefly for still-loading images above the target, then does one smooth scrollIntoView. If the user interacts before that pending scroll starts, the pending anchor scroll is cancelled.

I retested a fresh no-cache preview in Chromium and Firefox and no longer saw the scroll/pause/scroll or pull-back behavior. Full local Playwright e2e passed, and the fork Build & Test run is green:
https://github.com/apples-kksk/docsify/actions/runs/25939046947

Could you retry the direct hash-load case when you have a chance and see whether this feels better than the previous two versions? The upstream Actions/Vercel checks may still need maintainer authorization for this fork PR.

@paulhibbitts
Copy link
Copy Markdown
Member

Thanks very much @apples-kksk , that sounds very promising! Once @sy-records can approve a new deployment I will re-test and share the results back here. Thanks again for considering all the feedback so far 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Direct link to sub-item not scrolling to correct position

4 participants