opensource-joe opened a new pull request, #365:
URL: https://github.com/apache/fineract-backoffice-ui/pull/365
## What and why
44 icon-only `<ion-button>`s across 23 files had no accessible name, so a
screen reader announced each one as "button" and nothing else. On a row of icon
buttons that is a row of identical buttons, including the ones that delete a
record.
These are the 44 that #338 could not reach, because issue #233's own
detection script skips any button carrying an `appTooltip`, on the premise that
the tooltip already names it. It does not. `TooltipDirective` sets
`aria-describedby` and never `aria-label`, and only 300ms after hover or focus.
A description is never consulted by the accessible name computation, it is
absent at rest, and a screen reader user reading in browse mode never triggers
it at all.
Three commits, meant to be read in order:
1. **`fix(a11y)`** adds `[attr.aria-label]` to the 44, bound to the
translation key the tooltip already uses. Same shape as #338.
2. **`build(a11y)`** adds `scripts/check-a11y-names.mjs`, which fails the
build on an icon-only `<ion-button>` with no accessible name. This is the part
with lasting value: the pattern is easy to reintroduce precisely because the
page looks correct on screen. It runs in the `i18n-check` job beside
`check:icons`, where this repo already collects checks for failures that are
silent at runtime.
3. **`test(tooltip)`** adds the spec `tooltip.directive.ts` was missing,
while `has-permission` and `has-institution-feature` both have one.
Closes #354
One finding worth surfacing, because it is not obvious and I did not want it
rediscovered in review. `title` does not fix this either, and not for the
reason you would guess. `<ion-button>` renders a native `<button>` into its
shadow root, and that inner element 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`, leaving the button itself
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"`.
That is recorded in the check script's header so the next person does not have
to derive it.
## Verification
Measured against `main` at `48417d0`, element-level rather than line-level,
since a tag's attributes and its content span several lines in these inline
templates:
```
elements with appTooltip: 256
of which icon-only: 121
already had a name: 77
UNNAMED: 44 (23 files)
```
The count was derived independently twice, a day apart and against two
different `main` commits, and came out at 44 both times.
Everything below was run on a clean checkout of this branch:
| | |
|---|---|
| `npm run lint` | exit 0 |
| `npm run lint:prune` | exit 0 |
| `npm run format:check` | exit 0 |
| `npx ng test fineract-backoffice-ui` | **962 of 962 SUCCESS**, real
headless Chromium |
| `tooltip.directive.spec.ts` alone | 10 of 10 SUCCESS |
| `node scripts/check-a11y-names.mjs` | exit 0 on this branch |
**The guard was tested in both directions**, since a check that only ever
passes proves nothing. Removing a single `[attr.aria-label]` makes it exit 1
and name the file, the line and the fix. Restoring it returns it to exit 0.
The UI itself was not exercised against a backend. This change adds an
attribute and a build-time check; it alters no behaviour, no request and no
rendered layout.
## Screenshots
Not applicable. Nothing here changes anything visible on screen, which is
the whole difficulty with this class of bug.
## Checklist
- [x] I did not hand-edit generated files under `src/app/api/`.
- [x] New component or service code uses the adapter boundary in
`src/app/core/adapters/` instead of direct browser globals or imperative
third-party APIs. No new component or service code; the 44 edits reuse the
`translate` pipe each file already imports, so no new import crosses the
boundary.
- [x] User-facing strings use translation keys. Every label reuses the key
its tooltip already uses, so no new `en.json` entries were needed.
- [x] I added or updated tests appropriate to this change. The build check
is the regression guard; the directive spec covers the behaviour the check
depends on.
- [ ] UI workflow changes include suitable e2e coverage, including
real-backend testing where relevant. Not applicable, no workflow changes.
- [x] Commits are signed.
## One thing to flag before review
On these 44 the label and the description end up as the same string, so a
focused screen reader user hears it twice once the tooltip appears. I think
that is the right trade against a button with no name at all, and you said the
same when we discussed it on #354, but it is a real consequence and I would
rather it was stated here than found in review.
--
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]
