Joseph Doherty
e5a609be83
Interactive-render nav fix (CSS display:none-when-closed + nav-state.js MutationObserver re-wire) shipped in 0.3.1 and verified — ScadaBridge Central UI NavCollapseTests now pass. All six issues now resolved (5 fixed, 1 tradeoff).
358 lines
20 KiB
Markdown
358 lines
20 KiB
Markdown
# ZB.MOM.WW.Theme — Known Issues
|
|
|
|
Issues found in the `ZB.MOM.WW.Theme` kit that are best fixed **once in the kit and
|
|
re-distributed** to every consuming app, rather than worked around per-app. Found while
|
|
debugging the ScadaBridge Central UI Playwright suite against kit version **0.2.1** (the
|
|
version ScadaBridge consumed at the time).
|
|
|
|
All file references below point at the kit source under `src/ZB.MOM.WW.Theme/`.
|
|
|
|
> **RESOLVED in kit 0.3.0 (2026-06-05).** Issues 1, 2, 3, and 5 are fixed in the kit and
|
|
> redistributed; Issue 4 is an accepted, documented tradeoff (no code change). See
|
|
> [Resolution](#resolution-kit-030) below for what changed and why.
|
|
>
|
|
> **RESOLVED in kit 0.3.1 (2026-06-05).** Issue 6 (collapsible nav non-functional under
|
|
> interactive Blazor render) is fixed — CSS `display:none`-when-closed backstop +
|
|
> `MutationObserver` re-wire in `nav-state.js`. See the [Issue 6](#issue-6--collapsible-nav-is-non-functional-under-interactive-blazor-render-mode)
|
|
> resolution note. All six issues are now resolved (5 fixed, 1 accepted tradeoff).
|
|
|
|
## Summary
|
|
|
|
| # | Severity | Component | Issue | Status (0.3.0) |
|
|
|---|----------|-----------|-------|----------------|
|
|
| 1 | Medium | `NavRailSection` / `nav-state.js` | No programmatic expanded-state hook (`aria-expanded` / `data-*`) on the section toggle. | ✅ Fixed |
|
|
| 2 | Medium | `nav-state.js` | The section containing the active link is not auto-expanded on navigation. | ✅ Fixed |
|
|
| 3 | Medium | `nav-state.js` | Persistence wires once on `DOMContentLoaded`; not re-applied after Blazor enhanced navigation / dynamic re-render. | ✅ Fixed |
|
|
| 4 | Low | `NavRailSection` | Always-expanded SSR default causes a flash / layout shift of collapsed sections on load. | 📄 Accepted tradeoff (documented) |
|
|
| 5 | Low (optional) | `LoginCard` | Heading bakes the localizable `— sign in` suffix into the product title with no separate hook. | ✅ Fixed |
|
|
| 6 | High | `NavRailSection` / `nav-state.js` | Under **interactive** Blazor render mode the whole collapsible nav is non-functional: clicking a header doesn't hide items, and `nav-state.js` never wires (no aria sync, no persistence, no active-reveal). | ✅ Fixed (0.3.1) |
|
|
|
|
---
|
|
|
|
## Resolution (kit 0.3.0)
|
|
|
|
Shipped in `ZB.MOM.WW.Theme` **0.3.0** (2026-06-05) and adopted across all three apps.
|
|
|
|
- **Issue 1 — `aria-expanded` hook.** `NavRailSection.razor` now renders
|
|
`<summary class="rail-eyebrow-toggle" aria-expanded="…">`, computed from `Expanded` at SSR
|
|
time, and `nav-state.js` keeps it in sync with the native `<details open>` state on restore
|
|
and on every `toggle`. Tests/AT can now await a stable attribute instead of inferring from
|
|
child-link visibility. (bUnit:
|
|
`NavRailSection_summary_aria_expanded_true_when_open` / `…_false_when_collapsed`.)
|
|
- **Issue 2 — auto-expand the active section.** After restoring saved state, `nav-state.js`
|
|
force-opens any `details.rail-section` that contains an `a.rail-link.active`. The reveal is
|
|
**transient** — it is flagged with `data-zbnav-transient` before the open flip so the
|
|
`toggle` handler skips persistence and the user's saved collapse preference is preserved.
|
|
- **Issue 3 — re-apply after enhanced navigation.** `apply()` is now also bound to Blazor's
|
|
`enhancedload` event (`Blazor.addEventListener('enhancedload', apply)`); the per-element
|
|
`data-zbnav-initialized` guard keeps re-runs idempotent. Static-SSR consumers keep
|
|
persistence + active-reveal after enhanced navigations; interactive Server consumers (e.g.
|
|
ScadaBridge Central UI) are unaffected as before.
|
|
- **Issue 4 — SSR flash / CLS: accepted tradeoff (no code change).** The kit deliberately
|
|
keeps **client-only** persistence to stay render-mode-agnostic, so the server renders every
|
|
section `open` and JS collapses the saved-collapsed ones after first paint. The alternative —
|
|
an inline pre-paint `<head>` snippet that mutates not-yet-parsed `<details>` from
|
|
`localStorage` — adds a FOUC-script that runs against DOM that does not yet exist, for a
|
|
Low-severity cosmetic flash. We chose **not** to take on that complexity/risk. Consumers who
|
|
care about the flash for a specific layout can add their own pre-paint restore; the kit will
|
|
not ship one by default. (This paragraph is the documented decision the issue asks for.)
|
|
- **Issue 5 — `LoginCard` heading hook.** The product token is now wrapped in
|
|
`<span class="login-product">@Product</span> — sign in`, and an optional `Heading` parameter
|
|
fully replaces the heading copy when set (for localization / custom wording). Existing
|
|
`"<Product> — sign in"` assertions still pass. (bUnit:
|
|
`Product_is_wrapped_in_login_product_span` / `Heading_overrides_default_heading_when_set`.)
|
|
|
|
---
|
|
|
|
## Issue 1 — No programmatic expanded-state hook on `NavRailSection`
|
|
|
|
**Severity:** Medium · **Files:** `Components/NavRailSection.razor`, `wwwroot/js/nav-state.js`
|
|
|
|
**Symptom.** A section's open/closed state is exposed only through the native
|
|
`<details open>` boolean attribute on the `<details class="rail-section">` element. The
|
|
`<summary class="rail-eyebrow-toggle">` toggle carries no `aria-expanded` and there is no
|
|
`data-*` state attribute. E2E tests (and some older assistive tech) cannot reliably query
|
|
or await the expanded state — they must infer it from child-link visibility.
|
|
|
|
**Root cause.** `NavRailSection.razor` renders:
|
|
|
|
```razor
|
|
<details class="rail-section" open="@Expanded" data-nav-key="@ResolvedKey">
|
|
<summary class="rail-eyebrow-toggle">@Title</summary>
|
|
<div class="rail-section-body">@ChildContent</div>
|
|
</details>
|
|
```
|
|
|
|
There is no attribute that mirrors `open` in a test- or AT-stable way.
|
|
|
|
**Impact on consumers.** Every consumer's UI tests must assert collapse state indirectly
|
|
(e.g. waiting on a child link to become visible/hidden) instead of awaiting a stable
|
|
attribute. This was the proximate cause of several stale ScadaBridge nav tests. Native
|
|
`<details>`/`<summary>` is keyboard- and screen-reader-accessible by default, so this is
|
|
primarily a **testability** gap (with a modest a11y upside).
|
|
|
|
**Recommended fix.** Mirror `open` onto `aria-expanded` on the `<summary>`, kept in sync by
|
|
`nav-state.js` (set it during `apply()` from `el.open`, and update it inside the existing
|
|
`toggle` listener). This gives both tests and AT a stable, queryable attribute without
|
|
changing the CSS-only collapse mechanism.
|
|
|
|
**Verify.** After the fix, `summary.rail-eyebrow-toggle` exposes `aria-expanded="true|false"`
|
|
that flips when the section is toggled and after a reload restores saved state.
|
|
|
|
---
|
|
|
|
## Issue 2 — Active section is not auto-expanded on navigation
|
|
|
|
**Severity:** Medium · **File:** `wwwroot/js/nav-state.js`
|
|
|
|
**Symptom.** When a section is collapsed (either because the user previously collapsed it —
|
|
`localStorage` `zbnav:<key>` = `"0"` — or because a consumer sets `Expanded="false"`) and
|
|
the user navigates to a route whose link lives inside that section, the section **stays
|
|
collapsed**. The active link (`a.rail-link.active`) is present in the DOM but hidden by the
|
|
closed `<details>`, so the nav no longer shows the user where they are.
|
|
|
|
**Root cause.** `nav-state.js` only *restores saved open/closed state*; it has no concept of
|
|
the active link. Grep of the kit confirms the only "active" handling is the
|
|
`.rail-link.active` CSS rule in `wwwroot/css/layout.css` — there is no JS that opens the
|
|
section containing the active link.
|
|
|
|
**Impact on consumers.** Loss of the common "navigating into a section reveals it" behavior.
|
|
A user who collapses a section and then deep-links (or is redirected) into one of its pages
|
|
lands with the relevant nav group collapsed and the current page's link hidden. (ScadaBridge
|
|
previously had app-owned auto-expand-on-navigate; the kit cutover dropped it, and the
|
|
`NavigatingIntoCollapsedSection_AutoExpandsIt` test now fails because nothing re-expands.)
|
|
|
|
**Recommended fix.** In `nav-state.js`, after restoring saved state, force-open any section
|
|
that contains the active link:
|
|
|
|
```js
|
|
// after the saved-state restore loop, before wiring is "done":
|
|
document.querySelectorAll("details.rail-section a.rail-link.active").forEach(function (link) {
|
|
var sec = link.closest("details.rail-section");
|
|
if (sec && !sec.open) sec.open = true; // reveal the section the user is in
|
|
});
|
|
```
|
|
|
|
Run this both on initial load and after Blazor navigation (see Issue 3). Decide whether the
|
|
forced-open should also persist to `localStorage` or be a transient reveal (recommended:
|
|
transient — don't overwrite the user's saved preference).
|
|
|
|
**Verify.** Collapse a section, navigate to one of its pages (or reload directly on it):
|
|
the section opens and the active link is visible.
|
|
|
|
---
|
|
|
|
## Issue 3 — Persistence wires once and is not re-applied after navigation
|
|
|
|
**Severity:** Medium · **File:** `wwwroot/js/nav-state.js`
|
|
|
|
**Symptom.** `apply()` runs only on the initial `DOMContentLoaded` (or first script eval)
|
|
and guards each element with `data-zbnav-initialized`. Under Blazor **static SSR enhanced
|
|
navigation** — or any dynamic re-render that replaces the `<details>` nodes — newly inserted
|
|
sections are never wired: their saved state is not restored and their toggles are not
|
|
persisted. The active-section logic from Issue 2 would likewise not re-run.
|
|
|
|
**Root cause.** The script self-invokes once:
|
|
|
|
```js
|
|
if (document.readyState === "loading")
|
|
document.addEventListener("DOMContentLoaded", apply);
|
|
else
|
|
apply();
|
|
```
|
|
|
|
There is no hook for Blazor's enhanced-navigation lifecycle and no `MutationObserver`.
|
|
|
|
**Impact on consumers.** Static-SSR consumers (the kit explicitly targets "works in static
|
|
SSR") lose nav persistence after the first enhanced navigation. **Interactive Blazor Server
|
|
consumers (such as ScadaBridge Central UI) are largely unaffected**, because the rail is
|
|
prerendered once and then patched in place over the SignalR circuit, so the original
|
|
`<details>` elements and their listeners survive — which is why ScadaBridge's persistence
|
|
appears to work today. The kit should still be correct for its static-SSR audience.
|
|
|
|
**Recommended fix.** Also re-run `apply()` on Blazor's enhanced-load event (and keep the
|
|
per-element init guard so it stays idempotent):
|
|
|
|
```js
|
|
if (window.Blazor && Blazor.addEventListener) {
|
|
Blazor.addEventListener('enhancedload', apply);
|
|
}
|
|
```
|
|
|
|
Optionally add a `MutationObserver` on the rail container as a framework-agnostic backstop.
|
|
|
|
**Verify.** On a static-SSR host, expand/collapse a section, perform an enhanced navigation
|
|
to another page and back, and confirm the saved state is still restored and toggles still
|
|
persist.
|
|
|
|
---
|
|
|
|
## Issue 4 — Always-expanded SSR default flashes / shifts layout on load
|
|
|
|
**Severity:** Low · **File:** `Components/NavRailSection.razor`
|
|
|
|
**Symptom.** `NavRailSection.Expanded` defaults to `true`, so every section renders `open`
|
|
in the server HTML. `nav-state.js` only collapses the saved-collapsed sections *after* the
|
|
script runs, producing a brief flash of fully-expanded nav and a layout shift (CLS) on each
|
|
load for users who keep sections collapsed.
|
|
|
|
**Root cause.** State lives in `localStorage` and is applied by JS post-render, while the
|
|
server-rendered default is unconditionally expanded. The server has no knowledge of the
|
|
saved state at render time.
|
|
|
|
**Impact on consumers.** Cosmetic flash / minor CLS on initial load; more noticeable with
|
|
many sections collapsed.
|
|
|
|
**Recommended fix (pick one).**
|
|
- Inline a tiny restore snippet in `<head>` (via `ThemeHead`) that sets each `<details>`'s
|
|
`open` from `localStorage` before first paint; or
|
|
- Accept the tradeoff and document it (the kit deliberately chose client-only persistence to
|
|
stay render-mode-agnostic).
|
|
|
|
**Verify.** With several sections saved-collapsed, reload and confirm no expanded-then-collapse
|
|
flash.
|
|
|
|
---
|
|
|
|
## Issue 5 — `LoginCard` heading couples the product name and "— sign in" (optional)
|
|
|
|
**Severity:** Low (optional) · **File:** `Components/LoginCard.razor`
|
|
|
|
**Symptom.** The card heading is `<h1 class="login-title">@Product — sign in</h1>`.
|
|
The (localizable) `— sign in` suffix is baked into the product title with no separate hook,
|
|
so consumers can't restyle/override the heading copy or assert the product token in isolation
|
|
without string-matching the whole heading.
|
|
|
|
**Impact on consumers.** Minor: per-app heading customization and exact-text test assertions
|
|
are awkward (must match `"<Product> — sign in"` rather than the product alone).
|
|
|
|
**Recommended fix (optional).** Wrap the product in a span and/or expose an override:
|
|
|
|
```razor
|
|
<h1 class="login-title"><span class="login-product">@Product</span> — sign in</h1>
|
|
```
|
|
|
|
or add an optional `Heading` parameter that, when set, replaces the default heading entirely.
|
|
|
|
---
|
|
|
|
## Issue 6 — Collapsible nav is non-functional under interactive Blazor render mode
|
|
|
|
**Severity:** High · **Files:** `Components/NavRailSection.razor`, `wwwroot/js/nav-state.js`,
|
|
`wwwroot/css/layout.css` · **Status:** ✅ Fixed in 0.3.1
|
|
|
|
> **Resolution (kit 0.3.1, 2026-06-05).** Both recommended parts shipped, so the collapsible
|
|
> nav now works under interactive render modes as well as static SSR:
|
|
> 1. **CSS robust collapse** — `layout.css` hides the body explicitly when closed instead of
|
|
> relying on the native `::details-content` content-hiding (which an interactive framework
|
|
> desyncs): `.rail-section:not([open]) > .rail-section-body { display: none; }`.
|
|
> 2. **Render-mode-agnostic re-wire** — `nav-state.js` adds a `MutationObserver` on
|
|
> `document.documentElement` (childList + subtree) that re-runs `apply()` whenever
|
|
> `details.rail-section` nodes are added/replaced, so the interactive runtime's re-render
|
|
> gets wired (aria sync, `data-zbnav-initialized`, localStorage persistence, active-reveal).
|
|
> The existing `enhancedload` hook (Issue 3) is kept for static-SSR enhanced navigation.
|
|
>
|
|
> Verified live in ScadaBridge Central UI (global `@rendermode InteractiveServer`): the
|
|
> Playwright `NavCollapseTests` (toggle-hides-items, persistence-survives-reload,
|
|
> deep-link-auto-reveal) now pass against 0.3.1.
|
|
|
|
> **This corrects Issue 3's note**, which claimed interactive Blazor Server consumers are
|
|
> "largely unaffected because the rail is patched in place." Direct observation of the live
|
|
> ScadaBridge Central UI (global `@rendermode InteractiveServer`) shows that is **false** —
|
|
> the kit's `<details>`/JS nav does not work under interactive render modes at all.
|
|
|
|
**Symptom.** In an app that renders the rail under an interactive render mode
|
|
(`InteractiveServer`, `InteractiveWebAssembly`, or `InteractiveAuto`), the collapsible nav is
|
|
visually and functionally dead:
|
|
|
|
1. Clicking a section header toggles the chevron (▶/▼) but **does not hide the section's
|
|
items** — the links stay fully visible under a "collapsed" chevron.
|
|
2. `aria-expanded` never changes, `localStorage` is never written, the saved state is not
|
|
restored on reload, and the active-section auto-reveal (Issue 2) does not fire.
|
|
|
|
**Root cause.** The kit nav is a **static-SSR / CSS-only** design (NavRailSection's own
|
|
comment: *"works in static SSR"*). Under an interactive render mode, Blazor's runtime
|
|
**owns and re-renders the `<details>`/`<summary>` DOM** after it adopts the prerendered
|
|
markup. Two independent consequences, both observed live:
|
|
|
|
- **Native collapse is defeated.** On the live page a closed section has `details.open === false`
|
|
and its `::details-content` computes `content-visibility: hidden`, yet the
|
|
`.rail-section-body` and its links remain laid out and visible (measured non-zero height /
|
|
non-null `offsetParent`). Blazor's management of the native `<details>` desyncs the browser's
|
|
built-in content-hiding. The body's `display` value (flex/block/grid/inline) makes no
|
|
difference — only an explicit `display: none` actually hides it.
|
|
- **`nav-state.js` never wires the live DOM.** The interactive `<details>` elements have **no
|
|
`data-zbnav-initialized` attribute**, i.e. `wire()` never ran on them: `apply()` runs on
|
|
`DOMContentLoaded` against the *prerendered* nodes, which Blazor then replaces, and the only
|
|
re-run hook (`enhancedload`, added for Issue 3) does not fire under interactive render modes.
|
|
So aria sync, localStorage persistence, and active-reveal are all inert.
|
|
|
|
This matters for the kit's stated goal: per the normalization notes, nav-expand persistence was
|
|
promoted into the kit at 0.2.0 *"so all three apps share one persistence mechanism."* One of the
|
|
three consumers (ScadaBridge Central UI) is interactive Blazor Server, where that mechanism
|
|
silently does nothing.
|
|
|
|
**Verified evidence (live, global InteractiveServer).** On a logged-in dashboard:
|
|
`data-zbnav-initialized` absent on every `details.rail-section`; after clicking a header,
|
|
`details.open === false` but the section's link still reports `clientHeight: 33` and a non-null
|
|
`offsetParent`; setting `.rail-section-body { display:none }` is the only thing that hides it;
|
|
`localStorage` has no `zbnav:*` keys before or after toggling.
|
|
|
|
**Recommended fix (two parts — both belong in the kit).**
|
|
|
|
1. **Make the collapse render-mode-robust (CSS).** Don't rely solely on the native
|
|
`<details>` content-hiding; hide the body explicitly when closed:
|
|
|
|
```css
|
|
/* layout.css — robust across render modes; native ::details-content hiding
|
|
is unreliable once an interactive framework manages the <details>. */
|
|
.rail-section:not([open]) > .rail-section-body { display: none; }
|
|
```
|
|
|
|
(Verified live: this is exactly what hides the items.)
|
|
|
|
2. **Make persistence/aria/reveal work under interactive render.** `enhancedload` is
|
|
static-SSR-only; also wire after the interactive runtime has (re)rendered. Options, in
|
|
preference order:
|
|
- Re-run `apply()` from Blazor's post-render hooks — `Blazor.addEventListener('afterStarted', …)`
|
|
(interactive WASM/Server boot) and re-apply on circuit/render updates; and/or
|
|
- Add a `MutationObserver` on the rail container that calls `apply()` when
|
|
`details.rail-section` nodes are added/replaced (framework-agnostic backstop — covers
|
|
interactive re-renders, enhanced nav, and dynamic nav alike);
|
|
- **Or** ship an explicitly **interactive** `NavRailSection` variant (a small Blazor
|
|
component with an `@onclick` toggle and `[Parameter] bool Expanded` two-way state) for
|
|
consumers that render interactively — which is what NavRailSection's own comment already
|
|
gestures at (*"Apps that want cookie-persisted expand state keep their own interactive
|
|
NavSection"*). If the kit's intent is that interactive apps bring their own section
|
|
component, say so loudly in the docs and have the CSS-only one degrade gracefully (part 1
|
|
still applies so at least the visual collapse works).
|
|
|
|
**Verify.** In an interactive-render host: clicking a header hides the section's items; the
|
|
summary's `aria-expanded` flips; `localStorage` gets a `zbnav:<key>` entry; the state survives
|
|
a reload; and deep-linking into a collapsed section reveals it.
|
|
|
|
**Consumer note (ScadaBridge).** Resolved on 0.3.1: ScadaBridge's Central UI consumes
|
|
`ZB.MOM.WW.Theme` 0.3.1, and the Playwright `NavCollapseTests` (toggling, persistence,
|
|
auto-reveal) now pass — the `NavCollapseWiredAsync` gate (which waits for
|
|
`data-zbnav-initialized` on every `details.rail-section`) is satisfied under interactive
|
|
render, so those tests run unskipped and green.
|
|
|
|
---
|
|
|
|
## Not kit bugs — expected consumer adaptations
|
|
|
|
For the avoidance of doubt, the following are **not** theme issues; they are the normal cost
|
|
of adopting the kit and belong in each consumer's own tests/markup:
|
|
|
|
- Login markup moved from a hand-rolled `<h4>ScadaBridge</h4>` + `Sign In` button to the
|
|
kit's `LoginCard` (`h1.login-title` reading `"<Product> — sign in"`, button labelled
|
|
`Sign in`). Consumers must update selectors/text accordingly.
|
|
- Nav moved from app-owned `button.nav-section-toggle` + `aria-expanded` + a
|
|
`scadabridge_nav` cookie to the kit's `<details.rail-section>` + `<summary>` + `localStorage`
|
|
(`zbnav:<key>`). Collapsed sections now **keep their children in the DOM** (hidden), and
|
|
sections default to **expanded**, not collapsed — so DOM-presence-based "hidden" assertions
|
|
and "collapsed by default" assumptions must be rewritten around visibility and the
|
|
`<details open>` state.
|
|
|
|
These are being handled in the ScadaBridge Playwright suite separately.
|