fix(nav): PR #118 follow-ups (#119, #120, #121)

[yigitdot]

Resolves the three standalone follow-ups filed off the PR #118 review. Each is a pre-existing weakness deliberately deferred so #118 stayed scoped to the #116 hash-accumulation fix — none is a regression introduced by #118. Conventional commits, bisectable.

shouldInterceptNavClick extraction — Closes #120

Extracts the 6-flag modifier short-circuit from Chrome.tsx's handleNavClick into a pure shouldInterceptNavClick predicate in lib/scroll.ts, mirroring the #114→#117 precedent. Adds a lib/scroll.test.ts block: plain left-click → true; defaultPrevented / middle-click / each modifier → false. Zero new infra (Node-env Vitest, existing glob).

Ease scrollToAnchor + preserve horizontal scroll — Closes #121

scrollToAnchor now uses a quadratic ease-in-out instead of linear, and reads window.scrollX live each rAF frame (instead of the original hard-coded 0, which would yank horizontal scroll to the left edge). Live-read — not a captured startX — is the deliberate choice: the loop only drives Y, so re-reading X each frame is a no-op pass-through that never fights the user (see review threads). Reduced-motion early-return, single-flight cancel/restore, and the distance === 0 short-circuit are unchanged. Only caller is the mobile drawer's close-then-scroll.

Move focus to section on in-page nav jump — Closes #119

The intercepted same-page jump scrolled the section into view but never moved keyboard / screen-reader focus, leaving tab order and the SR reading cursor stuck on the nav. Both call sites (desktop handleNavClick, mobile drawer cleanup) now set tabindex="-1" (guarded by if (el.tabIndex < 0) so an already-focusable target isn't pulled out of tab order) and .focus({ preventScroll: true }). On mobile, an additional focusMovedToSectionRef flag stops the toggle-refocus effect from yanking focus back to the hamburger after an anchor-close (its setup runs after the cleanup that moves focus, in the same commit) — without it the mobile focus move was a no-op. No CSS guard needed — globals.css uses :focus-visible only, which doesn't fire for programmatic focus.

Review follow-ups

• Addressed gemini-code-assist inline review (scrollX live-read accepted; tabindex guard adopted; blur-removal cleanup declined on merit with rationale on-thread).
• /review-pr multi-agent pass caught a Critical regression: the mobile chrome(nav): desktop hash-anchor click does not move focus to the section #119 focus was being clobbered by the existing toggle-refocus effect — fixed (the focusMovedToSectionRef flag above), plus comment-accuracy polish.
• scroll: scrollToAnchor jumps to top when scrollMarginTop computes to NaN #124 filed for a pre-existing NaN scrollMarginTop → scroll-to-top footgun (moved verbatim from fix: prevent nav hash duplication on repeated click → reload → click #118; out of scope here, not fixed in this PR).

Verification

• pnpm test — 90 passed (incl. shouldInterceptNavClick cases)
• pnpm lint / pnpm format:check — clean
• pnpm build — static export succeeds

Not auto-verifiable (needs manual / AT testing): keyboard tab-order + VoiceOver focus landing for #119 (desktop and mobile drawer, incl. that Escape/scrim still returns focus to the toggle), and the eased scroll feel for #121.

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying website with    Cloudflare Pages

Latest commit:	18ea0b1
Status:	✅  Deploy successful!
Preview URL:	https://8ca35177.website-70y.pages.dev
Branch Preview URL:	https://fix-nav-followups-118.website-70y.pages.dev

View logs

[Copilot]

Pull request overview

This PR addresses three follow-ups from the PR #118 review by improving (1) testability of the nav-click interception guard, (2) smooth-scroll ergonomics in the mobile drawer’s custom anchor scroll, and (3) accessibility by ensuring focus follows in-page navigation.

Changes:

• Extracted the “modified click” short-circuit into a pure shouldInterceptNavClick helper and added unit tests for it.
• Updated scrollToAnchor to use quadratic ease-in-out and to preserve horizontal scroll by pinning scrollX during the animation.
• After intercepted in-page navigation, moved keyboard/screen-reader focus into the destination section (desktop + mobile drawer).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
lib/scroll.ts	Adds shouldInterceptNavClick; improves scrollToAnchor easing and preserves horizontal scroll during animation.
lib/scroll.test.ts	Adds unit tests covering shouldInterceptNavClick cases.
components/site/MobileMenu.tsx	Moves focus to the target section after the drawer closes and scrollToAnchor starts.
components/site/Chrome.tsx	Replaces inline click-guard with shouldInterceptNavClick and moves focus to the section after same-page nav interception.

---

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

[gemini-code-assist]

Code Review

This pull request refactors navigation click interception into a shared utility, adds unit tests, and enhances the custom smooth-scrolling implementation with quadratic easing. It also introduces accessibility improvements by shifting focus to target sections after scrolling. Feedback focuses on maintaining horizontal scroll responsiveness by using window.scrollX instead of a captured value and improving tabindex handling to ensure that already focusable elements are not negatively impacted.

[gemini-code-assist]

Using a captured startX locks the horizontal scroll position for the duration of the animation, which can feel restrictive and "stiff" if the user attempts to scroll horizontally. Using window.scrollX directly in each frame preserves the current horizontal position while remaining responsive to user input, providing a more natural feel. This also makes the startX variable at line 71 unnecessary.

Suggested change

	window.scrollTo(startX, startY + distance * ease);
	window.scrollTo(window.scrollX, startY + distance * ease);

References

1. When implementing custom smooth-scrolling, use easing functions (such as quadratic ease-in-out) and preserve the horizontal scroll position (window.scrollX) to ensure a natural feel and prevent horizontal jumping.

[yigitdot]

Accepted — fixed in 2eb08cd. You're right: startY is captured because it's the animation origin the loop interpolates from, but X isn't animated, so a captured startX re-asserted X every frame and would fight a horizontal scroll. Now reading window.scrollX live each tick (and the startX variable is gone). This also matches what #121 originally specified.

[gemini-code-assist]

Unconditionally setting tabindex="-1" can remove elements that are already focusable (e.g., those with tabindex="0") from the natural tab order. It is safer to only apply it to non-focusable elements and clean it up on blur to avoid leaving unnecessary attributes in the DOM.

      if (el.tabIndex < 0) {
        el.setAttribute("tabindex", "-1");
        el.addEventListener("blur", () => el.removeAttribute("tabindex"), { once: true });
      }
      el.focus({ preventScroll: true });

[yigitdot]

Partially adopted in d6ffafb. Took the guard — tabindex is now set only if (el.tabIndex < 0), so an already-focusable target can't be pulled out of tab order.

Declined the blur-removal cleanup, on merit: every nav target here renders through components/ui/Frame.tsx as a plain non-focusable <section>, and leaving tabindex="-1" on a focus-target region is the recommended WAI-ARIA-APG pattern for post-navigation focus — not stale state. A per-navigation blur listener that adds/removes the attribute each time is churn without a user-facing benefit.

[gemini-code-assist]

Unconditionally setting tabindex="-1" can remove elements that are already focusable from the natural tab order. Consider applying it only when necessary and cleaning up on blur to maintain a clean DOM state.

        if (targetEl.tabIndex < 0) {
          targetEl.setAttribute("tabindex", "-1");
          targetEl.addEventListener("blur", () => targetEl.removeAttribute("tabindex"), { once: true });
        }
        targetEl.focus({ preventScroll: true });

[yigitdot]

Same resolution as the Chrome.tsx thread — guard adopted in d6ffafb (if (targetEl.tabIndex < 0)); blur-removal cleanup declined for the same reason (target is a plain <section> via Frame.tsx; tabindex="-1" left on a focus-target region is the standard WAI-ARIA-APG pattern, and per-nav blur listeners are unnecessary churn).

[yigitdot]

/gemini review

[gemini-code-assist]

Code Review

This pull request improves accessibility by ensuring keyboard and screen reader focus is correctly moved to target sections after programmatic scrolls in Chrome.tsx and MobileMenu.tsx. It also refactors navigation click interception into a testable utility and upgrades the custom scroll animation to use quadratic easing. Feedback was provided to capture the initial horizontal scroll position at the start of the animation to prevent potential horizontal jumping during the scroll loop.

[gemini-code-assist]

The implementation currently uses window.scrollX live in each frame, which can cause horizontal jumping if the layout shifts or the user interacts with the page during the animation. To ensure a natural feel and strictly preserve the horizontal scroll position as per repository rules, capture the initial scrollX value (startX) at the beginning of the animation and use that fixed value in the scroll loop.

Suggested change

	window.scrollTo(window.scrollX, startY + distance * ease);
	window.scrollTo(startX, startY + distance * ease);

References

1. When implementing custom smooth-scrolling, use easing functions (such as quadratic ease-in-out) and preserve the horizontal scroll position (window.scrollX) to ensure a natural feel and prevent horizontal jumping.

[yigitdot]

Declining this — it reverses the change you correctly recommended in #122 (comment), which we accepted and shipped in 2eb08cd.

The reasoning here is inverted. Reading window.scrollX live each frame reads the current horizontal offset and writes it straight back — a no-op pass-through that never moves X and never fights the user. A captured startX is the opposite: it re-asserts the start offset on every one of ~36 frames, which is exactly the "horizontal jumping when the user interacts" this comment is trying to avoid. The cited rule ("preserve the horizontal scroll position (window.scrollX)") is satisfied by the shipped code.

Also concretely: startX was deleted in 2eb08cd, so the suggested window.scrollTo(startX, …) would not type-check.

Keeping window.scrollTo(window.scrollX, …) as-is. No code change.

[yigitdot]

/review-pr follow-up pushed (11368d7, 18ea0b1): fixed a Critical regression where the mobile #119 focus-into-section was clobbered every close by the toggle-refocus effect (its setup runs after the focus-moving cleanup in the same commit) — now gated by a focusMovedToSectionRef flag so anchor-close keeps focus in the section while Escape/scrim/toggle/resize still return focus to the hamburger. Also comment-accuracy polish. Pre-existing NaN scrollMarginTop footgun (not introduced here) tracked separately as #124.