Skip to content

feat(nav): dj-navigate keeps aria-current="page" in sync across SPA nav (#1756)#1757

Merged
johnrtipton merged 2 commits into
mainfrom
feat/dj-navigate-aria-current-1756
Jun 8, 2026
Merged

feat(nav): dj-navigate keeps aria-current="page" in sync across SPA nav (#1756)#1757
johnrtipton merged 2 commits into
mainfrom
feat/dj-navigate-aria-current-1756

Conversation

@johnrtipton

Copy link
Copy Markdown
Contributor

Addresses #1756 (part 1 — the primary, titled ask). document.title and the [dj-root] dj-view-attr sync remain tracked there.

Problem

A persistent nav usually lives outside [dj-root]. dj-navigate swaps only the [dj-root] subtree, so a server-rendered active-link highlight ({% if slug == active_page %}…) never updates on SPA navigation — it stays on the page you came from. (Reported on djust.org: Examples → another page kept "Examples" highlighted; worked around there with a per-app script.)

Fix (framework, generic, accessible)

dj-navigate now manages aria-current="page" itself. After each navigation it sets aria-current="page" on the [dj-navigate] link whose path matches the current URL and removes it from the others. Three call sites in 18-navigation.js:

  • handleLiveRedirect (right after pushState) — instant feedback on click;
  • bindNavigationDirectives (runs via reinitAfterDOMUpdate) — covers the WS mount + patches + initial load;
  • the popstate handler — back/forward.

Apps just style a[dj-navigate][aria-current="page"]no per-app JS.

Guarantees:

  • Cross-origin dj-navigate targets (sister-site links whose pathname is /) are never marked current.
  • An app-authored aria-current of a different value (step, true, …) is left untouched — we only manage the "page" value we set.
  • Exact-path match (the ARIA-correct semantic for page); section/ancestor highlighting is left to the app.

Verification

  • tests/js/dj_navigate_aria_current_1756.test.js — 4 cases (match-moves-on-nav, cross-origin-never-current, app-value-not-clobbered, function exposed). Gate-off proven: neutering updateAriaCurrent fails 3/4.
  • Bundle rebuilt from src (make build-js); client.js + min/br/gz/map committed.
  • Docs: navigation.md gains an "Active-link highlighting" section.

Once released, djust.org can drop its nav-active.js workaround and style [aria-current="page"] instead.

🤖 Generated with Claude Code

johnrtipton and others added 2 commits June 7, 2026 22:24
…av (#1756)

A persistent nav usually lives OUTSIDE [dj-root], so dj-navigate's dj-root-only
swap never updated a server-rendered active-link highlight — it stayed on the
page you navigated from. The client now re-derives the active link from the URL
after each navigation (on dj-navigate click, on the WS mount via
bindNavigationDirectives/reinitAfterDOMUpdate, and on popstate), setting
aria-current="page" on the matching [dj-navigate] link and removing it from the
others. Cross-origin targets are never current; an app-authored aria-current of
a different value is left untouched. Apps style a[dj-navigate][aria-current="page"].

Adds tests/js/dj_navigate_aria_current_1756.test.js (4 cases, gate-off proven).
document.title + [dj-root] dj-view-attr sync across SPA nav remain in #1756.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the security-review PR touches security-sensitive code and needs additional review label Jun 8, 2026
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Security Review Required

This PR modifies security-sensitive files. Please ensure the following areas receive additional review:

  • Client JavaScript: changes to the browser-side runtime

Review Checklist

  • Changes do not introduce XSS vulnerabilities (no mark_safe(f'...'))
  • Authentication/authorization checks are not weakened
  • Input validation is maintained or improved
  • No secrets or sensitive data exposed in state
  • Existing security tests still pass
  • New security tests added for changed behavior

See SECURITY_GUIDELINES.md for full security review criteria.

@johnrtipton

Copy link
Copy Markdown
Contributor Author

Stage-7 Adversarial Review — PR #1757 (feat #1756)

Verdict: APPROVE 🟢

dj-navigate auto-syncs aria-current="page" across SPA navigation. Implementation is correct, well-scoped, defensively coded, and the test is non-tautological (gate-off verified). No 🔴, no 🟡.


Mandatory checks

Check Result
STALE-BASE (#250) 🟢 BEHIND=0 — branch is up to date with origin/main; origin/main..HEAD = exactly the 2 PR commits
Two-commit shape (#181) 🟢 c1 4fd72de6 = src + bundle (client.js/.min/.br/.gz/.map) + test, NO CHANGELOG/docs; c2 0a240454 = CHANGELOG + navigation.md only
Bundle integrity 🟢 client.js contains updateAriaCurrent (5 refs); make build-js produced no diff — committed bundle matches src
No unguarded console.log 🟢 none added in src/18-navigation.js

Logic correctness (updateAriaCurrent, src/18-navigation.js:456–473) — 🟢

  • Exact-path matchsetAttribute('aria-current','page') (L467–468). ✓
  • Don't-clobber (L469–470): non-match removes the attr only if it currently equals "page" — an app-authored aria-current="step"/"true" is preserved. ✓ (test dj_navigate_aria_current_1756.test.js:84 asserts this)
  • Cross-origin guard (L461 new URL(value, location.origin) + L466 dest.origin === location.origin): a dj-navigate="https://djustlive.com" link (pathname /) is never current, even on /. ✓ (test L76–81)
  • Malformed-value guard: new URL(...) in try/catch with return (L460–464) — skips just that element, loop continues. ✓
  • Idempotent: pure attribute set/remove based on current URL; running twice = same result. ✓

Call-site coverage — 🟢 (no stale-highlight gap)

The 3 wired sites are click (handleLiveRedirect L120, after pushState), mount (bindNavigationDirectives L445, via reinitAfterDOMUpdate), and popstate (L261). Traced every other nav path:

  • reinitAfterDOMUpdate chain confirmed: 09-event-binding.js:1436 reinitAfterDOMUpdate → bindLiveViewEvents:1439 → navigation.bindDirectives:1113 → updateAriaCurrent. Every VDOM patch / mount reaches it.
  • skipMountHtml (turbo-prerender) — COVERED. 03-websocket.js:355 branch calls reinitAfterDOMUpdate() inside runPostMount (L408, deferred via rAF / sync fallback). Not a gap.
  • live_patch / url_change (same-view query nav, _executePatch L373) — COVERED. _executePatch pushState's without an immediate updateAriaCurrent, BUT (a) for a same-path query-only change the active link doesn't change, and (b) the server reply (patch/html_update) routes 02-response-handler.js:159/187 reinitAfterDOMUpdate → updateAriaCurrent after the round-trip. No real nav path leaves the highlight stale.

Perf — 🟢 negligible

updateAriaCurrent runs on every bindNavigationDirectives (i.e. every reinitAfterDOMUpdate, every patch). Cost = one querySelectorAll('[dj-navigate]') + a new URL() per link. Nav links are few (single-digit to low-double-digit); the work is trivially small relative to the surrounding VDOM patch + hook remount that already runs in reinitAfterDOMUpdate. Not worth gating on URL-change. Stating as informational, not a concern.

Gate-off (#1200 / #254) — 🟢 non-tautological

Added return; to the top of updateAriaCurrent, make build-js, re-ran: 3 of 4 tests FAILED (only the "exposes function" smoke test passed). The 3 behavior tests genuinely exercise the change. Restored src + rebuilt — clean.

No-regression — 🟢

  • dj_navigate_aria_current_1756.test.js: 4/4 pass.
  • navigation.test.js + hooks.test.js: 45/45 pass. Existing dj-navigate/dj-patch behavior intact.

Test quality note (acceptable)

Tests call updateAriaCurrent() directly, not via a real click. The 3 wiring call-sites are therefore inspection-only (not exercised by an integration test). Acceptable for a 3-line wiring change where each site is a single function call adjacent to an existing pushState/reinit; the function-under-test itself is well covered. Flagging per #1196 transparency, not as a blocker.


🤖 Generated with Claude Code

@johnrtipton johnrtipton merged commit 0b708c0 into main Jun 8, 2026
14 checks passed
@johnrtipton johnrtipton deleted the feat/dj-navigate-aria-current-1756 branch June 8, 2026 02:32
@johnrtipton

Copy link
Copy Markdown
Contributor Author

Retrospective — PR #1757 (#1756 part 1)

Task: make dj-navigate auto-manage aria-current="page" so a persistent nav tracks the current page across SPA navigation — the framework generalization of the djust.org nav-active.js workaround shipped this session.

Quality: 5/5

Generalizes a real downstream fix into an accessible, zero-per-app-JS framework feature; gate-off-proven; all nav paths traced.

What went well

  • Downstream workaround → upstream feature. djust.org hit this live (Examples → page kept "Examples" highlighted); I fixed it there with a per-app script, then the same session lifted it into the framework as the standard aria-current="page" mechanism. Apps now style one attribute; no per-app JS. Same "lift-from-downstream then generalize" pattern as Action tech-debt: Lift-from-downstream FIRST pattern (canonicalize via Stage 4 plan-template) #1077.
  • Call-site coverage verified, not assumed. Stage-7 traced every nav path (click, WS mount incl. the skipMountHtml turbo-prerender branch, live_patch, popstate) to confirm none leaves the highlight stale — the single hook is reinitAfterDOMUpdate → bindNavigationDirectives → updateAriaCurrent, which every patch/mount already goes through.
  • Correctness guards from the downstream lessons: cross-origin targets never current (the djustlive.com /-pathname false-match I'd already hit in nav-active.js), and never clobbering an app-authored aria-current value.
  • Gate-off (Add remaining 13 Django built-in template filters #254): neutering the function fails 3/4 — non-tautological.

What could be improved

  • Scoped to part 1 only. document.title sync needs the mount frame to carry the new title (a server/wire change), and the [dj-root] dj-view-attr update touches the mount handler — both deferred to keep this PR a clean single-file client change. Tracked in feat(nav): dj-navigate should keep active-link state (aria-current) in sync across SPA navigation #1756.
  • The 3 wiring call-sites are covered by inspection, not an integration test (the unit tests call updateAriaCurrent() directly). Acceptable for 3 one-liners; a real-click integration test would be stronger.

Verified

  • New test 4/4 (gate-off proven); navigation + hooks 45/45 green; bundle rebuilt from src (make build-js no-diff); CI green (js, nav-hooks-guard, python, rust, demo-checks). Two-commit shape intact.

RETRO_COMPLETE

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

Labels

security-review PR touches security-sensitive code and needs additional review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant