diff --git a/keel/web/static/css/keel.css b/keel/web/static/css/keel.css index 7cd0770..0947452 100644 --- a/keel/web/static/css/keel.css +++ b/keel/web/static/css/keel.css @@ -451,6 +451,75 @@ 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 + * -- `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; + 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 +1066,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..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,15 +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"); } - contentNode.setAttribute("aria-busy", "false"); } /** @@ -606,18 +629,71 @@ 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"); } +} + +/** + * 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 + // 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..7259392 --- /dev/null +++ b/tests/web/test_nav_transition.py @@ -0,0 +1,268 @@ +"""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) +_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: + """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_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: + """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"