opensource-joe opened a new issue, #354: URL: https://github.com/apache/fineract-backoffice-ui/issues/354
## What is wrong 44 icon-only buttons have no accessible name. They carry `appTooltip` and nothing else, and `appTooltip` does not name a control. This is the follow-up I offered in [#338](https://github.com/apache/fineract-backoffice-ui/pull/338), which fixed 34 of the buttons #233 identified and left these behind. On coming back to them I found the reason they were left is more interesting than "stragglers", so this issue re-states the problem rather than just carrying the number over. #233 says, correctly for the pattern as intended, that a tooltip "also gives them their accessible name". The directive does not do that. From `src/app/shared/directives/tooltip.directive.ts`: ```ts this.renderer.setAttribute(this.host.nativeElement, 'aria-describedby', this.id); ``` Three things follow from `aria-describedby` rather than `aria-label`: 1. **A description is not a name.** In the accessible name computation, `aria-describedby` is never consulted. It supplements a name, it does not supply one. A button with a description and no name is announced as "button", and the description is read after that, if the user's settings read descriptions at all. 2. **The attribute does not exist at rest.** It is set in `show()`, 300ms after `mouseenter` or `focusin`, and removed again in `hide()`. So for anything inspecting the button in its resting state, including axe, there is no `aria-describedby` there to find. 3. **Browse mode never triggers it.** `show()` is bound to `mouseenter` and `focusin`. A screen-reader user reading through a table in browse mode is not focusing these buttons and is not hovering them, so the tooltip never fires and the description never exists. The 77 icon-only buttons that also carry `[attr.aria-label]` are fine, and they are fine because of the `aria-label`, not the tooltip. The 44 in this issue rely on the tooltip alone. ```html <!-- one of the 44, src/app/features/clients/tabs/client-notes-list.component.ts --> <ion-button fill="clear" [appTooltip]="'COMMON.EDIT' | translate" (click)="onEdit(note)"> <ion-icon name="create-outline"></ion-icon> </ion-button> ``` ## Business value Same as #233, and the same rows: on a row of icon buttons the icon is the only thing separating edit from delete, and a screen-reader user gets "button, button, button" with nothing to choose from. Several of these sit on client documents, client identifiers, loan documents and delinquency management, where the wrong guess destroys a record. The difference from #233 is that these 44 look correct in review. They have a tooltip, the tooltip has a translated string, and the pattern matches the 246 uses elsewhere. Nothing about reading the template says the button is unnamed, which is why they survived the first pass. That makes this worth fixing as a stated rule rather than a one-off sweep: `appTooltip` describes, `aria-label` names, and an icon-only control needs both. ## Finding them Measured against `main` at 98770c4: ``` elements with appTooltip: 256 of which icon-only: 121 already have a name: 77 UNNAMED: 44 (23 files) ``` Element-level rather than line-level, since a tag's attributes and its content span several lines here: ```bash python3 - <<'PY' import pathlib, re TAG = re.compile(r"<([a-zA-Z][\w-]*)((?:[^<>]|\"[^\"]*\")*?)(/?)>", re.S) hits = 0 for p in sorted(pathlib.Path('src').rglob('*.ts')): if p.name.endswith('.spec.ts'): continue s = p.read_text(errors='ignore') if 'appTooltip' not in s: continue for m in TAG.finditer(s): attrs = m.group(2) if 'appTooltip' not in attrs: continue if re.search(r'\baria-label(ledby)?\b', attrs): continue close = s.find('</%s>' % m.group(1), m.end()) inner = s[m.end():close] if close > 0 else '' text = re.sub(r'<[^>]+>', '', re.sub(r'<!--.*?-->', '', inner, flags=re.S)).strip() if '<ion-icon' in inner and not text: hits += 1 print('%s:%d' % (p, s[:m.start()].count('\n') + 1)) print(hits, 'unnamed icon-only buttons') PY ``` The 23 files, worst first: `client-view.component.ts` (9), `delinquency-management.component.ts` (4), then two each in `client-documents-list`, `client-family-members-list`, `client-identifiers-list`, `client-notes-list`, `loan-documents-tab`, `loan-form`, `fixed-deposit-form`, `recurring-deposit-form`, `savings-account-form`, `datatables-list`, and one each in eleven more. ## Scope Add `[attr.aria-label]` bound to a translation key on each of the 44, reusing the existing key wherever the tooltip already names the action, exactly as #338 did for the other 34. The tooltips stay: they are useful to sighted users and they are the correct use of `aria-describedby` once a name exists alongside them. Two questions worth settling here rather than in review: - **Should the directive do this itself?** It knows the string. It could set `aria-label` when the host has no name, which would fix all 44 and prevent the next one. I did not propose that in #338 because it changes shared behaviour for 256 call sites, some of which are not icon-only and already have text, and a directive that silently renames controls is harder to reason about than an explicit attribute. Happy to do it either way, and I would rather be told than choose. - **A lint rule or a test** so this does not regrow. `sonarjs` will not catch it, but a spec that walks the templates the way the script above does would. I am picking this up unless someone else already has it. I will hold off opening a PR for a few days in case the directive question changes the shape of the fix. -- 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]
