ReviewOS

stacks/stx

Router active matching ignores the query string, so filter links lose active state and aria-current lands on the wrong link

#2017
Closed glennmichael123 opened this 9 days ago ·
9 days ago

The router matches active state against location.pathname, so any link whose href carries a query string loses its active class and never receives aria-current. The link that is not filtered gets both instead, so assistive tech is told the wrong thing.

Query-string links are how filter chips, tab bars and sort controls are normally built, so this affects a common pattern rather than an exotic one.

Reproduction

Using the repo's own SPA harness, booted at /list?status=up:

layout('<nav>'
  + '<a href="/list?status=up"   data-stx-link class="chip active" id="up">Up</a>'
  + '<a href="/list?status=down" data-stx-link class="chip"        id="down">Down</a>'
  + '<a href="/list"             data-stx-link class="chip"        id="all">All</a>'
  + '</nav>')
#up   class: "chip"                       aria-current: null
#all  class: "chip exact-active active"   aria-current: page

The server rendered class="chip active" on #up, because ?status=up is the filter in effect. After updateActiveLinks() runs:

  • #up has lost active entirely, so the chip the user is on is styled as inactive
  • #all has gained active, exact-active and aria-current="page", so the unfiltered link claims to be the current page

Cause

dist/chunk-bqdbf8k2.js:

function updateActiveLinks() {
  var links = document.querySelectorAll('[data-stx-link]')
  var cur = location.pathname                         // <- query string dropped
  links.forEach(function (link) {
    var href = link.getAttribute('to') || link.getAttribute('href') || ''
    ac.split(' ').forEach(cls => link.classList.remove(cls))   // removed unconditionally
    var isExact = cur === href
    var isActive = href !== '/' ? cur.startsWith(href) : cur === '/'
    if (isActive) ac.split(' ').forEach(cls => link.classList.add(cls))
    markCurrent(link, isExact)
  })
}

cur has no search component while href does, so cur === href and cur.startsWith(href) are both false for every query-carrying link. Meanwhile a bare /list href passes cur.startsWith('/list'), which is why the unfiltered link wins.

Note the class is removed before anything is re-added, so this also overwrites correct server-rendered state. An app that computes the active filter server-side, which is the natural thing to do when the filter comes from the query, has its markup undone on the client.

Two things that make it hard to spot

markCurrent only clears aria-current when the value is exactly 'page':

if (link.getAttribute('aria-current') === 'page') link.removeAttribute('aria-current')

So a consumer who works around the missing attribute by stamping aria-current="true" server-side keeps it forever: the router neither updates nor clears it, and after one SPA navigation it names the wrong link. The workaround is worse than the bug.

It also only shows after client-side navigation. A full load renders correct server markup, so this looks fine until the router takes over.

Suggested

Compare like with like. Use location.pathname + location.search as cur when the href has a search component, or parse both and compare path and query separately. isActive should probably stay prefix-based on the path while isExact considers the query, so a nav link to /list can still be "active" on /list?status=up without being "exact".

Also worth not removing the active class before deciding, so that a link the router has no opinion about keeps whatever the server rendered.

Impact

Found while measuring component adoption across six apps. Query-carrying router links: statushq 22, analyticshq 20, loghq 5, bughq 2. statushq's dashboard filter chips are the clearest case — 28 chips across 10 views, all of the form ?status=, ?type=, ?range=.

Related: upstream's own FilterChip (src/ui/mobile/FilterChip.stx) stamps aria-current="true" server-side on its active chip, so it has the stale-attribute problem described above as soon as the row is used for query-string filters.

9 days ago

Fixed in 0f4e590d90. Your conclusion was right, and the cause is a shade different from the one in the report — worth saying because it changed the shape of the fix.

The quoted code is a stale build

dist/chunk-bqdbf8k2.js predates linkState(), which does compare query params. So "the query string is dropped" is not the current cause and cur is not location.pathname alone any more.

What is wrong is that the comparison is one-directional. It checked that every param the link names is in effect here — and a link with no params satisfies that trivially. So a bare /list was exact on /list?status=up, took exact-active and aria-current="page", and your measured symptom followed exactly.

Why only the no-query case tightened

The obvious fix — require the two to carry the same params both ways — broke a tested contract from #1777:

/dashboard?site=1&range=7d   on   /dashboard?site=1&range=7d&country=US

A range picker where the page also carries a country filter. That link must stay current: it names site and range, both are in effect. So only a link with no query is held to the stricter test. The two requests read as opposed and are not — one is about a link naming more than the page, the other about a link naming nothing.

/dashboard?site=1&range=7d  on  /dashboard?…&country=US   -> still exact
/list                       on  /list?status=up           -> no longer exact

aria-current="true"

Cleared now, as you asked — both values say "this is the current one" with no further meaning. The role-specific ones (step, location, date, time) are still left alone, since those are the page using the attribute for something else. There is a test with a aria-current="step" link beside the chips to hold that line.

One existing assertion changed on purpose

client-navigation.test.ts:669 asserted a no-query link was aria-current at a filtered URL, reasoning that "a link with no query matches on its path, so the nav entry stays current". Matching on its path is what active is for; aria-current="page" says this is the page you are on. Keeping a nav entry lit across filters is what data-stx-active-match exists for. The reasoning is written into the test so it is not quietly reverted.

What I could not confirm

It also only shows after client-side navigation.

I could not reproduce that half. In the repo's harness, refreshCurrentLinks runs once at boot and not after a query-only navigate() — which would be a second, separate defect — but a fragment swap in that harness may simply not reach the swap path, so I am not claiming it either way rather than guessing. If you are seeing stale classes specifically after a client navigation on a real page, that is worth its own issue with the before/after, because the fix would be elsewhere.

Also

The harness could not express any of this: boot() looked up the rendered document by the full string, so a URL with a query threw no rendered document for /list?status=up. It now finds the document by path — routes are paths, and a server renders the same page for every filter — while the window gets the full URL. That is why there were no tests here before.

On FilterChip: you are right that it stamps aria-current="true" server-side. With markCurrent now clearing that value, the stale-attribute trap is gone for it too.

Suite green: 14,165 pass.

Sign in to comment on this issue.