diff --git a/ZB.MOM.WW.Theme/Directory.Build.props b/ZB.MOM.WW.Theme/Directory.Build.props index 94422ba..4212faa 100644 --- a/ZB.MOM.WW.Theme/Directory.Build.props +++ b/ZB.MOM.WW.Theme/Directory.Build.props @@ -4,7 +4,7 @@ enable enable latest - 0.2.1 + 0.3.0 true diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/LoginCard.razor b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/LoginCard.razor index d15fc15..3589db7 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/LoginCard.razor +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/LoginCard.razor @@ -8,7 +8,17 @@
@code { - /// Product name shown in the card heading. Required. + /// + /// Product name shown in the card heading (rendered inside a + /// <span class="login-product">, followed by the "— sign in" + /// suffix). Required. Ignored when is set. + /// [Parameter, EditorRequired] public string Product { get; set; } = string.Empty; + /// + /// Optional full heading override. When set (non-whitespace), it replaces the + /// default <Product> — sign in heading entirely — use it to + /// localize or fully customize the heading copy. When unset, the default heading + /// (with in a .login-product span) is rendered. + /// + [Parameter] public string? Heading { get; set; } + /// /// Form action URL the sign-in POST targets. Defaults to /auth/login. /// diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailSection.razor b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailSection.razor index 5c1dcf4..e1442e6 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailSection.razor +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailSection.razor @@ -2,7 +2,10 @@ Apps that want cookie-persisted expand state keep their own interactive NavSection. *@ @namespace ZB.MOM.WW.Theme
- @Title + @* aria-expanded mirrors the native
state so tests and assistive + tech have a stable, queryable attribute (kit issue #1). It is rendered from + Expanded at SSR time and kept in sync by nav-state.js on restore and toggle. *@ + @Title
@ChildContent
diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/js/nav-state.js b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/js/nav-state.js index 92c054a..2fa0fb9 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/js/nav-state.js +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/js/nav-state.js @@ -5,22 +5,64 @@ (function () { var PREFIX = "zbnav:"; var INIT_ATTR = "data-zbnav-initialized"; - function apply() { - document.querySelectorAll("details.rail-section[data-nav-key]").forEach(function (el) { - if (el.hasAttribute(INIT_ATTR)) return; // already wired — avoid duplicate listeners - el.setAttribute(INIT_ATTR, ""); - var key = PREFIX + el.getAttribute("data-nav-key"); - var saved = null; - try { saved = window.localStorage.getItem(key); } catch (e) { return; } - if (saved === "1") el.open = true; - else if (saved === "0") el.open = false; - el.addEventListener("toggle", function () { - try { window.localStorage.setItem(key, el.open ? "1" : "0"); } catch (e) { /* ignore */ } - }); + var TRANSIENT_ATTR = "data-zbnav-transient"; + + // Mirror a section's native
onto its + // so tests and assistive tech have a stable, queryable attribute (issue #1). + function syncAria(el) { + var summary = el.querySelector("summary.rail-eyebrow-toggle"); + if (summary) summary.setAttribute("aria-expanded", el.open ? "true" : "false"); + } + + function wire(el) { + el.setAttribute(INIT_ATTR, ""); + var key = PREFIX + el.getAttribute("data-nav-key"); + var saved = null; + try { saved = window.localStorage.getItem(key); } catch (e) { saved = null; } + if (saved === "1") el.open = true; + else if (saved === "0") el.open = false; + el.addEventListener("toggle", function () { + syncAria(el); + // An active-link reveal (issue #2) is a transient open that must NOT + // overwrite the user's saved preference. The reveal flags the element + // before flipping open; consume the flag here and skip persistence. + if (el.getAttribute(TRANSIENT_ATTR) !== null) { + el.removeAttribute(TRANSIENT_ATTR); + return; + } + try { window.localStorage.setItem(key, el.open ? "1" : "0"); } catch (e) { /* ignore */ } }); } + + function apply() { + document.querySelectorAll("details.rail-section[data-nav-key]").forEach(function (el) { + if (!el.hasAttribute(INIT_ATTR)) wire(el); // wire once — avoid duplicate listeners + syncAria(el); // re-sync aria on every pass + }); + + // Reveal the section that holds the active link even if the user (or the + // app) left it collapsed, so the nav always shows where the user is + // (issue #2). Transient: flagged so the toggle handler does not persist it. + document.querySelectorAll("details.rail-section a.rail-link.active").forEach(function (link) { + var sec = link.closest("details.rail-section"); + if (sec && !sec.open) { + sec.setAttribute(TRANSIENT_ATTR, ""); + sec.open = true; + syncAria(sec); + } + }); + } + if (document.readyState === "loading") document.addEventListener("DOMContentLoaded", apply); else apply(); + + // Re-run after Blazor static-SSR enhanced navigation (or any re-render that + // replaces the rail nodes) so freshly inserted sections are wired, restored, + // and active-revealed (issue #3). The per-element INIT_ATTR guard keeps this + // idempotent for nodes that survived the navigation. + if (window.Blazor && typeof window.Blazor.addEventListener === "function") { + window.Blazor.addEventListener("enhancedload", apply); + } })(); diff --git a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/LoginCardTests.cs b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/LoginCardTests.cs index a98a74c..cc69192 100644 --- a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/LoginCardTests.cs +++ b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/LoginCardTests.cs @@ -41,4 +41,28 @@ public class LoginCardTests : TestContext var cut = RenderComponent(p => p.Add(x => x.Product, "OtOpcUa")); Assert.Empty(cut.FindAll("input[name=returnUrl]")); } + + // Theme issue #5: the product token is isolated in a .login-product span so it + // can be styled/asserted apart from the "— sign in" suffix. + [Fact] + public void Product_is_wrapped_in_login_product_span() + { + var cut = RenderComponent(p => p.Add(x => x.Product, "OtOpcUa")); + var product = cut.Find(".login-title .login-product"); + Assert.Equal("OtOpcUa", product.TextContent); + Assert.Contains("sign in", cut.Find(".login-title").TextContent); + } + + // Theme issue #5: Heading replaces the whole heading copy when set. + [Fact] + public void Heading_overrides_default_heading_when_set() + { + var cut = RenderComponent(p => p + .Add(x => x.Product, "OtOpcUa") + .Add(x => x.Heading, "Welcome back")); + var title = cut.Find(".login-title"); + Assert.Equal("Welcome back", title.TextContent); + Assert.Empty(cut.FindAll(".login-title .login-product")); + Assert.DoesNotContain("sign in", title.TextContent); + } } diff --git a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavRailTests.cs b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavRailTests.cs index 6b0d86f..e3d6e9b 100644 --- a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavRailTests.cs +++ b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavRailTests.cs @@ -47,6 +47,17 @@ public class NavRailTests : TestContext Assert.NotNull(cut.Find(".rail-section-body .rail-link")); } + // Theme issue #1: the mirrors the
state via + // aria-expanded so tests and assistive tech have a stable, queryable attribute. + [Fact] + public void NavRailSection_summary_aria_expanded_true_when_open() + { + var cut = RenderComponent(p => p + .Add(x => x.Title, "Navigation") + .AddChildContent("X")); + Assert.Equal("true", cut.Find("summary.rail-eyebrow-toggle").GetAttribute("aria-expanded")); + } + [Fact] public void NavRailSection_collapsed_when_not_expanded() { @@ -56,6 +67,16 @@ public class NavRailTests : TestContext Assert.False(cut.Find("details.rail-section").HasAttribute("open")); } + // Theme issue #1: aria-expanded reflects the collapsed SSR state too. + [Fact] + public void NavRailSection_summary_aria_expanded_false_when_collapsed() + { + var cut = RenderComponent(p => p + .Add(x => x.Title, "Nav").Add(x => x.Expanded, false) + .AddChildContent("X")); + Assert.Equal("false", cut.Find("summary.rail-eyebrow-toggle").GetAttribute("aria-expanded")); + } + [Fact] public void NavRailSection_emits_data_nav_key_slug_from_title_by_default() { diff --git a/ZB.MOM.WW.Theme/themeissues.md b/ZB.MOM.WW.Theme/themeissues.md new file mode 100644 index 0000000..5ebdb88 --- /dev/null +++ b/ZB.MOM.WW.Theme/themeissues.md @@ -0,0 +1,248 @@ +# 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 + ``, computed from `Expanded` at SSR + time, and `nav-state.js` keeps it in sync with the native `
` 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 `` snippet that mutates not-yet-parsed `
` 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 + ` — sign in`, and an optional `Heading` parameter + fully replaces the heading copy when set (for localization / custom wording). Existing + `" — 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 +`
` boolean attribute on the `
` element. The +`` 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 +
+ @Title +
@ChildContent
+
+``` + +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 +`
`/`` 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 ``, 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:` = `"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 `
`, 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 `
` 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 +`
` 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 `` (via `ThemeHead`) that sets each `
`'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 `

@Product — sign in

`. +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 `" — sign in"` rather than the product alone). + +**Recommended fix (optional).** Wrap the product in a span and/or expose an override: + +```razor +

— sign in

+``` + +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 `

ScadaBridge

` + `Sign In` button to the + kit's `LoginCard` (`h1.login-title` reading `" — 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 `` + `` + `localStorage` + (`zbnav:`). 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 + `
` state. + +These are being handled in the ScadaBridge Playwright suite separately.