Skip to content

fix(client): honor scroll-margin-top when scrolling to hash - #1724

Open
Mister-Hope wants to merge 1 commit into
mainfrom
fix/hash-scroll-margin-top
Open

Mister-Hope wants to merge 1 commit into
mainfrom
fix/hash-scroll-margin-top

Conversation

@Mister-Hope

Copy link
Copy Markdown
Member

Summary

Hash navigation ignores scroll-margin-top, so themes cannot keep anchor targets clear of a fixed navbar with it.

The router scrolls with { el: to.hash }, and vue-router resolves that element itself (elRect.top - docRect.top + window.scrollTo) instead of using scrollIntoView, so scroll-margin-top / scroll-padding-top have no effect. Themes are left with a negative margin-top paired with a matching padding-top, which shifts the box (including its padding) above the element and makes the content above it unclickable.

Changes

  • packages/client: scroll hash targets with scrollIntoView and return false, so scroll-margin-top is honored.
    • The scroll is deferred to the next frame, which also fixes hash links pointing to another page: the target element has not been rendered yet when the scroll behavior is called, so those links never scrolled at all.
    • getElementById is used instead of querySelector, as an id may contain characters that are invalid in a CSS selector (e.g. the space in #foo bar or the : in #foo:1).

Tests

Added e2e/tests/router/scroll-to-hash.spec.ts with two pages whose targets have scroll-margin-top: 100px, covering:

  • a hash on the same page,
  • a hash containing special characters (#target:1),
  • a hash on another page,
  • a hash on another page reached by a markdown link,
  • navigating to another page without a hash (still scrolls to the top).

All 5 fail without the change (the target ends up at the top of the viewport, or the page does not scroll at all) and pass with it.

Verification

  • pnpm lint, pnpm check-types, pnpm test:unit (666 tests) pass.
  • pnpm e2e:dev (86 tests) and pnpm e2e:build (93 tests) pass.

@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 36333404826

Coverage remained the same at 73.948%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 1087
Covered Lines: 800
Line Coverage: 73.6%
Relevant Branches: 552
Covered Branches: 412
Branch Coverage: 74.64%
Branches in Coverage %: Yes
Coverage Strength: 48.34 hits per line

💛 - Coveralls

This branch has not been deployed

No deployments
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.

2 participants