Joseph Doherty
0e41e7c2e4
Addresses ZB.MOM.WW.Theme/themeissues.md: - #1 NavRailSection <summary> renders aria-expanded (SSR from Expanded), kept in sync by nav-state.js on restore + toggle. - #2 nav-state.js auto-expands the section holding a.rail-link.active (transient via data-zbnav-transient — does not overwrite saved state). - #3 nav-state.js re-applies on Blazor 'enhancedload' (idempotent via per-element init guard). - #5 LoginCard wraps product in span.login-product + optional Heading override param. - #4 documented as an accepted client-only-persistence tradeoff (no code change). +4 bUnit tests (48 total, all green).
249 lines
13 KiB
Markdown
249 lines
13 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.
|
|
|
|
## 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 |
|
|
|
|
---
|
|
|
|
## 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.
|
|
|
|
---
|
|
|
|
## 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.
|