Skip to content

fix(a11y): four widget icons rendered the wrong glyph, three controls were keyboard-dead - #458

Open
rubenvdlinde wants to merge 2 commits into
developmentfrom
fix/a11y-and-icon-registry
Open

fix(a11y): four widget icons rendered the wrong glyph, three controls were keyboard-dead#458
rubenvdlinde wants to merge 2 commits into
developmentfrom
fix/a11y-and-icon-registry

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Full-tree measurement of origin/development with the hydra-gates runner from ConductionNL/.github@main — no diff scope. Four gates fixed, eleven findings, all real. Two more gates' findings are false positives and are reported rather than "fixed".

gate-55 — four widget icons that do not exist

Page / widget was now
ModuleDetail.md-files BookOpenVariantOutline BookOpenVariant
ModuleDetail.md-versions ViewModule SourceBranch
SuiteDetail.suite-data PackageVariant Package
BioMaatregelDetail.bm-data ShieldLockOutline ShieldCheckOutline

CnWidgetGrid's resolveWidgetIcon() falls back to DEFAULT_ICON'ViewDashboard' — for any name not in its registry. These four widgets rendered a generic dashboard glyph, silently: no console warning, no build error, and check:manifest passes because the schema types icon as a plain string.

Verified against the real registry, not the gate's copy of it

hydra-gates gate-55 carries a hardcoded mirror of the registry and says so in its own comments. A mirror is only as fresh as its last manual edit and fails in both directions — inventing findings for icons added upstream, passing icons removed upstream. So the finding was checked against the dependency this app actually resolves:

  • node_modules/@conduction/nextcloud-vue/src/components/CnWidgetGrid/widgetIcons.js at the pinned 2.2.0-vue3.3: all four flagged names absent, near-neighbours BookOpenVariant / Package / ShieldCheckOutline present.
  • git log -S over that file in the nextcloud-vue repo: none of the four was ever present, so this is not a version-skew artefact.
  • An independent scan of every widget icon in the manifest found exactly the same four — an agreement between two instruments built differently.

gate-32 — three controls no keyboard can reach

Component What it is Fix
EmailConfiguration template-variable tags — a clickable <span> that inserts a variable into the editor a real <button type="button">, with the UA chrome reset in CSS so the rendering is unchanged
MergeObject merge-target rows — a click-only <div> choosing which object survives a merge, the consequential decision in that dialog role="option" + tabindex + Enter/Space, inside a role="listbox" parent so the selected state reaches a screen reader
OrganisatieCard the whole card navigates to the organisation role="button" + tabindex + Enter/Space; the nested .cardHeaderActions already stops propagation, so the NcActions menu is unaffected

gate-39 — two icon-only buttons with no accessible name

ViewObject's "cancel label editing" button sits directly beside a "save labels" button that already carries an :aria-label — the sibling gave the exact pattern to copy. ArchiMateImportExport's error-details close button had none either. v-tooltip is not an accessible name.

gate-58 — two networkidle waits in e2e

Nextcloud keeps long-lived connections open, so waitUntil: 'networkidle' never settles — it can only time out or be satisfied by luck (ADR-074 rule 4). Both call sites already assert on a heading immediately afterwards, which is the real readiness signal, so the wait was doing no work it could reliably do. A third site in the same file already had the right value and the comment explaining why.

Evidence

tests/vitest/manifestWidgetIcons.spec.js asserts every widget icon in src/manifest.json against the registry parsed out of the installed dependency.

Can-fail proof: reverting src/manifest.json makes it name all four sites by page and widget id —

+ [
+   "ModuleDetail.md-files: BookOpenVariantOutline",
+   "ModuleDetail.md-versions: ViewModule",
+   "SuiteDetail.suite-data: PackageVariant",
+   …

The positive controls earned their place immediately. The test carries two controls on its inputs. The first version of the manifest walker assumed page.widgets; widgets actually live under config.widgets and nested arrays at a depth that varies by page type. It collected zero icons — and would have reported a clean tree forever. The "finds widget icons to check" control failed and caught it before the real assertion could pass vacuously.

Two findings deliberately NOT "fixed" — the code is already right

gate-12 (nc-input-labels, 2 findings) — false positive. Both <NcSelect> tags in EmailConfiguration.vue do carry input-label: "Transport Type" (line 106) and "Encryption" (line 137). The gate extracts the tag with

grep -oE '<NcSelect[^>]*>'

and [^>]* stops at the first > — which is the arrow in :reduce="option => option.value". The extracted "tag" therefore ends before input-label can appear. The finding text in the log shows the truncation directly: it ends at :reduce="option =>. Same shape as the gate-9 admin rule's [^)]* (.github#198). Filed upstream.

gate-38 (skip-link, 1 finding) — false positive. templates/settings/admin.php is a Nextcloud admin-settings section, registered via lib/Settings/SoftwareCatalogAdmin.php and rendered inside core's settings frame, which already provides the skip link and the <NcContent> shell. It is not a root component; adding a second skip target would be a regression. Filed upstream.

One more measured gate behaviour, worth knowing

After the gate-32 fixes, the gate still reported the same three files — matching the explanatory comments this PR adds, because those comments contained the literal text of the markup they describe (<span @click>, <div @click>). Rewording them to prose cleared the gate, which confirms the mechanism rather than assuming it. Same class as .github#184: a checker that greps a string literal matches every comment. Filed upstream.

Checks

Check Result
eslint on all changed .vue 0 errors (128 pre-existing jsdoc warnings, untouched)
stylelint 0 errors
check:manifest Ajv validation PASS (0 errors), schema 2.22.0
vitest 213 passed (210 before, 3 added)
hydra-gates full-tree: gate-32 / 39 / 55 / 58 all PASS

… were keyboard-dead

Full-tree measurement of origin/development with the hydra-gates runner
from ConductionNL/.github@main. Four gates, eleven findings, all real —
plus two gates whose findings are false positives and are reported
rather than "fixed".

GATE-55 — FOUR WIDGET ICONS THAT DO NOT EXIST (detail-page-discipline).

  ModuleDetail.md-files    BookOpenVariantOutline -> BookOpenVariant
  ModuleDetail.md-versions ViewModule             -> SourceBranch
  SuiteDetail.suite-data   PackageVariant         -> Package
  BioMaatregelDetail.bm-data ShieldLockOutline    -> ShieldCheckOutline

CnWidgetGrid's resolveWidgetIcon() falls back to DEFAULT_ICON —
'ViewDashboard' — for any name not in its registry, so these four widgets
rendered a generic dashboard glyph. Silently: no console warning, no
build error, and `check:manifest` passes because the schema types `icon`
as a string.

Verified against the REAL registry, not the gate's copy of it.
hydra-gates gate-55 carries a hardcoded mirror and says so in its own
comments; a mirror is only as fresh as its last manual edit and fails in
both directions. Read from
node_modules/@conduction/nextcloud-vue/src/components/CnWidgetGrid/widgetIcons.js
at the pinned 2.2.0-vue3.3: all four flagged names are absent, and
`git log -S` over that file shows none of them was ever present. The
replacements are the nearest true members of the same glyph family.

GATE-32 — THREE CONTROLS NO KEYBOARD CAN REACH (semantic-controls).

  EmailConfiguration  the template-variable tags: a clickable <span> that
                      inserts a variable into the editor. Now a real
                      <button type="button">, with the UA chrome reset in
                      CSS so the rendering is unchanged.
  MergeObject         the merge-target rows: a click-only <div> choosing
                      which object survives a merge — the consequential
                      decision in that dialog. Now role="option" +
                      tabindex + Enter/Space, inside a role="listbox"
                      parent so the selected state reaches a screen
                      reader.
  OrganisatieCard     the whole card navigates to the organisation. Now
                      role="button" + tabindex + Enter/Space; the nested
                      .cardHeaderActions already stops propagation, so
                      the NcActions menu is unaffected.

GATE-39 — TWO ICON-ONLY BUTTONS WITH NO ACCESSIBLE NAME (button-name).

ViewObject's "cancel label editing" button sits directly beside a "save
labels" button that already carries an :aria-label — the sibling gave the
exact pattern. ArchiMateImportExport's error-details close button had
none either. v-tooltip is not an accessible name.

GATE-58 — TWO networkidle WAITS IN E2E (e2e-networkidle).

Nextcloud keeps long-lived connections open, so `waitUntil: 'networkidle'`
never settles; it can only time out or be satisfied by luck (ADR-074
rule 4). Both call sites already assert on a heading immediately
afterwards, which is the real readiness signal, so the wait was doing no
work it could reliably do. A third site in the same file already had the
right value and the comment explaining why.

Full-tree after: gate-32, gate-39, gate-55 and gate-58 all PASS.

EVIDENCE. tests/vitest/manifestWidgetIcons.spec.js asserts every widget
icon in src/manifest.json against the registry parsed out of the
installed dependency. Reverting src/manifest.json makes it name all four
sites by page and widget id.

It carries two positive controls on its INPUTS, and the second one earned
its place immediately: the first version of the manifest walker assumed
`page.widgets`, but widgets live under `config.widgets` and nested arrays
at a depth that varies by page type. It collected ZERO icons — and would
have reported a clean tree forever. The "finds widget icons to check"
control failed and caught it before the real assertion could pass
vacuously.

TWO FINDINGS DELIBERATELY NOT "FIXED", because the code is already right:

gate-12 (nc-input-labels, 2) is a false positive. Both <NcSelect> tags in
EmailConfiguration.vue DO carry input-label — "Transport Type" and
"Encryption". The gate extracts the tag with `grep -oE '<NcSelect[^>]*>'`,
and `[^>]*` stops at the first `>`, which is the arrow in
`:reduce="option => option.value"`. The extracted "tag" therefore ends
before input-label can appear. Same shape as the gate-9 admin rule's
`[^)]*` (.github#198). Filed upstream.

gate-38 (skip-link, 1) is a false positive. templates/settings/admin.php
is a Nextcloud admin-settings section, registered via
lib/Settings/SoftwareCatalogAdmin.php and rendered INSIDE core's settings
frame, which already provides the skip link and the <NcContent> shell.
It is not a root component; adding a second skip target would be a
regression. Filed upstream.

Also measured, and worth knowing: gate-32's remaining three findings
after the fix were matching the explanatory COMMENTS this commit added,
because those comments contained the literal text of the markup they
describe. Rewording them to prose cleared the gate — confirming the
mechanism. Filed upstream too; the same class as .github#184.

Checks: eslint 0 errors, stylelint 0 errors, check:manifest Ajv PASS,
vitest 213 green (210 before, 3 added).
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/softwarecatalog @ 40e3b58

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
composer ✅ 128/128
npm ✅ 718/718
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-08 11:23 UTC

Download the full PDF report from the workflow artifacts.

There are TWO icon registries and an icon must be in both.

CnWidgetGrid resolves widget icons through nc-vue's widgetIcons.js;
CnAppNav, CnIcon and the Cn*Page headers resolve through this app's own
src/icons.js (ADR-077). The lists overlap but are not equal, and the
failure modes differ: an unknown name in the widget registry renders
DEFAULT_ICON, while an unknown name in the app registry renders NO ICON
AT ALL — src/icons.js says exactly that in its own header.

The first pass of this branch replaced four unknown widget icons with
names taken from the widget registry alone. Two of them, BookOpenVariant
and SourceBranch, were absent from src/icons.js, so the repair traded a
wrong glyph for no glyph. hydra-gates checks the two registries in two
different gates — 55 and 60 — so neither on its own would have said so,
and my local baseline run had reported gate-60 PASS while printing
'vue-material-design-icons is not installed — could not verify that icon
names exist upstream'. A caveated pass is not a pass.

Both names are verified present in vue-material-design-icons (the .vue
files exist in the installed package, and nc-vue imports them).

The vitest spec now checks BOTH registries, with its own positive control
on each. Reverting src/icons.js alone turns the new arm red and names
both icons.
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Hydra Gates on this PR: what is red and why

Six gates fail. Five are development's own debt by name; one was mine and is fixed in adf353d.

gate on development full-tree verdict
gate-12 nc-input-labels (2) FAIL (2) false positive, see the PR body — [^>]* truncates at the arrow in :reduce="option => …". .github#236
gate-40 form-label-association (7) FAIL (19) pre-existing subset
gate-43 table-headers (8) FAIL (19) pre-existing subset
gate-45 prefers-reduced-motion (5) FAIL (10) pre-existing subset
gate-60 icon-vocabulary (1) PASS mine — fixed in adf353d
gate-63 settings-surface (2) "PASS" pre-existing, newly visible — see below

gate-60 was my own defect and it is the useful lesson of this PR. There are two icon registries: nc-vue's widgetIcons.js (what gate-55 checks, unknown name → DEFAULT_ICON) and this app's src/icons.js (what gate-60 checks, unknown name → no icon at all). Two of my four replacements were in the first and not the second, so the repair traded a wrong glyph for no glyph. Neither gate alone would have said so.

My local baseline had reported gate-60: PASS while printing "vue-material-design-icons is not installed — could not verify that icon names exist upstream." A caveated pass is not a pass, and CI — which installs the dependency — measured what I could not. Fixed by registering both names in src/icons.js, and the vitest spec now checks both registries with a positive control on each.

gate-63 is pre-existing debt that this PR merely makes visible. The gate is diff-scoped to the manifest and says so in its own log when it does nothing:

No changed manifest / menu-layout — gate skipped (ADR-020 diff scoping).

src/manifest.json has not been touched on development for long enough that every gate-63 run in this repo has been a skip printed as PASS. Changing four icon strings pulls the whole file into scope and surfaces two ADR-079 findings that were always there — including a navigation foldout that renders Settings > Settings.

Proven pre-existing rather than inferred from counts: reverting src/manifest.json to origin/development's exact bytes while keeping it in scope reproduces both findings identically. Filed as #460; both fixes are renames on a live navigation surface and belong in their own change with their own e2e evidence, not bolted onto an accessibility PR.

Merge posture

This PR is therefore not a clean strict-subset by name: gate-63 is red here and green-by-skip on development. That is a blocker I am leaving visible rather than silencing — no waiver, no exclude, no baseline entry. It clears either by landing #460 first, or by an explicit decision to accept a gate that was never measuring the file.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/softwarecatalog @ 94469c9

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
composer ✅ 128/128
npm ✅ 718/718
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-08 11:43 UTC

Download the full PDF report from the workflow artifacts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant