From df4d9e5ff27ccbc92ee7a3b0e94fe82cb6ce2f35 Mon Sep 17 00:00:00 2001 From: Yigit Gurbulak Date: Fri, 9 Oct 2026 05:27:09 +0300 Subject: [PATCH 1/2] fix(nav): focus the section after a cross-route nav tap Closes #274 Co-Authored-By: Claude Opus 5.5 --- components/site/Chrome.tsx | 41 +++++++++++++++------- components/site/MobileMenu.tsx | 47 +++++++++++++++---------- lib/scroll.test.ts | 63 +++++++++++++++++++++++++++++++++- lib/scroll.ts | 38 ++++++++++++++++++++ 4 files changed, 156 insertions(+), 33 deletions(-) diff --git a/components/site/Chrome.tsx b/components/site/Chrome.tsx index 9be3b17..3c8a062 100644 --- a/components/site/Chrome.tsx +++ b/components/site/Chrome.tsx @@ -6,8 +6,11 @@ import { usePathname } from "next/navigation"; import { links } from "@/lib/links"; import { HOME_SECTION_ID, + focusSection, homeNavTarget, + queueSectionFocus, shouldInterceptNavClick, + takeSectionFocus, } from "@/lib/scroll"; import { MobileMenu } from "@/components/site/MobileMenu"; import { resolveActiveSection } from "@/components/site/chrome-active"; @@ -56,8 +59,10 @@ export function Chrome() { // hash (`/#method#method…`, #116) under output: "export" + // trailingSlash; Next 16.3.8 fixed that upstream. Off this route the // section isn't in the DOM (e.g. /blog/*), so the native - // soft-navs home and scrolls to the hash itself. Modified clicks (new - // tab, middle-click) also fall through to the native . + // soft-navs home and scrolls to the hash itself; the id is queued so + // the arrival effect below moves focus into the section once home has + // committed. Modified clicks (new tab, middle-click) also fall through + // to the native . const handleNavClick = useCallback( (e: React.MouseEvent) => { if (!shouldInterceptNavClick(e)) return; @@ -89,8 +94,11 @@ export function Chrome() { const { id } = target; const el = document.getElementById(id); // Off this route: fall through to the native , as the top - // branch does. - if (el === null) return; + // branch does, and focus the section on arrival. + if (el === null) { + queueSectionFocus(id); + return; + } e.preventDefault(); // Write the exact single hash so it can never accumulate; // replaceState keeps a same-hash click a URL no-op. @@ -101,15 +109,9 @@ export function Chrome() { // desktop, so the drawer's custom rAF isn't needed. el.scrollIntoView({ block: "start" }); // Unlike a real anchor nav, the intercepted scroll doesn't move - // keyboard / SR focus — without this, tab order and the reading - // cursor stay stuck on the nav. Add tabindex only when the target - // isn't already focusable (don't pull a focusable el out of tab - // order); leaving the injected tabindex="-1" in place is the - // established programmatic-focus pattern (focusable via script, - // not via Tab). preventScroll so .focus() doesn't run its own - // scroll and race the scrollIntoView above. - if (el.tabIndex < 0) el.setAttribute("tabindex", "-1"); - el.focus({ preventScroll: true }); + // keyboard / SR focus; focusSection does, without racing the + // scrollIntoView above. + focusSection(el); }, [], ); @@ -198,6 +200,19 @@ export function Chrome() { return () => window.removeEventListener("scroll", onScroll); }, [pathname]); + // Cross-route arrival: a section tap from another route (this nav or + // the drawer) queued its id before soft-navving to `/#id`. Router + // navigations commit the new page together with the new pathname, and + // this passive effect runs after Next's layout-phase hash scroll, so + // the section is in the DOM and already at the viewport top. Same-page + // taps focus directly and never queue. + useEffect(() => { + const id = takeSectionFocus(window.location.hash); + if (id === null) return; + const el = document.getElementById(id); + if (el) focusSection(el); + }, [pathname]); + const effectiveActive = resolveActiveSection( pathname, scrolled, diff --git a/components/site/MobileMenu.tsx b/components/site/MobileMenu.tsx index 416d3ae..72390e0 100644 --- a/components/site/MobileMenu.tsx +++ b/components/site/MobileMenu.tsx @@ -10,7 +10,12 @@ import { import { createPortal } from "react-dom"; import { useRouter } from "next/navigation"; import { links } from "@/lib/links"; -import { HOME_SECTION_ID, scrollToAnchor } from "@/lib/scroll"; +import { + HOME_SECTION_ID, + focusSection, + queueSectionFocus, + scrollToAnchor, +} from "@/lib/scroll"; type SectionId = "intro" | "compare" | "method" | "faq" | "contact"; @@ -81,11 +86,12 @@ export function MobileMenu({ activeSection, tone, onOpenChange }: Props) { // the prior scroll position — otherwise the body unlock would override // the anchor jump. const pendingAnchorRef = useRef(null); - // An in-page anchor-select close (target in this DOM) moves focus into - // the target section from the scroll-lock effect's cleanup, which runs - // *before* the toggle-refocus effect's setup in the same commit. This - // flag — set there, consumed there — stops that effect from yanking - // focus back to the hamburger. + // A section-select close moves focus into the target section — at once + // from the scroll-lock effect's cleanup when the target is in this DOM, + // or after the route commits (Chrome's arrival effect) when it isn't. + // That cleanup runs *before* the toggle-refocus effect's setup in the + // same commit. This flag — set there, consumed there — stops that + // effect from yanking focus back to the hamburger. const focusMovedToSectionRef = useRef(false); useEffect(() => { @@ -180,8 +186,15 @@ export function MobileMenu({ activeSection, tone, onOpenChange }: Props) { // no-op against a still-pinned body. Historically a full reload, // because soft-nav hash handling under output: "export" was // untrusted; its one concrete failure, `/#id#id` (#116), was fixed - // upstream in Next 16.3.8. Skip the local restore and focus move — - // the router owns scroll and the URL. + // upstream in Next 16.3.8. Skip the local restore — the router + // owns scroll and the URL. A section can't be focused until the + // route commits, so queue it for Chrome's arrival effect and keep + // the toggle-refocus effect off it; home leaves focus on the + // toggle. + if (!isHome) { + queueSectionFocus(anchor); + focusMovedToSectionRef.current = true; + } router.push(isHome ? "/" : `/#${anchor}`); return; } @@ -215,13 +228,9 @@ export function MobileMenu({ activeSection, tone, onOpenChange }: Props) { history.replaceState(null, "", `#${anchor}`); scrollToAnchor(targetEl); // Move keyboard / SR focus into the section — scrollToAnchor - // only moves the viewport. Add tabindex only when the target - // isn't already focusable (don't pull a focusable el out of - // tab order); leaving the injected tabindex="-1" in place is - // the established programmatic-focus pattern. preventScroll so - // .focus() doesn't race the in-flight rAF scroll. - if (targetEl.tabIndex < 0) targetEl.setAttribute("tabindex", "-1"); - targetEl.focus({ preventScroll: true }); + // only moves the viewport. focusSection won't race the + // in-flight rAF scroll. + focusSection(targetEl); focusMovedToSectionRef.current = true; } }; @@ -232,10 +241,10 @@ export function MobileMenu({ activeSection, tone, onOpenChange }: Props) { const wasOpenRef = useRef(false); useEffect(() => { if (wasOpenRef.current && !open) { - // An anchor-select close already moved focus into the section; - // leave it there. Normal closes (Escape, scrim, toggle, resize) and - // an off-route anchor close (soft-navs home) leave the flag false - // and still return focus to the toggle. + // A section-select close already moved focus into the section, or + // will once the route commits; leave it alone. Normal closes + // (Escape, scrim, toggle, resize) and a home-select close leave the + // flag false and still return focus to the toggle. if (focusMovedToSectionRef.current) { focusMovedToSectionRef.current = false; } else { diff --git a/lib/scroll.test.ts b/lib/scroll.test.ts index e588e27..47b916a 100644 --- a/lib/scroll.test.ts +++ b/lib/scroll.test.ts @@ -1,9 +1,12 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { HOME_SECTION_ID, + focusSection, homeNavTarget, parseScrollMarginTop, + queueSectionFocus, shouldInterceptNavClick, + takeSectionFocus, } from "./scroll"; describe("homeNavTarget", () => { @@ -134,3 +137,61 @@ describe("shouldInterceptNavClick", () => { ); }); }); + +describe("focusSection", () => { + const fakeEl = (tabIndex: number) => ({ + tabIndex, + setAttribute: vi.fn(), + focus: vi.fn(), + }); + + it("makes a non-focusable section programmatically focusable", () => { + const el = fakeEl(-1); + focusSection(el); + expect(el.setAttribute).toHaveBeenCalledWith("tabindex", "-1"); + }); + + it("leaves an already-focusable element in the tab order", () => { + const el = fakeEl(0); + focusSection(el); + expect(el.setAttribute).not.toHaveBeenCalled(); + }); + + it("focuses without scrolling, so it can't race the nav's scroll", () => { + for (const el of [fakeEl(-1), fakeEl(0)]) { + focusSection(el); + expect(el.focus).toHaveBeenCalledWith({ preventScroll: true }); + } + }); +}); + +describe("queueSectionFocus / takeSectionFocus", () => { + it("returns nothing when no section is queued", () => { + expect(takeSectionFocus("#method")).toBeNull(); + }); + + it("returns the queued id when the URL landed on its hash", () => { + queueSectionFocus("method"); + expect(takeSectionFocus("#method")).toBe("method"); + }); + + it("is single-use", () => { + queueSectionFocus("method"); + takeSectionFocus("#method"); + expect(takeSectionFocus("#method")).toBeNull(); + }); + + it("ignores a stale id on an arrival with a different hash", () => { + // e.g. an aborted section nav followed by a home nav (no hash). + queueSectionFocus("method"); + expect(takeSectionFocus("")).toBeNull(); + queueSectionFocus("method"); + expect(takeSectionFocus("#faq")).toBeNull(); + }); + + it("clears the queue even when the hash doesn't match", () => { + queueSectionFocus("method"); + takeSectionFocus(""); + expect(takeSectionFocus("#method")).toBeNull(); + }); +}); diff --git a/lib/scroll.ts b/lib/scroll.ts index e7c5c66..6e63043 100644 --- a/lib/scroll.ts +++ b/lib/scroll.ts @@ -55,6 +55,44 @@ export function shouldInterceptNavClick(e: { ); } +// Moves keyboard / SR focus into a section after a nav tap — scrolling +// alone only moves the viewport, so tab order and the reading cursor +// would stay on the nav. Add tabindex only when the target isn't already +// focusable (don't pull a focusable el out of tab order); leaving the +// injected tabindex="-1" in place is the established programmatic-focus +// pattern (focusable via script, not via Tab). preventScroll so .focus() +// doesn't run its own scroll and race whichever scroll got us here. +// Typed structurally so it's unit-testable without a DOM. +export function focusSection( + el: Pick, +) { + if (el.tabIndex < 0) el.setAttribute("tabindex", "-1"); + el.focus({ preventScroll: true }); +} + +// A section tap from another route (e.g. /blog/*) soft-navs to `/#id`, +// so the target isn't in the DOM yet when the nav is tapped. The tap +// queues the id here; Chrome — mounted in the root layout, so it +// survives the route change — takes it in a pathname effect once the +// home page has committed and focuses the section with focusSection. +// Next's own hash scroll also calls .focus() on the target, but that's a +// no-op on a section without tabindex. +let pendingSectionFocus: string | null = null; + +export function queueSectionFocus(id: string) { + pendingSectionFocus = id; +} + +// Single-use: always clears the queue. Returns the id only when the URL +// actually landed on that hash, so a queued id that outlives an aborted +// nav can't focus a section on a later, unrelated arrival (e.g. home, +// which lands on `/` with no hash). +export function takeSectionFocus(hash: string): string | null { + const id = pendingSectionFocus; + pendingSectionFocus = null; + return id !== null && hash === `#${id}` ? id : null; +} + // `` scroll-behavior is forced to `auto` for the lifetime of // the rAF: without it, each per-frame `window.scrollTo` defers to // `motion-safe:scroll-smooth` and queues yet another browser From 4a719c4f55e3c0edf50093e121c4ebc335e86e18 Mon Sep 17 00:00:00 2001 From: Yigit Gurbulak Date: Fri, 9 Oct 2026 05:37:58 +0300 Subject: [PATCH 2/2] fix(nav): correct focus comments and tighten section-focus tests Co-Authored-By: Claude Opus 5.5 --- components/site/Chrome.tsx | 2 +- components/site/MobileMenu.tsx | 14 ++++++++------ lib/scroll.test.ts | 30 +++++++++++++++++++++++++++++- lib/scroll.ts | 5 +++-- 4 files changed, 41 insertions(+), 10 deletions(-) diff --git a/components/site/Chrome.tsx b/components/site/Chrome.tsx index 3c8a062..3358c05 100644 --- a/components/site/Chrome.tsx +++ b/components/site/Chrome.tsx @@ -204,7 +204,7 @@ export function Chrome() { // the drawer) queued its id before soft-navving to `/#id`. Router // navigations commit the new page together with the new pathname, and // this passive effect runs after Next's layout-phase hash scroll, so - // the section is in the DOM and already at the viewport top. Same-page + // the section is in the DOM and already scrolled into place. Same-page // taps focus directly and never queue. useEffect(() => { const id = takeSectionFocus(window.location.hash); diff --git a/components/site/MobileMenu.tsx b/components/site/MobileMenu.tsx index 72390e0..c6bb0cd 100644 --- a/components/site/MobileMenu.tsx +++ b/components/site/MobileMenu.tsx @@ -90,8 +90,9 @@ export function MobileMenu({ activeSection, tone, onOpenChange }: Props) { // from the scroll-lock effect's cleanup when the target is in this DOM, // or after the route commits (Chrome's arrival effect) when it isn't. // That cleanup runs *before* the toggle-refocus effect's setup in the - // same commit. This flag — set there, consumed there — stops that - // effect from yanking focus back to the hamburger. + // same commit. This flag — set in that cleanup, consumed by the + // toggle-refocus effect — stops it from yanking focus back to the + // hamburger. const focusMovedToSectionRef = useRef(false); useEffect(() => { @@ -188,9 +189,10 @@ export function MobileMenu({ activeSection, tone, onOpenChange }: Props) { // untrusted; its one concrete failure, `/#id#id` (#116), was fixed // upstream in Next 16.3.8. Skip the local restore — the router // owns scroll and the URL. A section can't be focused until the - // route commits, so queue it for Chrome's arrival effect and keep - // the toggle-refocus effect off it; home leaves focus on the - // toggle. + // route commits, so queue it for Chrome's arrival effect and set + // the flag so the toggle-refocus effect doesn't grab focus + // meanwhile; a home tap leaves the flag false, so focus returns + // to the toggle. if (!isHome) { queueSectionFocus(anchor); focusMovedToSectionRef.current = true; @@ -242,7 +244,7 @@ export function MobileMenu({ activeSection, tone, onOpenChange }: Props) { useEffect(() => { if (wasOpenRef.current && !open) { // A section-select close already moved focus into the section, or - // will once the route commits; leave it alone. Normal closes + // will once the route lands on its hash; leave it alone. Normal closes // (Escape, scrim, toggle, resize) and a home-select close leave the // flag false and still return focus to the toggle. if (focusMovedToSectionRef.current) { diff --git a/lib/scroll.test.ts b/lib/scroll.test.ts index 47b916a..61c1b39 100644 --- a/lib/scroll.test.ts +++ b/lib/scroll.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { HOME_SECTION_ID, focusSection, @@ -151,6 +151,14 @@ describe("focusSection", () => { expect(el.setAttribute).toHaveBeenCalledWith("tabindex", "-1"); }); + it("adds the tabindex before focusing (focus is a no-op without it)", () => { + const el = fakeEl(-1); + focusSection(el); + expect(el.setAttribute.mock.invocationCallOrder[0]).toBeLessThan( + el.focus.mock.invocationCallOrder[0]!, + ); + }); + it("leaves an already-focusable element in the tab order", () => { const el = fakeEl(0); focusSection(el); @@ -166,6 +174,12 @@ describe("focusSection", () => { }); describe("queueSectionFocus / takeSectionFocus", () => { + // The queue is module state: drain it so no case depends on another + // having taken what it queued. + beforeEach(() => { + takeSectionFocus(""); + }); + it("returns nothing when no section is queued", () => { expect(takeSectionFocus("#method")).toBeNull(); }); @@ -189,6 +203,20 @@ describe("queueSectionFocus / takeSectionFocus", () => { expect(takeSectionFocus("#faq")).toBeNull(); }); + it("keeps only the latest queued section", () => { + queueSectionFocus("method"); + queueSectionFocus("faq"); + expect(takeSectionFocus("#faq")).toBe("faq"); + queueSectionFocus("method"); + queueSectionFocus("faq"); + expect(takeSectionFocus("#method")).toBeNull(); + }); + + it("matches the hash exactly", () => { + queueSectionFocus("method"); + expect(takeSectionFocus("#methodology")).toBeNull(); + }); + it("clears the queue even when the hash doesn't match", () => { queueSectionFocus("method"); takeSectionFocus(""); diff --git a/lib/scroll.ts b/lib/scroll.ts index 6e63043..867af26 100644 --- a/lib/scroll.ts +++ b/lib/scroll.ts @@ -75,8 +75,9 @@ export function focusSection( // queues the id here; Chrome — mounted in the root layout, so it // survives the route change — takes it in a pathname effect once the // home page has committed and focuses the section with focusSection. -// Next's own hash scroll also calls .focus() on the target, but that's a -// no-op on a section without tabindex. +// Next's hash scroll doesn't move focus — 16.3.8's default +// appNewScrollHandler leaves it untouched — so without this, focus stays +// on the tapped link (or the drawer). let pendingSectionFocus: string | null = null; export function queueSectionFocus(id: string) {