opensource-joe commented on code in PR #365: URL: https://github.com/apache/fineract-backoffice-ui/pull/365#discussion_r3792486999
########## scripts/check-a11y-names.mjs: ########## @@ -0,0 +1,164 @@ +#!/usr/bin/env node +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Verifies that every icon-only `<ion-button>` has an accessible name. + * + * A button whose only content is an `<ion-icon>` has nothing to compute a name from: the + * icon is a font glyph, not text. A screen reader announces the control as "button" and + * nothing else, so a row of them is a row of identical buttons, including the ones that + * delete a record. + * + * Three things look like they solve this. Two of them do not: + * + * - `[appTooltip]`. `TooltipDirective` sets `aria-describedby`. A description is not a + * name: it is never consulted by the accessible name computation, it is only present + * 300ms after hover or focus, and a screen reader user reading in browse mode never + * triggers it at all. + * - `title`. `<ion-button>` renders a native `<button>` into its shadow root, and that + * inner element is what carries `role=button`. Ionic forwards `aria-label` to it but not + * `title`, so a title names the outer host, which the accessibility tree exposes as + * `role=generic`, and the button itself stays anonymous. Read out of Chromium's own + * accessibility tree rather than inferred: `title` on the host gives `role=generic + * name="Edit"` sitting over `role=button name=""`, while `aria-label` on the same host + * gives `role=button name="Edit"`. + * - An `aria-label` on the `<ion-icon>` itself. This one does work, because + * name-from-content descends into children, and the check accepts it. + * + * The fix is `[attr.aria-label]` on the button, bound to the translation key that already + * names the action. + * + * Usage: node scripts/check-a11y-names.mjs + */ + +import { readFileSync, readdirSync, statSync } from 'node:fs'; +import { join, relative } from 'node:path'; + +const SRC = 'src'; +const TAG = 'ion-button'; + +/** Attributes that name the element outright, static or bound. */ +const NAMING_ATTRIBUTE = /(?:\[attr\.)?aria-label(?:ledby)?\]?=|aria-labelledby=/; +/** An `<ion-icon>` element, self-closed or paired. Its content is a glyph, never text. */ +const ICON = /<ion-icon\b[^>]*?(?:\/>|>[\s\S]*?<\/ion-icon>)/g; +const COMMENT = /<!--[\s\S]*?-->/g; +/** Angular control flow left behind once the icons inside it are removed. */ +const CONTROL_FLOW = /@(?:if|else|for|empty|switch|case|default)\b[^{]*\{|\}/g; + +function walk(dir) { + const out = []; + for (const entry of readdirSync(dir)) { + const path = join(dir, entry); + if (statSync(path).isDirectory()) { + // Generated API client has no templates. + if (entry === 'api' || entry === 'node_modules') continue; + out.push(...walk(path)); + } else if (path.endsWith('.ts') || path.endsWith('.html')) { + if (path.endsWith('.spec.ts')) continue; + out.push(path); + } + } + return out; +} + +/** + * End index of the opening tag that starts at `start`, tracking quotes. + * + * A plain search for `>` is wrong here: binding expressions contain them, and + * `[disabled]="from > to"` would truncate the tag halfway through its own attributes, + * hiding every attribute after it, including the aria-label this check looks for. + */ +function endOfOpeningTag(source, start) { + let quote = null; + for (let i = start; i < source.length; i++) { + const char = source[i]; + if (quote) { + if (char === quote) quote = null; + } else if (char === '"' || char === "'") { + quote = char; + } else if (char === '>') { + return i; + } + } + return -1; +} + +/** Every `<ion-button>` in `source`. Buttons cannot nest, so the first close tag is ours. */ +function buttons(source) { + const found = []; + const opening = new RegExp(`<${TAG}(?=[\\s/>])`, 'g'); + + for (const match of source.matchAll(opening)) { + const tagEnd = endOfOpeningTag(source, match.index); + if (tagEnd === -1) continue; + + const openTag = source.slice(match.index, tagEnd + 1); + if (openTag.endsWith('/>')) { + found.push({ openTag, content: '', index: match.index }); + continue; + } + + const close = source.indexOf(`</${TAG}>`, tagEnd); + if (close === -1) continue; + found.push({ openTag, content: source.slice(tagEnd + 1, close), index: match.index }); + } + return found; +} + +/** True when the button renders icons and nothing a name could be computed from. */ +function isIconOnly(content) { + ICON.lastIndex = 0; + if (!ICON.test(content)) return false; + ICON.lastIndex = 0; + + return content.replace(COMMENT, '').replace(ICON, '').replace(CONTROL_FLOW, '').trim() === ''; Review Comment: @Aman-Mittal Looked at it, and it is fixed in `2b7b1f1`. The alert is `js/incomplete-multi-character-sanitization`, on the comment strip in `isIconOnly`. The query fires on removing a multi-character delimiter such as `<!--` in a single `String.replace` pass, because on input that can nest, one pass leaves behind a delimiter a second pass would have caught. I want to say why I do not think it was exploitable here before saying what I changed, since "the scanner is wrong" is a claim worth showing the work on: 1. This is a build-time script. It reads files already in the repo, runs in the `i18n-check` job, and produces an exit code and a list of `file:line`. Nothing it computes reaches a DOM, a response body, a template or a shell. 2. HTML comments do not nest, so the non-greedy `<!--[\s\S]*?-->` is the correct parse rather than an approximation of one. The failure mode the query describes needs nesting to exist. 3. If a delimiter did survive anyway, the consequence is a false negative in a lint check: a button that should have been flagged gets skipped. That is a missed accessibility bug, not an injection. None of which I thought was a good reason to ask you to dismiss it. This script gates other people's builds, so anyone who trips the alert later has to re-derive all three points before they can trust it, and a `replace`-chain sanitizer is exactly the shape that gets copied out of a repo into somewhere the threat model is real. So `isIconOnly` now walks the content once instead of stripping it: skip a comment, skip an `<ion-icon>`, skip Angular control flow, skip whitespace, and treat anything else as content a name could be computed from. There is no `String.replace` left in it, so the query has nothing to match on. ## Verification All on a clean checkout of the branch, x86_64 Linux. **Classification is unchanged.** I removed the naming-attribute guard from both the old and the new implementation, so each lists every icon-only button rather than only the unnamed ones, then diffed the two lists. Both find the same **192** icon-only buttons across `src`, diff empty. | | | |---|---| | `node scripts/check-a11y-names.mjs` | exit 0 | | delete the `[attr.aria-label]` at `accounting-closures-list.component.ts:74` | exit 1, naming `accounting-closures-list.component.ts:69` | | restore it | exit 0 | | `npm run lint` | exit 0 | | `npm run lint:prune` | exit 0 | | `npm run format:check` | exit 0 | I did not re-run the unit suite for this commit. It touches only `scripts/`, and no file under `src/` differs from the run that was already green on this PR, so CI's own run is the one worth reading rather than a repeat of mine. ## One thing that fell out of it The walk is stricter than the regex it replaces, in a case that surprised me. It ends an icon's opening tag with `endOfOpeningTag`, which tracks quotes, so a self-closing `<ion-icon>` carrying a `>` inside one of its bindings is now seen. The old `[^>]*?` could not match that icon at all, which left the icon's own markup sitting in the button's content, so the button read as "has content", and it escaped the check silently. Confirmed with a throwaway component rather than assumed: the old implementation does not flag it, the new one does. Nothing in `src` hits that shape today, which is why the 192 come out identical, but the check is now correct about it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
