fix(web): harden, polish and optimize the web UI per the Sept 2026 audit (#568)

* fix(web): harden, polish and optimize the web UI per the September 2026 audit Works through docs/archive/WEB_UI_AUDIT_2026-09.md (health 8/20). Implementation integrity (P0) - app.css now defines every utility class the templates and JS use, including .hidden, so the ~145 JS show/hide toggles work. Button reset, and base component rules (.btn, .form-control) wrapped in :where() so utility classes on the same element win. New static-audit test fails when a used utility class has no rule. Accessibility - Focus rings render (the old ring rule referenced undefined variables); one :focus-visible outline everywhere; skip link; labelled nav landmarks. - Shared dialog helper (js/utils/dialog.js): role/aria-modal, focus trap, Escape, focus return, applied to every modal. - Named icon-only buttons and labelled ~70 form fields. - Toasts announced once; errors persist >= 10s; one showNotification. - Captive WiFi page: live region, timeouts, dark mode, 16px inputs. Performance (Pi Zero 2 W) - SSE streams and tab timers pause when hidden or off-tab; the display stream only runs while a preview is visible. app-shell.js deferred. - Widget scripts served as one versioned bundle (/assets/widgets.js): 52 -> 21 script tags, 66 -> 35 requests on first load. - Stdlib gzip fallback when flask-compress is missing: first-load JS/CSS 1358 KB -> 291 KB on the wire. SSE untouched. Theming and responsive - File managers, form fields and Fonts upload on theme tokens; bare inputs themed in dark mode; no more white surfaces. - No horizontal overflow at 375px on any tab; 44px touch targets on coarse pointers; reduced-motion respected; header title truncates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): clear Codacy findings on #568 - json-file-manager: focus-trap releases kept in a Map (no dynamic property access or delete; no value-returning forEach callback) - notification / schedule-picker: style and day-label lookups via Map - app.js: move the pending-queue assignment out of the expression - diff_viewer / error_handler: named function declarations instead of arrow consts No behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: check the OAuth widget ships in the widget bundle base.html no longer tags widget scripts one by one; they load through /assets/widgets.js. Assert the page requests the bundle and the bundle contains google-oauth.js, which is what the test was protecting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): address review feedback on #568 - widget bundle version fingerprints every file (name, mtime_ns, size) - gzip fallback appends Accept-Encoding to an existing Vary header - dialog helper: releasing a non-top dialog no longer moves focus out of the dialog the user is in - labels: file-upload targets its file input; fallback config fields get label for/id pairs; native color input has a fallback name - utility audit also reads class names inside bound :class expressions Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): give the native color-picker input an accessible name CodeRabbit flagged this on PR #568 as an outside-diff finding (never posted inline, so it was missed in the round of fixes that addressed the other 6 review comments). The <input type="color"> only carried a title attribute; screen readers don't reliably announce title, and there's no other label naming the control when showHexInput is false. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): clear Codacy findings in app-shell.js - drop the unused catch binding on the SSE JSON parse - move the pending-notification queue assignment out of the expression No behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): contain plugin widgets/ dir and bound style-editor retries From CodeRabbit review on #568 (code that arrived with the main merge): - serve_plugin_widget resolves widgets/ with resolve_under before resolving the manifest script under it, so a symlinked widgets directory can't become the containment base (CWE-22). New test. - style-editor init stops polling after ~10s when the widget never registers and leaves the plain fallback fields in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-10-04 14:25:08 +00:00 · 2026-09-14 09:42:24 -04:00
parent 69d408b321
commit d1e821c625
45 changed files with 2869 additions and 870 deletions
@@ -0,0 +1,25 @@
+"""Regression: the native color input in the color-picker widget must have
+an accessible name independent of showHexInput.
+
+CodeRabbit flagged (PR #568) that the <input type="color"> only carried a
+`title` attribute -- screen readers don't reliably announce `title`, and
+when showHexInput is false there is no other label naming the control.
+"""
+from pathlib import Path
+
+WIDGET_JS = (
+    Path(__file__).resolve().parent.parent
+    / "web_interface" / "static" / "v3" / "js" / "widgets" / "color-picker.js"
+)
+
+
+def test_native_color_input_has_aria_label():
+    source = WIDGET_JS.read_text(encoding="utf-8")
+    start = source.index('<input type="color"')
+    end = source.index(">", start)
+    tag = source[start:end]
+    assert 'aria-label=' in tag, (
+        "native color <input> lost its accessible name; screen readers need "
+        "aria-label since it has no associated <label> and showHexInput can "
+        "be false"
+    )
@@ -164,6 +164,18 @@ def test_static_asset_served(client, asset):
    assert len(resp.data) > 0


+def test_widget_bundle_is_served_and_requested(client):
+    """base.html loads every widget through one versioned bundle request."""
+    page = client.get("/").get_data(as_text=True)
+    assert "/assets/widgets.js?v=" in page
+    resp = client.get("/assets/widgets.js")
+    assert resp.status_code == 200
+    assert resp.mimetype == "application/javascript"
+    body = resp.get_data(as_text=True)
+    assert body.index("/* registry.js */") < body.index("/* notification.js */")
+    assert "/* json-file-manager.js */" in body
+
+
 def test_durations_page_groups_by_plugin(client):
    """One duration input per display mode of each enabled plugin, plus the
    leftover group for saved keys no enabled plugin owns."""
@@ -37,11 +37,11 @@ def test_every_used_breakpoint_class_is_defined():
    class_attr = re.compile(r'class="([^"]*)"')
    bp_class = re.compile(r"\b(%s):[A-Za-z0-9_.-]+" % "|".join(BP_PREFIXES))
    for path in _template_files():
-        for attr in class_attr.findall(path.read_text()):
+        for attr in class_attr.findall(path.read_text(encoding="utf-8")):
            for m in bp_class.finditer(attr):
                used.add(m.group(0))

-    css = APP_CSS.read_text()
+    css = APP_CSS.read_text(encoding="utf-8")
    defined = {
        m.group(0).lstrip(".").replace("\\:", ":")
        for m in re.finditer(
@@ -56,13 +56,90 @@ def test_every_used_breakpoint_class_is_defined():
    )


+_VARIANTS = (
+    r"(?:(?:sm|md|lg|xl|2xl|hover|focus|focus-visible|active|disabled|first|last"
+    r"|group-hover|peer-checked|peer-focus|placeholder|file):)*"
+)
+_UTILITY = re.compile(
+    "^" + _VARIANTS + r"-?(?:"
+    r"(?:block|inline|inline-block|inline-flex|flex|grid|hidden|table|contents"
+    r"|static|fixed|absolute|relative|sticky|truncate|sr-only|transform"
+    r"|uppercase|lowercase|capitalize|italic|underline|line-through|border|shadow"
+    r"|rounded|transition|grow|shrink|group|peer)"
+    r"|(?:m|p)[trblxy]?-[\w./]+"
+    r"|(?:w|h|min-w|min-h|max-w|max-h|gap|gap-x|gap-y|space-x|space-y|top|right"
+    r"|bottom|left|inset|inset-x|inset-y|z|opacity|duration|scale|rotate"
+    r"|translate-x|translate-y|grid-cols|col-span|leading|tracking)-[\w./\[\]]+"
+    r"|(?:text|bg|border|divide|ring|placeholder|accent)-(?:[a-z]+-\d{2,3}(?:/\d+)?"
+    r"|white|black|transparent|xs|sm|base|md|lg|xl|[2-5]xl|left|center|right"
+    r"|opacity-\d+|[trblxy](?:-\d)?|\d|dashed|dotted|offset-\d)"
+    r"|font-(?:mono|sans|serif|thin|light|normal|medium|semibold|bold|extrabold)"
+    r"|select-(?:none|all|text|auto)"
+    r"|(?:rounded|shadow|items|justify|self|flex|whitespace|break|object|list"
+    r"|overflow|overflow-x|overflow-y|cursor|pointer-events|appearance"
+    r"|ease|divide|line-clamp)-[a-z0-9]+(?:-[a-z0-9]+)?(?:\[[^\]]+\])?"
+    r")$"
+)
+
+
+def _css_light_classes(css):
+    """Classes that get a rule outside [data-theme="dark"]."""
+    css = re.sub(r"/\*.*?\*/", "", css, flags=re.S)
+    defined = set()
+    for block in re.finditer(r"([^{}]+)\{", css):
+        for sel in block.group(1).split(","):
+            sel = sel.strip()
+            if sel.startswith("@") or sel.startswith('[data-theme="dark"]'):
+                continue
+            for m in re.finditer(r"\.((?:\\.|[\w-])+)", sel):
+                defined.add(m.group(1).replace("\\", ""))
+    return defined
+
+
+def test_every_used_utility_class_is_defined():
+    """app.css is the whole stylesheet (no Tailwind build), so a utility class
+    it doesn't define silently does nothing. `.hidden` was missing for years,
+    which broke every JS show/hide toggle. Scans templates and static JS."""
+    attr = re.compile(r"""(?:class|className)\s*[=:]\s*(["'`])(.*?)\1""", re.S)
+    class_list = re.compile(r"classList\.(?:add|remove|toggle)\(([^)]*)\)")
+    template_expr = re.compile(r"\{\{.*?\}\}|\{%.*?%\}|\$\{[^}]*\}", re.S)
+    files = list(_template_files()) + [
+        p for p in (STATIC / "v3").rglob("*.js")
+        if "vendor" not in p.parts and not p.name.endswith(".min.js")
+    ]
+    used = {}
+    for path in files:
+        text = template_expr.sub(" ", path.read_text(encoding="utf-8"))
+        chunks = [m.group(2) for m in attr.finditer(text)]
+        chunks += re.findall(r"""['"]([^'"]*)['"]""", " ".join(
+            m.group(1) for m in class_list.finditer(text)))
+        # :class="{ 'hidden': open }" and ternaries hold class names inside
+        # string literals; read those as well as the raw chunk.
+        bound = re.compile(r""":class\s*=\s*"([^"]*)""")
+        for m in bound.finditer(text):
+            chunks.append(m.group(1))
+        for chunk in chunks:
+            candidates = [chunk] + re.findall(r"""['`]([^'`]*)['`]""", chunk)
+            for candidate in candidates:
+                for token in candidate.split():
+                    if _UTILITY.match(token):
+                        used.setdefault(token, path.relative_to(PROJECT_ROOT))
+
+    defined = _css_light_classes(APP_CSS.read_text(encoding="utf-8"))
+    missing = sorted(f"{cls} ({used[cls]})" for cls in used if cls not in defined)
+    assert not missing, (
+        "Utility classes used in templates/JS but not defined in app.css "
+        f"(they silently no-op): {missing}"
+    )
+
+
 def test_every_static_url_for_points_to_a_real_file():
    ref = re.compile(
        r"url_for\(\s*['\"]static['\"]\s*,\s*filename\s*=\s*['\"]([^'\"]+)['\"]"
    )
    missing = []
    for path in _template_files():
-        for filename in ref.findall(path.read_text()):
+        for filename in ref.findall(path.read_text(encoding="utf-8")):
            if not (STATIC / filename).is_file():
                missing.append(f"{path.relative_to(PROJECT_ROOT)}: {filename}")
    assert not missing, f"Templates reference missing static assets: {missing}"
@@ -73,7 +150,7 @@ def test_js_files_calling_debuglog_declare_the_global():
    for path in sorted((STATIC / "v3").rglob("*.js")):
        if "vendor" in path.parts:
            continue
-        text = path.read_text()
+        text = path.read_text(encoding="utf-8")
        # Calls debugLog( but neither defines it nor declares the global
        calls = re.search(r"(?<![.\w])debugLog\(", text)
        defines = "window.debugLog" in text
@@ -1,66 +1,99 @@
-"""Guard: every widget JS file must be loaded by base.html or explicitly allowlisted.
+"""Guard: every widget JS file must ship in the widget bundle or be allowlisted.

 Widget files register themselves with LEDMatrixWidgets at load time; a file
-that exists but is never <script>-included silently breaks any plugin whose
-config schema declares that widget (the field renders as an empty container
-that polls the registry forever). base.html's widget list is maintained by
-hand, so this test keeps it honest.
+that exists but is never loaded silently breaks any plugin whose config schema
+declares that widget (the field renders as an empty container that polls the
+registry forever).
+
+base.html loads them as one concatenated request (web_interface/widget_bundle.py),
+so BUNDLE_ORDER is the hand-maintained list this test keeps honest. It also
+checks that base.html actually requests the bundle, and that the bundle's
+concatenation order puts the registry and base class first.
 """
 import re
+import sys
 from pathlib import Path
 from typing import Set

 PROJECT_ROOT = Path(__file__).resolve().parent.parent
+sys.path.insert(0, str(PROJECT_ROOT))
+
+from web_interface import widget_bundle  # noqa: E402
+
 WIDGETS_DIR = PROJECT_ROOT / 'web_interface' / 'static' / 'v3' / 'js' / 'widgets'
 BASE_HTML = PROJECT_ROOT / 'web_interface' / 'templates' / 'v3' / 'base.html'

-# Matches url_for('static', filename='...') inside actual <script> tags.
-SCRIPT_SRC_RE = re.compile(
-    r"""<script\s[^>]*src="\{\{\s*url_for\(\s*'static'\s*,\s*filename='([^']+)'\s*\)\s*\}\}[^"]*"""
-)
-
-# Files that must NOT be script-included, with the reason.
-ALLOWLIST = {
-    # Documentation example (docs/widget-guide.md); registers the name
-    # 'color-picker' and would shadow the real color-picker.js if loaded.
-    'example-color-picker.js',
-}
+# Files that must NOT be loaded, with the reason.
+ALLOWLIST = set(widget_bundle.EXCLUDED)


-def _included_widget_scripts() -> Set[str]:
-    """Return widget JS basenames referenced by real <script> tags in base.html."""
-    base_html = BASE_HTML.read_text(encoding='utf-8')
-    return {
-        Path(filename).name
-        for filename in SCRIPT_SRC_RE.findall(base_html)
-        if filename.startswith('v3/js/widgets/')
-    }
+def _bundled_names() -> Set[str]:
+    return set(widget_bundle.BUNDLE_ORDER)


-def test_every_widget_script_is_included_in_base_html() -> None:
-    """Every non-allowlisted widget file must be loaded by a <script> tag."""
+def test_every_widget_script_is_bundled() -> None:
+    """Every non-allowlisted widget file must be in BUNDLE_ORDER."""
    assert WIDGETS_DIR.is_dir(), f'Widget directory missing: {WIDGETS_DIR}'
-    included = _included_widget_scripts()
-    assert included, 'No widget <script> tags found in base.html — regex or template drift?'
+    bundled = _bundled_names()
+    assert bundled, 'BUNDLE_ORDER is empty — widget_bundle drift?'
    missing = [
        js_file.name
        for js_file in sorted(WIDGETS_DIR.glob('*.js'))
-        if js_file.name not in ALLOWLIST and js_file.name not in included
+        if js_file.name not in ALLOWLIST and js_file.name not in bundled
    ]
    assert not missing, (
-        'Widget files exist but are never <script>-included in base.html '
-        '(plugins declaring these widgets get blank config fields): '
-        + ', '.join(missing)
-        + '. Add a script tag to base.html or add the file to ALLOWLIST '
-        'with a reason.'
+        'Widget files exist but are never loaded (plugins declaring these '
+        'widgets get blank config fields): ' + ', '.join(missing)
+        + '. Add the file to BUNDLE_ORDER in web_interface/widget_bundle.py, '
+        'or to EXCLUDED with a reason.'
    )


-def test_allowlisted_widgets_are_not_included() -> None:
-    """Allowlisted (must-not-load) widget files must stay out of base.html."""
-    included = _included_widget_scripts()
-    wrongly_included = [name for name in ALLOWLIST if name in included]
+def test_allowlisted_widgets_are_not_bundled() -> None:
+    """Allowlisted (must-not-load) widget files stay out of the bundle."""
+    wrongly_included = sorted(ALLOWLIST & _bundled_names())
    assert not wrongly_included, (
-        'Allowlisted (must-not-load) widget files are script-included in '
-        'base.html: ' + ', '.join(wrongly_included)
+        'Allowlisted (must-not-load) widget files are in the bundle: '
+        + ', '.join(wrongly_included)
    )
+
+
+def test_bundle_order_lists_only_existing_files() -> None:
+    """A renamed or deleted widget must not linger in BUNDLE_ORDER."""
+    stale = [name for name in widget_bundle.BUNDLE_ORDER
+             if not (WIDGETS_DIR / name).is_file()]
+    assert not stale, f'BUNDLE_ORDER names files that do not exist: {stale}'
+
+
+def test_registry_and_base_widget_load_first() -> None:
+    """Widgets call into the registry and base class as they load."""
+    order = widget_bundle.BUNDLE_ORDER
+    assert order[0] == 'registry.js', order[:3]
+    assert order[1] == 'base-widget.js', order[:3]
+
+
+def test_base_html_requests_the_bundle() -> None:
+    """base.html must load the bundle (and no longer tag widgets one by one)."""
+    base_html = BASE_HTML.read_text(encoding='utf-8')
+    assert 'widgets_bundle_url()' in base_html, (
+        'base.html does not request the widget bundle'
+    )
+    per_file = re.findall(
+        r"""<script\s[^>]*src="\{\{\s*url_for\(\s*'static'\s*,\s*"""
+        r"""filename='(v3/js/widgets/[^']+)'""",
+        base_html,
+    )
+    assert not per_file, (
+        'base.html still loads widget files individually alongside the '
+        f'bundle (they would run twice): {per_file}'
+    )
+
+
+def test_bundle_concatenates_every_file() -> None:
+    """The built bundle contains each file, separated so sources can't merge."""
+    body, version = widget_bundle.build_bundle()
+    assert version and version == widget_bundle.bundle_version()
+    for name in widget_bundle.BUNDLE_ORDER:
+        assert f'/* {name} */' in body, f'{name} missing from the built bundle'
+    for name in ALLOWLIST:
+        assert f'/* {name} */' not in body, f'{name} must not be bundled'
@@ -99,9 +99,15 @@ class TestTheRoutesExistAtAll:
        assert "'google-oauth'" in allow_list_line[0], allow_list_line[0]

    def test_the_widget_script_is_served(self):
+        # Widgets load through one bundle (web_interface/widget_bundle.py), so
+        # the page must request the bundle and the bundle must carry the file.
+        from web_interface import widget_bundle
        base = (Path(project_root) / 'web_interface/templates/v3/base.html'
                ).read_text(encoding='utf-8')
-        assert 'widgets/google-oauth.js' in base
+        assert 'widgets_bundle_url()' in base
+        assert 'google-oauth.js' in widget_bundle.BUNDLE_ORDER
+        body, _version = widget_bundle.build_bundle()
+        assert '/* google-oauth.js */' in body

    def test_the_status_line_is_announced(self):
        # Every message the widget gives arrives after an async call, so a
@@ -0,0 +1,68 @@
+"""The stdlib gzip fallback must compress the UI's text assets.
+
+flask-compress is optional (app.py tolerates its absence). Without a fallback a
+Pi missing it ships ~1.2 MB of uncompressed JavaScript to a phone over WiFi on
+every first load and every update. These pin the fallback's contract: text
+assets compress and round-trip byte-for-byte, clients that don't ask for gzip
+get the original bytes, and small or non-text responses are left alone.
+"""
+
+import gzip
+from pathlib import Path
+
+import pytest
+
+STATIC = Path(__file__).resolve().parents[2] / "web_interface" / "static"
+
+
+@pytest.fixture
+def client(monkeypatch):
+    import web_interface.app as web_app
+    # Exercise the fallback even on machines that do have flask-compress.
+    monkeypatch.setattr(web_app, "_HAVE_FLASK_COMPRESS", False)
+    web_app.app.config["TESTING"] = True
+    with web_app.app.test_client() as c:
+        yield c
+
+
+@pytest.mark.parametrize("url,path", [
+    ("/static/v3/plugins_manager.js", "v3/plugins_manager.js"),
+    ("/static/v3/app.css", "v3/app.css"),
+])
+def test_static_text_asset_is_gzipped_and_round_trips(client, url, path):
+    resp = client.get(url, headers={"Accept-Encoding": "gzip, deflate"})
+    assert resp.status_code == 200
+    assert resp.headers.get("Content-Encoding") == "gzip"
+    assert "Accept-Encoding" in resp.headers.get("Vary", "")
+    original = (STATIC / path).read_bytes()
+    assert gzip.decompress(resp.data) == original
+    assert len(resp.data) < len(original) / 2
+    assert int(resp.headers["Content-Length"]) == len(resp.data)
+
+
+def test_repeat_request_serves_identical_compressed_bytes(client):
+    headers = {"Accept-Encoding": "gzip"}
+    first = client.get("/static/v3/js/app-shell.js", headers=headers).data
+    second = client.get("/static/v3/js/app-shell.js", headers=headers).data
+    assert first == second
+
+
+def test_client_without_gzip_gets_the_original_bytes(client):
+    resp = client.get("/static/v3/app.css", headers={"Accept-Encoding": "identity"})
+    assert resp.status_code == 200
+    assert "Content-Encoding" not in resp.headers
+    assert resp.data == (STATIC / "v3/app.css").read_bytes()
+
+
+def test_binary_assets_are_not_recompressed(client):
+    resp = client.get("/static/v3/icons/icon-192.png", headers={"Accept-Encoding": "gzip"})
+    assert resp.status_code == 200
+    assert "Content-Encoding" not in resp.headers
+
+
+def test_widget_bundle_is_gzipped_and_immutable(client):
+    resp = client.get("/assets/widgets.js", headers={"Accept-Encoding": "gzip"})
+    assert resp.status_code == 200
+    assert resp.headers.get("Content-Encoding") == "gzip"
+    assert "immutable" in resp.headers.get("Cache-Control", "")
+    assert b"/* registry.js */" in gzip.decompress(resp.data)
@@ -165,6 +165,27 @@ class TestPathTraversal:
        assert "hunter2" not in r.get_data(as_text=True)


+    def test_a_symlinked_widgets_directory_cannot_escape(self, tmp_path, make_client):
+        """widgets/ itself is contained before the script is resolved under it,
+        so a symlink out of the plugin can't become the containment base."""
+        outside = tmp_path / "outside"
+        outside.mkdir()
+        (outside / "loot.js").write_text("const KEY='hunter2';", encoding="utf-8")
+        d = tmp_path / "soccer-scoreboard"
+        d.mkdir()
+        manifest = {"id": "soccer-scoreboard", "name": "soccer-scoreboard",
+                    "version": "1.0.0",
+                    "widgets": [{"name": "loot", "script": "loot.js"}]}
+        (d / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8")
+        try:
+            (d / "widgets").symlink_to(outside, target_is_directory=True)
+        except (OSError, NotImplementedError):
+            pytest.skip("symlinks unavailable on this platform")
+        r = make_client().get(URL.format("soccer-scoreboard", "loot"))
+        assert r.status_code == 404
+        assert "hunter2" not in r.get_data(as_text=True)
+
+
 class TestDegradation:
    def test_an_unknown_plugin_is_404(self, tmp_path, make_client):
        assert make_client().get(URL.format("nope", "w")).status_code == 404