From 47883846bd8f022904951ed28d4cf6050abc4209 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 7 Sep 2026 13:00:23 -0400 Subject: [PATCH 1/2] fix(web): the nav may not say where you are until the rows agree (#754) `show()` set `document.title` and `aria-current="page"`, then awaited the read. For the length of the fetch every sighted signal said "you are on Balances" over Positions' rows -- a stale figure under a fresh label, which is the one claim `_session_banner` refuses to make and the balances tiles carry an as-of stamp to prevent. THE CLAIM NOW WAITS FOR THE DOM THAT BACKS IT `commitNavigation` is the single place that says "you are here" -- the tab title and `aria-current` together -- and `paint` calls it after `rebuildInto`, on all three branches. A stopped view and a refused view are outcomes of this navigation too; leaving the nav on the previous route while the content reports this one is the same mismatch wearing different clothes. AND A CLICK IS STILL ACKNOWLEDGED, WITH A WEAKER CLAIM Deferring `aria-current` removes every immediate signal from a click, which on a slow route reads as a dead link and invites a second one. `data-pending` is that signal and is deliberately NOT `aria-current`: dotted rather than solid, muted rather than foreground. A pending link that looked like the current one would restore exactly the confusion the deferral removes. THE BUSY STATE WAS ALREADY NAVIGATION-ONLY `aria-busy` is raised in exactly one place and always was -- `show`, plus `index.html`'s initial value for the first paint. Neither the 15-second poll nor an SSE repaint raises it, so hanging the visual off it needs no new flag; a redundant `is_navigating` would be a second statement of one fact. Pinned by COUNTING the assignments rather than locating one. The content dims rather than clears (clearing costs a layout jump and claims more than is true) and takes `pointer-events: none`, because acting on a figure about to be replaced is the failure mode of a dimmed view left live. The bar is indeterminate: the server sends one response, not a stream, and a percentage would be a claim about something nothing counts. TWO VACUOUS TESTS, BOTH CAUGHT BY MUTATION The first cut of the show() assertion failed against a CORRECT implementation, because the new comment explains that it no longer sets `aria-current` and a raw substring scan cannot tell an explanation from an assignment. A test a docstring can break is one a docstring can satisfy; both scans now strip comments first. Worse, the CSS assertion was `"aria-busy" in _CSS`, which SURVIVED renaming the selector to `#content[data-nope="true"]` -- the comment above the rule still said the word. It asserts the selector against comment-stripped CSS now. Four mutants killed: commit-before-swap, the renamed busy selector, the dropped pending rule, and the removed keyframes. Reduced motion needed no new rule: the blanket query written before there was any animation to honour covers `keel-busy`, flattening it to a static bar rather than removing the signal. That comment is updated to say so, having claimed there was no animation in the file. 769 web + thinness tests pass. What no test here can prove is unchanged and listed in the module docstring: whether the dim is perceptible, whether a pending link reads as pending, whether any browser honours the query. Hand checks against a running `keel serve`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/web/static/css/keel.css | 73 ++++++++++++- keel/web/static/js/main.js | 59 +++++++++- tests/web/test_nav_transition.py | 181 +++++++++++++++++++++++++++++++ 3 files changed, 303 insertions(+), 10 deletions(-) create mode 100644 tests/web/test_nav_transition.py diff --git a/keel/web/static/css/keel.css b/keel/web/static/css/keel.css index 7cd0770..9ad3a8e 100644 --- a/keel/web/static/css/keel.css +++ b/keel/web/static/css/keel.css @@ -451,6 +451,66 @@ header a { header a:hover { color: var(--fg); } header a[aria-current="page"] { color: var(--fg); border-bottom-color: var(--accent); } +/* ── the navigation transition (#754) ───────────────────────────────────────────────────────── + * + * `show()` used to set `aria-current` and the tab title before awaiting the read, so for the + * length of the fetch the nav said "Balances" over Positions' rows -- a stale figure under a + * fresh label. The claim now waits for the DOM (`commitNavigation`), which leaves two states to + * draw here, and they must not look alike: + * + * [data-pending] your click was heard; the rows are still the old route's + * [aria-current="page"] you are here, and what is on screen is this route + * + * Pending is deliberately WEAKER than current -- dotted rather than solid, and it does not take + * the foreground colour. A pending link that looked identical to the current one would restore + * exactly the confusion the deferral removes. */ +header a[data-pending] { + border-bottom-style: dotted; + border-bottom-color: var(--muted); +} + +/* The busy state, which `main.js` raises ONLY on navigation -- never on the 15-second poll or an + * SSE repaint, both of which would otherwise flash the page four times a minute on a route + * nobody navigated. `index.html` ships `aria-busy="true"` so the first paint is covered too. + * + * Dimmed rather than emptied. Clearing would be the stronger claim and it costs a layout jump on + * every navigation; dimming says "this is not current" while leaving the reader's place. The + * figures beneath are still the previous route's and are still legible, which is why the nav no + * longer says otherwise -- the two halves of this fix only work together. */ +#content[aria-busy="true"] { + opacity: 0.55; + /* The rows underneath are stale by definition here. Nothing beneath should be clickable while + * it is: acting on a figure that is about to be replaced is the failure mode of a dimmed view + * that stays live. */ + pointer-events: none; +} + +/* An indeterminate bar, because the read has no measurable progress to report -- the server + * sends one response, not a stream of them, and a fake percentage would be a claim about + * something nothing is counting. + * + * On `#view` rather than `#content` so it sits above the dimmed rows without inheriting the + * dim. Reduced motion is handled by the blanket rule further down, which flattens the animation + * to a static bar rather than removing the signal. */ +#content[aria-busy="true"] #view::before { + content: ""; + position: fixed; + top: 0; + left: 0; + right: 0; + height: 2px; + background: var(--accent); + transform-origin: 0 50%; + animation: keel-busy 1.1s ease-in-out infinite; + z-index: 1; +} + +@keyframes keel-busy { + 0% { transform: scaleX(0); } + 50% { transform: scaleX(0.7); } + 100% { transform: scaleX(1); } +} + /* ── layout ─────────────────────────────────────────────────────────────────────────────────── * * `max-width: 62rem` is render.py's measure, kept: it is the line length the existing page was @@ -997,10 +1057,15 @@ footer { .grid { grid-template-columns: repeat(auto-fit, minmax(0, 1fr)); } } -/* Honour a reader who has asked the operating system for less motion. There is no animation in - * this stylesheet today; the rule is here so that the first one added inherits the courtesy - * rather than depending on its author remembering -- the same reasoning `server.needs_database` - * gives for guarding a whole route set instead of one page. */ +/* Honour a reader who has asked the operating system for less motion. Written before there was + * any animation to honour, so that the first one added inherited the courtesy rather than + * depending on its author remembering -- the same reasoning `server.needs_database` gives for + * guarding a whole route set instead of one page. #754's `keel-busy` is that first animation, + * and it arrived already covered, which is what the rule was for. + * + * Note what this does to it: `animation-duration: 0.01ms` plus `iteration-count: 1` leaves the + * busy bar drawn at its final frame -- a static full-width rule -- rather than removing it. The + * signal survives; only the movement goes. */ @media (prefers-reduced-motion: reduce) { *, *::before, *::after { animation-duration: 0.01ms !important; diff --git a/keel/web/static/js/main.js b/keel/web/static/js/main.js index fd7b6c3..711c401 100644 --- a/keel/web/static/js/main.js +++ b/keel/web/static/js/main.js @@ -523,6 +523,11 @@ async function paint(route, rebuild, force) { } else { rebuildInto(mount(route, readings), Boolean(force)); } + // AFTER the swap, never before: this is the point at which the nav label, the tab title and the + // rows on screen describe the same route (#754). Reached by all three branches above -- a + // stopped view and a refused view are outcomes of this navigation too, and leaving the nav + // pointing at the previous route while the content reports on this one is the same mismatch. + commitNavigation(route); contentNode.setAttribute("aria-busy", "false"); } @@ -606,18 +611,60 @@ const SETUP_ROUTE = ROUTES.find((route) => route.name === "setup") ?? DEFAULT_RO * @param {Route} route * @param {boolean} focus */ -function show(route, focus) { - current = route; - document.title = "keel — ".concat(route.label); +/** + * The attribute marking the link a reader clicked, while its route is still loading. + * + * Deliberately NOT `aria-current`. That one means "this IS the current page" to a screen reader + * and to the stylesheet, and saying it before the view exists is the bug #754 is about. This says + * only "your click was heard", which is a different claim and needs a different attribute -- + * without it, deferring `aria-current` would leave a slow route looking like a dead link and + * invite a second click. + * + * `data-` rather than a class so it cannot collide with the palette's own state classes, and so + * the stylesheet hook and the meaning stay in one name. + * @type {string} + */ +const PENDING_ATTR = "data-pending"; +/** + * Acknowledge a click on `route` without claiming arrival. + * @param {Route} route + */ +function markPending(route) { + for (const link of document.querySelectorAll("header nav a")) { + if (link.getAttribute("href") === pathFor(route)) link.setAttribute(PENDING_ATTR, ""); + else link.removeAttribute(PENDING_ATTR); + } +} + +/** + * Say where the reader now is -- called by `paint` AFTER the DOM backing it has been swapped in. + * + * Every claim about arrival lives here, in one function, so there is one place to get the + * ordering wrong instead of three. `aria-current="page"` remains both the assistive signal and + * the CSS hook (`header a[aria-current="page"]`), so the underline a sighted user sees and the + * word a reader hears still come from one attribute and cannot drift apart. + * + * Clearing `PENDING_ATTR` belongs here for the same reason: the acknowledgement ends exactly when + * the arrival begins, so a marker cannot outlive the read that set it. + * @param {Route} route + */ +function commitNavigation(route) { + document.title = "keel — ".concat(route.label); for (const link of document.querySelectorAll("header nav a")) { const isCurrent = link.getAttribute("href") === pathFor(route); - // `aria-current="page"` is both the assistive signal and the CSS hook (`header - // a[aria-current="page"]`), so the underline a sighted user sees and the word a reader hears - // come from one attribute and cannot drift apart. if (isCurrent) link.setAttribute("aria-current", "page"); else link.removeAttribute("aria-current"); + link.removeAttribute(PENDING_ATTR); } +} + +function show(route, focus) { + current = route; + // NOT `aria-current` and NOT `document.title` -- both are claims about where you ARE, and the + // DOM that would back them is still the previous route's until `paint` swaps it (#754). What + // this may say is that a read is happening, which is `data-pending` and `aria-busy`. + markPending(route); contentNode.setAttribute("aria-busy", "true"); if (focus) viewNode.focus(); diff --git a/tests/web/test_nav_transition.py b/tests/web/test_nav_transition.py new file mode 100644 index 0000000..df3a0ad --- /dev/null +++ b/tests/web/test_nav_transition.py @@ -0,0 +1,181 @@ +"""The navigation transition: what the page may claim while a read is in flight (#754). + +A browser cannot run here (see `test_client_assets`'s docstring for the standing argument), so +these are assertions over the shipped source. That makes them vulnerable in the usual way -- a +substring is satisfied by a declaration -- so every test below either slices ONE function's body +and asserts about that body alone, or counts occurrences across the file. Presence anywhere is +never the assertion. + +THE BUG THESE PIN. `show()` used to set `document.title` and `aria-current="page"` and then +`await` the read. For the length of the fetch every sighted signal said "you are on Balances" +while the DOM still held Positions' rows: a stale figure under a fresh label, which is the exact +false claim `_session_banner` refuses to make elsewhere. The fix defers the claim until the DOM +that backs it has been swapped in. + +WHAT THIS FILE CANNOT PROVE, stated so a green run is not read as more than it is: that the +indicator is actually visible, that the dim is perceptible, that a pending link reads as pending, +or that the reduced-motion query is honoured by any particular browser. Those are hand checks +against a running `keel serve`. +""" + +from __future__ import annotations + +import re + +from keel.web import staticfiles + +_JS = staticfiles.STATIC_ROOT / "js" +_MAIN = (_JS / "main.js").read_text() +_CSS = (staticfiles.STATIC_ROOT / "css" / "keel.css").read_text() +#: The stylesheet with its `/* ... */` prose removed. CSS has no line comments, so this is the +#: whole job -- and it is not optional: renaming the busy SELECTOR to something inert left every +#: assertion below green, because the comment ABOVE the rule still said "aria-busy". Caught by +#: mutation, which is the only thing that finds a scan satisfied by its own explanation. +_CSS_RULES = re.sub(r"/\*.*?\*/", "", _CSS, flags=re.S) + + +def _body(source: str, name: str) -> str: + """The source of one top-level `function name(...)`, up to its closing brace. + + Top-level functions in this file close on a `}` in column zero, which is what bounds the + slice. Deliberately NOT a brace counter: a counter would have to understand strings, regexes + and comments to be right, and being subtly wrong would silently widen every assertion built + on it. A column-zero brace is a property of the file that is checked below. + """ + for prefix in ("\nfunction ", "\nasync function "): + needle = prefix + name + "(" + if needle in source: + start = source.index(needle) + return source[start : source.index("\n}", start)] + raise AssertionError(name + " is not a top-level function in this file") + + +def _code_only(source: str) -> str: + """`source` with comments removed, so an assertion is about CODE and not about prose. + + Necessary, not tidy. The first cut of `test_show_makes_no_claim_about_arrival_before_the_read` + failed against a correct implementation, because `show`'s new comment *explains* that it no + longer sets `aria-current` -- and a raw substring scan cannot tell an explanation from an + assignment. A test that a docstring can break is a test that a docstring can also satisfy. + + `//` is stripped only when not preceded by `:`, so a `https://` inside a string survives. + `test_the_comment_stripper_keeps_code_and_drops_prose` pins both halves. + """ + source = re.sub(r"/\*.*?\*/", "", source, flags=re.S) + return re.sub(r"(? None: + """The helper two assertions below rest on. A stripper that ate everything would make them + pass against any input at all.""" + sample = 'a(); // mentions aria-current\nconst u = "https://x/y";\n/* aria-current */\nb();' + stripped = _code_only(sample) + assert "aria-current" not in stripped, "prose survived -- the assertions become prose checks" + assert "a();" in stripped and "b();" in stripped, "code was eaten" + assert "https://x/y" in stripped, "a URL in a string was mangled" + + +def test_the_slicer_finds_a_bounded_body() -> None: + """The helper every other test rests on. If this ever returns the whole file, the assertions + below become presence-anywhere checks and stop meaning what they say.""" + body = _body(_MAIN, "show") + assert "function show(" in body + assert len(body) < len(_MAIN) / 4, "the slice is not bounded -- every test here is vacuous" + assert "async function paint(" not in body, "the slice ran past its own function" + + +def test_show_makes_no_claim_about_arrival_before_the_read() -> None: + """`show` runs before the fetch. It may say a read is happening; it may not say where you are. + + Both halves matter and both were wrong: `aria-current` is what a screen reader announces as + the current page and what CSS underlines, and `document.title` is what the tab and the + history entry say. + """ + body = _code_only(_body(_MAIN, "show")) + assert "aria-current" not in body, ( + "show() sets aria-current before awaiting the read -- that is the stale-view-under-fresh-" + "label bug this issue is about" + ) + assert "document.title" not in body, "show() renames the tab before the tab's content exists" + + +def test_arrival_is_claimed_only_after_the_dom_that_backs_it() -> None: + """`commitNavigation` is the one place that says "you are here", and `paint` calls it after + `rebuildInto` -- never before.""" + assert "function commitNavigation(" in _MAIN + commit = _body(_MAIN, "commitNavigation") + assert "aria-current" in commit + assert "document.title" in commit + + paint = _body(_MAIN, "paint") + assert "commitNavigation(" in paint, "paint never commits the navigation it completed" + assert paint.index("rebuildInto(") < paint.index("commitNavigation("), ( + "paint commits the navigation before swapping the DOM -- the same ordering bug, moved" + ) + + +def test_a_click_is_acknowledged_without_claiming_arrival() -> None: + """Deferring `aria-current` removes every immediate signal from a click, which invites a + second click on a slow read. A pending marker restores the feedback WITHOUT the claim, so it + must be a different attribute from the one that means "you are here".""" + body = _code_only(_body(_MAIN, "show")) + assert "markPending(" in body, "a click on a slow route gives no feedback at all" + assert "aria-current" not in body + + # And the marker it sets is a DIFFERENT attribute from the one meaning "you are here". + # Asserting the call alone would pass over a `markPending` that set `aria-current`, which is + # the bug wearing a new name. + marker = _code_only(_body(_MAIN, "markPending")) + assert "PENDING_ATTR" in marker + assert "aria-current" not in marker + assert _code_only(_MAIN).count('PENDING_ATTR = "data-pending"') == 1 + + +def test_the_pending_marker_is_always_cleared_where_arrival_is_claimed() -> None: + """A `data-pending` left behind outlives the read and marks a link forever.""" + commit = _body(_MAIN, "commitNavigation") + assert "removeAttribute" in commit + assert "PENDING_ATTR" in commit or "data-pending" in commit + + +def test_only_navigation_raises_the_busy_flag() -> None: + """The visual indicator hangs off `aria-busy`, so a poll or an SSE repaint raising it would + flash the page four times a minute on a route nobody navigated. + + Counted, not merely located: exactly ONE assignment of `"true"` in the client, and it is in + `show`. `index.html` ships the initial `aria-busy="true"` for the first paint, which is a + navigation in every sense that matters here. + """ + raised = re.findall(r'aria-busy",\s*"true"', _MAIN) + assert len(raised) == 1, f"aria-busy is raised in {len(raised)} places; exactly one is nav" + assert 'aria-busy", "true"' in _body(_MAIN, "show").replace("\n", " ").replace(" ", " ") + + +def test_the_busy_flag_is_lowered_on_every_path_that_raised_it() -> None: + """`paint` returns early twice -- a superseded route and a banner-only poll. Neither raised + the flag, so neither needs to lower it; the rebuild path does and must.""" + paint = _body(_MAIN, "paint") + assert 'aria-busy", "false"' in paint.replace("\n", " ").replace(" ", " ") + + +def test_the_stylesheet_gives_the_busy_state_a_visible_form() -> None: + """The accessible half shipped correct and the visual half did not exist: `aria-busy` was + toggled properly and nothing in the CSS referred to it.""" + assert '#content[aria-busy="true"]' in _CSS_RULES, ( + "nothing styles the busy state -- a sighted user sees no change. Asserted against the " + 'SELECTOR in comment-stripped CSS: `"aria-busy" in _CSS` passes on the prose alone.' + ) + assert "opacity" in _CSS_RULES + + +def test_the_pending_link_is_styled_distinctly_from_the_current_one() -> None: + """Otherwise the acknowledgement and the arrival look the same, which is the bug again.""" + assert "header a[data-pending]" in _CSS_RULES + assert 'header a[aria-current="page"]' in _CSS_RULES, "the state it must differ from is gone" + + +def test_the_transition_is_disabled_under_reduced_motion() -> None: + """Anything that animates has to answer this, and a media query naming it is the only way to + say so in a stylesheet.""" + assert "@media (prefers-reduced-motion: reduce)" in _CSS_RULES + assert "@keyframes keel-busy" in _CSS_RULES, "the busy bar has no animation to reduce" From b82ee64a31b0c6e26878f8d4350745598337c4b3 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 7 Sep 2026 13:35:01 -0400 Subject: [PATCH 2/2] fix(web): the busy bar selected nothing, and a throw would have bricked the view Review of this PR's own diff. Two defects, and the first meant half the feature did not exist. THE BAR NEVER DREW `#content[aria-busy="true"] #view::before` asks for a `#view` INSIDE `#content`. The shell nests it the other way -- `main#view > div#content` -- so the rule matched no document this client ever produces. The dim worked; the bar was inert from the first commit. `test_the_stylesheet_gives_the_busy_state_a_visible_form` stayed green throughout, because a selector's TEXT being present says nothing about whether it selects anything, and CSS has no other way to fail. The new test reads the nesting out of `index.html` and compares the rule against it, rather than against the stylesheet's own claim. `:has(> ...)` now, verified by re-running the original selector as a mutant. A RENDERER THAT THREW WOULD HAVE LEFT THE VIEW DEAD `paint` had no `try`/`finally`. `read` resolves on a transport failure rather than rejecting, but `mount` is a renderer and renderers throw; `paint` is called as `void paint(...)`, so nothing catches it. Before this branch a stranded `aria-busy` was harmless -- nothing styled it. As of this branch it carries `opacity: 0.55` AND `pointer-events: none`, so the same throw would leave the view dimmed with every control inside it dead: sort headers, status tabs, the timeline chip. The stale-but-usable view this change set out to improve on was strictly better than that. The flag now comes down in a `finally`. `commitNavigation` deliberately stays INSIDE the try: a render that threw is not an arrival, and claiming one would put the nav label back out of step with the rows, which is the whole bug. Nothing awaits between the `route !== current` guard and the block, so `current` cannot move under it and the flag lowered is always this route's own. AND CLEARING THE MARKER IS NOT THE SAME EVENT AS ARRIVING `commitNavigation` owned both, so a failed render left the clicked link marked pending for the life of the page. `clearPending` is separate and runs in the `finally`: the read being over is true on every path; arrival is not. `pointer-events: none` is KEPT, now that the flag is guaranteed to come down. Acting on a figure about to be replaced is the failure mode of a dimmed view left live, and that argument only holds while the dim is guaranteed to end. 6,322 passed / 3 skipped, ruff clean. Three mutants killed, one of them the shipped bug itself: the inside-out selector, the finally removed, and arrival claimed in the finally. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/web/static/css/keel.css | 17 ++++-- keel/web/static/js/main.js | 55 +++++++++++++----- tests/web/test_nav_transition.py | 97 ++++++++++++++++++++++++++++++-- 3 files changed, 147 insertions(+), 22 deletions(-) diff --git a/keel/web/static/css/keel.css b/keel/web/static/css/keel.css index 9ad3a8e..0947452 100644 --- a/keel/web/static/css/keel.css +++ b/keel/web/static/css/keel.css @@ -489,10 +489,19 @@ header a[data-pending] { * sends one response, not a stream of them, and a fake percentage would be a claim about * something nothing is counting. * - * On `#view` rather than `#content` so it sits above the dimmed rows without inheriting the - * dim. Reduced motion is handled by the blanket rule further down, which flattens the animation - * to a static bar rather than removing the signal. */ -#content[aria-busy="true"] #view::before { + * On `#view` rather than `#content` so it sits above the dimmed rows without inheriting the dim + * -- `opacity` creates a stacking context and would take the bar down with the content. + * + * `:has(> ...)` and NOT a descendant combinator, because the nesting runs the OTHER WAY: + * `main#view > div#content`. The first cut of this rule was `#content[aria-busy="true"] + * #view::before`, which asks for a `#view` INSIDE `#content` -- true of no document this shell + * ever produces, so it matched nothing and the bar never drew. CSS fails silently, and the test + * asserting the selector's text passed the whole time; + * `test_the_busy_bar_selector_matches_the_shell_it_targets` is what makes that failure loud. + * + * Reduced motion is handled by the blanket rule further down, which flattens the animation to a + * static bar rather than removing the signal. */ +#view:has(> #content[aria-busy="true"])::before { content: ""; position: fixed; top: 0; diff --git a/keel/web/static/js/main.js b/keel/web/static/js/main.js index 711c401..2050d14 100644 --- a/keel/web/static/js/main.js +++ b/keel/web/static/js/main.js @@ -506,7 +506,22 @@ async function paint(route, rebuild, force) { if (!rebuild) return; - if (primary.data === null) { + // `try`/`finally` around the swap, and the flag comes down in the `finally` (#757 review). + // `read` resolves on a transport failure rather than rejecting, but `mount` is a renderer and a + // renderer can throw -- and `paint` is called as `void paint(...)`, so nothing catches it. Left + // raised, `aria-busy` is no longer the harmless assistive-only signal it was before #754: it now + // carries `opacity` and `pointer-events: none`, so a throw here would leave the view dimmed and + // every control inside it dead. The stale-but-usable view we had before is strictly better than + // that, and this is what keeps it. + // + // `commitNavigation` stays INSIDE the try, after the swap: a render that threw is not an + // arrival, and claiming one would put the nav label back out of step with the rows -- the exact + // bug #754 exists to remove. + // + // Nothing awaits between the `route !== current` guard above and here, so `current` cannot move + // under this block and the flag being lowered is always this route's own. + try { + if (primary.data === null) { // `data: null` is the ONLY route into these two views, and `payload.envelope` guarantees the // key is `null` rather than `{}` for exactly this reason -- see `render.stoppedView`. // @@ -515,20 +530,23 @@ async function paint(route, rebuild, force) { // refused this browser. The two 403s (`_admitted`'s host check and its session check) share // this branch on purpose. They have the same remedy from the operator's side: the address // keel printed is what admits this browser, and pasting it is the action either one needs. - if (primary.error && primary.error.status === REFUSED_STATUS) { - rebuildInto(refusedView(primary, reconnect), Boolean(force)); + if (primary.error && primary.error.status === REFUSED_STATUS) { + rebuildInto(refusedView(primary, reconnect), Boolean(force)); + } else { + rebuildInto(stoppedView(primary, pathFor(SETUP_ROUTE)), Boolean(force)); + } } else { - rebuildInto(stoppedView(primary, pathFor(SETUP_ROUTE)), Boolean(force)); + rebuildInto(mount(route, readings), Boolean(force)); } - } else { - rebuildInto(mount(route, readings), Boolean(force)); + // AFTER the swap, never before: this is the point at which the nav label, the tab title and + // the rows on screen describe the same route (#754). Reached by all three branches above -- a + // stopped view and a refused view are outcomes of this navigation too, and leaving the nav + // pointing at the previous route while the content reports on this one is the same mismatch. + commitNavigation(route); + } finally { + clearPending(); + contentNode.setAttribute("aria-busy", "false"); } - // AFTER the swap, never before: this is the point at which the nav label, the tab title and the - // rows on screen describe the same route (#754). Reached by all three branches above -- a - // stopped view and a refused view are outcomes of this navigation too, and leaving the nav - // pointing at the previous route while the content reports on this one is the same mismatch. - commitNavigation(route); - contentNode.setAttribute("aria-busy", "false"); } /** @@ -655,10 +673,21 @@ function commitNavigation(route) { const isCurrent = link.getAttribute("href") === pathFor(route); if (isCurrent) link.setAttribute("aria-current", "page"); else link.removeAttribute("aria-current"); - link.removeAttribute(PENDING_ATTR); } } +/** + * Drop the click acknowledgement, however the read ended. + * + * Separate from `commitNavigation` because the two answer different questions. Arrival is a claim + * about where you ARE and must not be made when the render failed; the acknowledgement is about a + * read being over, which is true whether it succeeded, failed, or was superseded. Folding it into + * the commit left a link marked pending forever on any path that never arrived. + */ +function clearPending() { + for (const link of document.querySelectorAll("header nav a")) link.removeAttribute(PENDING_ATTR); +} + function show(route, focus) { current = route; // NOT `aria-current` and NOT `document.title` -- both are claims about where you ARE, and the diff --git a/tests/web/test_nav_transition.py b/tests/web/test_nav_transition.py index df3a0ad..7259392 100644 --- a/tests/web/test_nav_transition.py +++ b/tests/web/test_nav_transition.py @@ -32,6 +32,61 @@ #: assertion below green, because the comment ABOVE the rule still said "aria-busy". Caught by #: mutation, which is the only thing that finds a scan satisfied by its own explanation. _CSS_RULES = re.sub(r"/\*.*?\*/", "", _CSS, flags=re.S) +_INDEX = (staticfiles.STATIC_ROOT / "index.html").read_text() + + +def _ancestors(html: str, element_id: str) -> list[str]: + """The ids enclosing `element_id`, outermost first, read from the shipped markup. + + Deliberately a real nesting walk over the file rather than a guess: the whole point is to + compare a SELECTOR against the document it targets, and a helper that inferred the document + would only restate the selector's own assumption. + """ + body = re.sub(r"", "", html, flags=re.S) + open_stack: list[str] = [] + for match in re.finditer(r"<(/?)([a-zA-Z][a-zA-Z0-9]*)\b([^>]*?)(/?)>", body): + closing, tag, attrs, self_closing = match.groups() + if tag.lower() in {"meta", "link", "br", "img", "input", "path", "rect", "use"}: + continue + found = re.search(r'id="([^"]+)"', attrs) + if closing: + if open_stack: + open_stack.pop() + continue + if found and found.group(1) == element_id: + return list(open_stack) + if not self_closing: + open_stack.append(found.group(1) if found else "") + raise AssertionError(element_id + " is not in the shipped markup") + + +def test_the_busy_bar_selector_matches_the_shell_it_targets() -> None: + """The rule must describe the REAL nesting, which is `main#view > div#content`. + + THE BUG THIS EXISTS FOR. The first cut was `#content[aria-busy="true"] #view::before` -- a + `#view` inside `#content`, which is backwards. It matched nothing, the bar never drew, and + `test_the_stylesheet_gives_the_busy_state_a_visible_form` stayed green the entire time, + because a selector's TEXT being present says nothing about whether it selects anything. CSS + has no other way to fail. + + So this one asserts against `index.html` rather than against the stylesheet's own claim. + """ + assert "content" in _ancestors(_INDEX, "view") or "view" in _ancestors(_INDEX, "content"), ( + "neither element encloses the other; the busy rule cannot be written as a relationship" + ) + # `#content` is the one carrying `aria-busy`, and `#view` is its PARENT. + assert "view" in _ancestors(_INDEX, "content"), "the shell moved; re-derive the rule" + assert "content" not in _ancestors(_INDEX, "view") + + bar = [line for line in _CSS_RULES.splitlines() if "::before" in line and "aria-busy" in line] + assert bar, "the busy bar rule is gone" + selector = bar[0] + assert ':has(> #content[aria-busy="true"])' in selector, ( + "the bar hangs off a descendant relationship that does not exist in the shell: " + selector + ) + assert selector.index("#view") < selector.index("#content"), ( + "ancestor and descendant are the wrong way round: " + selector + ) def _body(source: str, name: str) -> str: @@ -131,11 +186,43 @@ def test_a_click_is_acknowledged_without_claiming_arrival() -> None: assert _code_only(_MAIN).count('PENDING_ATTR = "data-pending"') == 1 -def test_the_pending_marker_is_always_cleared_where_arrival_is_claimed() -> None: - """A `data-pending` left behind outlives the read and marks a link forever.""" - commit = _body(_MAIN, "commitNavigation") - assert "removeAttribute" in commit - assert "PENDING_ATTR" in commit or "data-pending" in commit +def test_the_pending_marker_is_cleared_however_the_read_ends() -> None: + """A `data-pending` left behind marks a link as loading forever. + + It used to be cleared inside `commitNavigation`, which is only reached when the render + SUCCEEDS -- so a renderer that threw left the link pending for the life of the page. Clearing + says the read is over, which is true on every path, so it belongs in the `finally` beside the + busy flag. Arrival does not: claiming it after a failed render would put the nav label back + out of step with the rows, which is the bug this whole change removes. + """ + assert "function clearPending(" in _MAIN + clearer = _code_only(_body(_MAIN, "clearPending")) + assert "removeAttribute" in clearer + assert "PENDING_ATTR" in clearer + assert "aria-current" not in clearer, "clearing an acknowledgement must not touch arrival" + + paint = _code_only(_body(_MAIN, "paint")) + assert "} finally {" in paint, "paint does not guarantee cleanup" + tail = paint[paint.index("} finally {") :] + assert "clearPending(" in tail, "the marker is not cleared on the failure path" + assert "commitNavigation(" not in tail, ( + "arrival is claimed in the finally -- a render that threw would still say you got there" + ) + + +def test_the_busy_flag_comes_down_even_when_the_render_throws() -> None: + """`aria-busy` is no longer decorative. Since #754 it carries `opacity` and + `pointer-events: none`, so a flag left raised leaves the view dimmed and every control inside + it dead -- worse than the stale-but-usable view that preceded this change. + + `read` resolves rather than rejects on a transport failure, but `mount` is a renderer and + `paint` is invoked as `void paint(...)`, so nothing catches what a renderer throws. + """ + paint = _code_only(_body(_MAIN, "paint")) + lowered = chr(34) + "aria-busy" + chr(34) + ", " + chr(34) + "false" + chr(34) + assert lowered in paint + tail = paint[paint.index("} finally {") :] + assert lowered in tail, "the flag is lowered on the success path only" def test_only_navigation_raises_the_busy_flag() -> None: