fix(Link): emit aria-current and match paths in inertia mode - #6809
fix(Link): emit aria-current and match paths in inertia mode#6809kodjosama wants to merge 4 commits into
Conversation
The inertia override diverges from the vue-router one in three ways, and since every component that renders a link goes through it, all three reach the whole library in inertia mode. `isLinkActive` only fed `linkClass` and the slot prop, so the anchor never received `aria-current` — the vue-router variant binds it, and the prop's own documentation says it is "passed to the attribute aria-current". An Inertia app moving its navigation onto Nuxt UI lost the current-page announcement. `ariaCurrentValue` was missing from the `reactiveOmit` list, so it was forwarded to `ULinkBase` and landed on the DOM as `ariacurrentvalue`. `vue-router` omits it too, but there `RouterLink` declares it as a prop and consumes it; `InertiaLink` does not. Because the prop carries a default the attribute appeared even on components rendering no link at all. Active matching ran `page.url.startsWith(href)` against a url that carries the query string, on raw string prefixes, so a query defeated the match and `/inventory` reported active on `/inventory-adjustments`. Compare on the path, ignoring query and hash, and treat a match as the target path or a descendant of it — `/` stays a parent of every route, as with RouterLink. The overrides had no test project: the `vue` project resolves `#components` to the vue-router overrides only, which is how three defects coexisted here. Add a `vue-inertia` project mirroring it, with a stub for the optional `@inertiajs/vue3` peer dependency.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Inertia Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/runtime/vue/overrides/inertia/Link.vueParsing error: Unexpected token ) test/inertia/Link.spec.tsParsing error: Unexpected token : Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/vue/overrides/inertia/Link.vue`:
- Line 199: Update src/runtime/vue/overrides/inertia/Link.vue at lines 199-199
and 216-216 to emit aria-current whenever isLinkExactActive is true, without
requiring the exact prop. Add a regression test in test/inertia/Link.spec.ts at
lines 14-23 covering an exactly matching to path with exact omitted and
verifying aria-current is emitted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0e0d269-abf5-42b5-83d8-4b0843240643
📒 Files selected for processing (5)
src/runtime/vue/overrides/inertia/Link.vuetest/inertia/Link.spec.tstest/utils/inertia.tstest/utils/mount-inertia.tsvitest.config.ts
Merging this PR will degrade performance by 28.18%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | table (~13 slots) |
292.7 µs | 407.6 µs | -28.18% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing kodjosama:fix/inertia-link-aria-current (800d5cd) with v4 (e7b126b)
commit: |
Binding the attribute to `exact && isExactActive`, as this PR first did to match vue-router, makes the accessible behaviour opt-in: a navigation link to the page you are on stays silent unless the caller also passes `exact`. And `exact` cannot simply be switched on, because it doubles as the scope of the active class — turning it on to get the announcement narrows the highlight a sidebar entry needs. Binding it to the active state instead is worse. An app that migrated its primitives hit this: form Cancel buttons became links to their section index, and each one claimed to be the current page — /inventory/create marking its /inventory link with aria-current. The two questions are different. `isLinkActive` keeps section semantics for the class; a new `isLinkCurrentPage` answers "is this the page being rendered" by comparing paths, and only that drives aria-current. `exact` goes back to the raw url comparison it has always done, so nothing changes for callers already using it.
|
Updated: I originally mirrored the The alternative, driving it from So This leaves |
|
@codspeedbot fix this regression |
`normalizePath` stripped the query and hash before its empty-path
fallback, so `?tab=orders` and `#section` both collapsed to `/`. That
made the non-exact branch of `isLinkActive` reduce to
`currentPath.startsWith('/')` — true for every url — so a query- or
fragment-only link rendered active on every page, and every sibling tab
in a switcher reported active at once.
Return `undefined` for a pathless href instead. `isLinkActive` already
guards on `!targetPath`; `isLinkCurrentPage` now guards on it too,
keeping `exact` a raw url comparison as before.
2 participants
🔗 Linked issue
Resolves #6803
❓ Type of change
📚 Description
The
inertiaLinkoverride diverges from thevue-routerone in three ways. Every component that renders a link (UButton,UBreadcrumb,UNavigationMenu,UDropdownMenu, …) goes through it, so all three reach the whole library in Inertia mode.1.
aria-currentwas never emitted.isLinkActiveonly fedlinkClassand theactiveslot prop.vue-router/Link.vuebinds it (:189,:206), and the prop's own JSDoc says it is "passed to the attributearia-current". An Inertia app moving its navigation onto Nuxt UI silently lost the current-page announcement.2.
ariaCurrentValueleaked onto the DOM. It was missing from thereactiveOmitlist, souseForwardPropspassed it toULinkBase, which doesn't declare it either, and it reached the DOM asariacurrentvalue="page".vue-routeromits it from its list too, but there it's harmless:RouterLinkdeclaresariaCurrentValueas a real prop and consumes it.InertiaLinkdoesn't. Because the prop carries a default, the attribute appeared even on components rendering no link at all —<UButton>no link</UButton>rendered<button ariacurrentvalue="page">.3. Active matching used a raw prefix against a query-carrying URL.
page.url.startsWith(href)meant a query string defeated the match, and/inventoryreported active on/inventory-adjustments. Now compared on the path, matching the target path or a descendant of it.exactis untouched and still compares the raw url.What drives
aria-currentThis PR first mirrored
vue-router's condition —exact && isExactActive— on the grounds that parity is the point of the report. I no longer think that's right, and I'd rather say why than quietly ship it.Requiring
exactmakes the accessible behaviour opt-in: a nav link to the page you are on stays silent unless the caller passes an extra prop. Andexactcan't simply be switched on, because it doubles as the scope of the active class — turning it on to get the announcement also narrows the highlight that a sidebar entry needs while browsing its section.Binding it to
isLinkActiveinstead is worse. An app that migrated its primitives to Nuxt UI hit this in production: form Cancel buttons became links to their section index, and each one started claiming to be the current page —/inventory/createmarking its/inventorylink witharia-current.So the two questions are separated:
isLinkActivekeeps section semantics for the class, and a newisLinkCurrentPageanswers "is this the page being rendered" by comparing paths. Only the latter drivesaria-current.This leaves
vue-routeras the odd one out, which is the opposite of where the report started, so it deserves your call rather than mine.vue-router/Link.vuehas the sameexact &&requirement and, I'd argue, the same problem —isExactActiveis already the right signal there without the extra prop. I deliberately did not touch it in this PR: it changes behaviour for existing Nuxt apps and that's a bigger decision than a bug fix. Happy to include it here, or open a separate PR, whichever you prefer.One correction to the issue
I listed
to="/"being active everywhere as a defect. With segment-aware matching that's just/being a parent of every route, which is whatRouterLinkdoes too — so this PR keeps it, andexactremains the way to make a home link exclusive. The real defect there was segment-blindness, which is fixed.Tests
The overrides had no test project at all: the
vueproject resolves#componentsto[...vueRouterOverrides, ...vueComponents, ...components], so theinertiadirectory was never loaded by anything. That's how three defects coexisted here.This adds a
vue-inertiaproject mirroring thevueone withrouter: 'inertia', plus a stub for@inertiajs/vue3(an optional peer dependency, not installed in this repo) and a router-lessmountSuspended. 15 behavioural specs, including the Cancel-button case: a link to the section index from a page inside it stays active but must not announce itself as current.Each guard is mutation-checked — reverting a fix must turn the suite red:
aria-currentfromisLinkActiveexactforaria-currentaria-currententirelyariaCurrentValuefromreactiveOmitpage.url.startsWith(href)Verification
pnpm vitest run --project vue-inertia→ 15 passed.pnpm lintclean on the touched files.On the full suite I see pre-existing failures in
DropdownMenu,Select,ContextMenuandContentSearchthat also fail on a clean checkout ofv4in my environment and flake between runs. Comparing the failing-test sets with and without this branch, the modified tree's set is a strict subset of the clean tree's — nothing fails that didn't already.📝 Checklist