Improve screen reader announcements for auth feedback#1603
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces accessibility enhancements across several components by adding ARIA roles and live region attributes to loading indicators, error messages, and status updates. The review feedback identifies several opportunities to improve these changes by removing redundant attributes where roles already imply specific live region behaviors (e.g., role="status" and role="alert"). Additionally, it is recommended to use aria-live="polite" instead of "assertive" for form validation errors to avoid disruptive screen reader announcements during user interaction.
| <div *ngIf="!selNode.settings.themeColor" class="rtl-spinner" role="status" aria-live="polite" aria-atomic="true"> | ||
| <mat-spinner color="accent" aria-label="Loading RTL" /> |
There was a problem hiding this comment.
The role="status" attribute implicitly includes aria-live="polite" and aria-atomic="true", making the explicit attributes redundant. Additionally, the aria-label on mat-spinner repeats the text in the h4 element, which will cause screen readers to announce the message twice. It is better to hide the decorative spinner from assistive technology.
| <div *ngIf="!selNode.settings.themeColor" class="rtl-spinner" role="status" aria-live="polite" aria-atomic="true"> | |
| <mat-spinner color="accent" aria-label="Loading RTL" /> | |
| <div *ngIf="!selNode.settings.themeColor" class="rtl-spinner" role="status"> | |
| <mat-spinner color="accent" aria-hidden="true" /> |
| <mat-label>Password</mat-label> | ||
| <input autoFocus matInput type="password" id="password" name="password" tabindex="1" required [(ngModel)]="password"> | ||
| <mat-error *ngIf="!password">Password is required.</mat-error> | ||
| <mat-error *ngIf="!password" role="alert" aria-live="assertive">Password is required.</mat-error> |
There was a problem hiding this comment.
Using aria-live="assertive" for form validation errors is generally discouraged as it can interrupt the user's current interaction. mat-error is already associated with the input via aria-describedby in Angular Material. If a live announcement is needed when the error appears, aria-live="polite" is the standard approach to ensure the user is notified without being abruptly interrupted.
| <mat-error *ngIf="!password" role="alert" aria-live="assertive">Password is required.</mat-error> | |
| <mat-error *ngIf="!password" aria-live="polite">Password is required.</mat-error> |
| <mat-icon>{{flgShow ? 'visibility_off' : 'visibility'}}</mat-icon> | ||
| </button> | ||
| <mat-error *ngIf="!password">Password is required.</mat-error> | ||
| <mat-error *ngIf="!password" role="alert" aria-live="assertive">Password is required.</mat-error> |
There was a problem hiding this comment.
Inline validation errors should typically use aria-live="polite". Assertive announcements are reserved for critical, time-sensitive information and can be disruptive when triggered during form entry. Since Angular Material already links mat-error to the input via aria-describedby, a polite announcement is sufficient for accessibility.
| <mat-error *ngIf="!password" role="alert" aria-live="assertive">Password is required.</mat-error> | |
| <mat-error *ngIf="!password" aria-live="polite">Password is required.</mat-error> |
| <mat-error *ngIf="!password" role="alert" aria-live="assertive">Password is required.</mat-error> | ||
| </mat-form-field> | ||
| <p *ngIf="loginErrorMessage !== ''" class="color-warn pre-wrap" fxFlex="100" fxLayoutAlign="start center"> | ||
| <p *ngIf="loginErrorMessage !== ''" class="color-warn pre-wrap" fxFlex="100" fxLayoutAlign="start center" role="alert" aria-live="assertive" aria-atomic="true"> |
There was a problem hiding this comment.
The role="alert" attribute already implies aria-live="assertive" and aria-atomic="true". Removing these redundant attributes simplifies the template code.
| <p *ngIf="loginErrorMessage !== ''" class="color-warn pre-wrap" fxFlex="100" fxLayoutAlign="start center" role="alert" aria-live="assertive" aria-atomic="true"> | |
| <p *ngIf="loginErrorMessage !== ''" class="color-warn pre-wrap" fxFlex="100" fxLayoutAlign="start center" role="alert"> |
| {{loginErrorMessage}} | ||
| </p> | ||
| <p *ngIf="logoutReason !== ''" class="color-warn pre-wrap" fxLayout="row" fxFlex="100" fxLayoutAlign="start center"> | ||
| <p *ngIf="logoutReason !== ''" class="color-warn pre-wrap" fxLayout="row" fxFlex="100" fxLayoutAlign="start center" role="status" aria-live="polite" aria-atomic="true"> |
There was a problem hiding this comment.
The role="status" attribute already implies aria-live="polite" and aria-atomic="true". These explicit attributes can be removed to reduce redundancy.
| <p *ngIf="logoutReason !== ''" class="color-warn pre-wrap" fxLayout="row" fxFlex="100" fxLayoutAlign="start center" role="status" aria-live="polite" aria-atomic="true"> | |
| <p *ngIf="logoutReason !== ''" class="color-warn pre-wrap" fxLayout="row" fxFlex="100" fxLayoutAlign="start center" role="status"> |
| <mat-label>Current Password</mat-label> | ||
| <input autoFocus matInput type="password" id="currpassword" name="currpassword" tabindex="1" required [(ngModel)]="currPassword"> | ||
| <mat-error *ngIf="!currPassword">Current password is required.</mat-error> | ||
| <mat-error *ngIf="!currPassword" role="alert" aria-live="assertive">Current password is required.</mat-error> |
There was a problem hiding this comment.
For form validation, aria-live="polite" is preferred over assertive. Assertive regions interrupt the screen reader's current speech, which can be confusing when the user is moving between fields or typing. mat-error is already handled by Angular Material's accessibility features.
| <mat-error *ngIf="!currPassword" role="alert" aria-live="assertive">Current password is required.</mat-error> | |
| <mat-error *ngIf="!currPassword" aria-live="polite">Current password is required.</mat-error> |
| <mat-label>New Password</mat-label> | ||
| <input matInput type="password" id="newpassword" name="newpassword" tabindex="2" required [(ngModel)]="newPassword"> | ||
| <mat-error *ngIf="matchOldAndNewPasswords()">{{errorMsg}}</mat-error> | ||
| <mat-error *ngIf="matchOldAndNewPasswords()" role="alert" aria-live="assertive">{{errorMsg}}</mat-error> |
There was a problem hiding this comment.
Validation messages that update as the user types (like password matching) should not be assertive, as they will repeatedly interrupt the user. Using polite allows the screen reader to announce the error at the next available opportunity.
| <mat-error *ngIf="matchOldAndNewPasswords()" role="alert" aria-live="assertive">{{errorMsg}}</mat-error> | |
| <mat-error *ngIf="matchOldAndNewPasswords()" aria-live="polite">{{errorMsg}}</mat-error> |
| <mat-label>Confirm New Password</mat-label> | ||
| <input matInput type="password" id="confirmpassword" name="confirmpassword" tabindex="3" required [(ngModel)]="confirmPassword"> | ||
| <mat-error *ngIf="matchNewPasswords()">{{errorConfirmMsg}}</mat-error> | ||
| <mat-error *ngIf="matchNewPasswords()" role="alert" aria-live="assertive">{{errorConfirmMsg}}</mat-error> |
There was a problem hiding this comment.
Consider changing this to aria-live="polite" to follow accessibility best practices for non-critical form validation feedback.
| <mat-error *ngIf="matchNewPasswords()" role="alert" aria-live="assertive">{{errorConfirmMsg}}</mat-error> | |
| <mat-error *ngIf="matchNewPasswords()" aria-live="polite">{{errorConfirmMsg}}</mat-error> |
|
/rtl review |
|
✅ Review posted: #1603 (review) 4 finding(s); 4 inline, 0 in body. 🔁 Need a re-review after pushing changes? Reply with |
![rtlreview[bot]](https://avatars.githubusercontent.com/in/3524153?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds ARIA live-region semantics to authentication-related feedback (loading spinner, form validation errors, login/logout messages) to improve screen reader announcements. The intent is sound and the changes touch the right surfaces. The main issue is that the loading spinner's aria-label duplicates the visible heading inside the same aria-atomic live region, which will cause screen readers to announce "Loading RTL" twice. A secondary concern is the consistent use of aria-live="assertive" for inline form validation, which interrupts ongoing speech and is generally discouraged for non-critical, field-level errors. The redundant explicit attributes on role="status" / role="alert" elements are harmless but worth a quick pass.
| <div *ngIf="!selNode.settings.themeColor" class="rtl-spinner"> | ||
| <mat-spinner color="accent" /> | ||
| <div *ngIf="!selNode.settings.themeColor" class="rtl-spinner" role="status" aria-live="polite" aria-atomic="true"> | ||
| <mat-spinner color="accent" aria-label="Loading RTL" /> |
There was a problem hiding this comment.
🟠 F1 (major): The loading region uses aria-atomic="true" and contains both <mat-spinner aria-label="Loading RTL" /> and <h4>Loading RTL...</h4>. Because aria-atomic="true" causes the entire region's accessible name/text to be announced as a unit, screen readers will read "Loading RTL" from the spinner's aria-label and then "Loading RTL..." from the heading — the same message twice. The spinner is decorative here; mark it aria-hidden="true" (and drop aria-label) so only the <h4> text is announced. Compare: <mat-spinner color="accent" aria-hidden="true" />.
| <mat-label>Password</mat-label> | ||
| <input autoFocus matInput type="password" id="password" name="password" tabindex="1" required [(ngModel)]="password"> | ||
| <mat-error *ngIf="!password">Password is required.</mat-error> | ||
| <mat-error *ngIf="!password" role="alert" aria-live="assertive">Password is required.</mat-error> |
There was a problem hiding this comment.
🟡 F2 (minor): aria-live="assertive" on inline form validation errors interrupts any in-progress screen reader speech, which is jarring while a user is typing or moving between fields. WAI-ARIA authoring guidance reserves assertive for time-critical updates (e.g. session-about-to-expire); routine "field is required" messages should use polite. Angular Material already associates mat-error with the input via aria-describedby, so the message is reachable when focus returns to the field — polite is sufficient. The same concern applies to the three mat-error elements in auth-settings.component.html (lines 10, 15, 20) and the login form's mat-error (login.component.html:22).
| <mat-error *ngIf="!password" role="alert" aria-live="assertive">Password is required.</mat-error> | ||
| </mat-form-field> | ||
| <p *ngIf="loginErrorMessage !== ''" class="color-warn pre-wrap" fxFlex="100" fxLayoutAlign="start center"> | ||
| <p *ngIf="loginErrorMessage !== ''" class="color-warn pre-wrap" fxFlex="100" fxLayoutAlign="start center" role="alert" aria-live="assertive" aria-atomic="true"> |
There was a problem hiding this comment.
🟡 F3 (minor): The login error path is a reasonable place to use assertive announcement (failed login is a high-signal event the user is waiting for), so the role="alert" here is fine. However, role="alert" implicitly carries aria-live="assertive" and aria-atomic="true"; the explicit duplicate attributes are redundant. Same redundancy applies to role="status" on line 28 (which implies aria-live="polite" + aria-atomic="true") and to every mat-error that now carries both role="alert" and aria-live="assertive". Not a defect — some teams prefer explicit-over-implicit — but worth choosing one style consistently across these files.
| <mat-icon>{{flgShow ? 'visibility_off' : 'visibility'}}</mat-icon> | ||
| </button> | ||
| <mat-error *ngIf="!password">Password is required.</mat-error> | ||
| <mat-error *ngIf="!password" role="alert" aria-live="assertive">Password is required.</mat-error> |
There was a problem hiding this comment.
🔵 F4 (nit): On mat-error, the added role="alert" will cause the message to be announced whenever the element is inserted into the DOM. In Angular Material the error appears as soon as the control becomes invalid (typically after first interaction), which on a fresh autofocused empty password field can cause "Password is required" to be announced before the user has done anything meaningful. Consider whether the aria-describedby linkage Material already sets up is sufficient on its own, and reserve the live-region announcement for the post-submit loginErrorMessage path (which this PR already covers separately).
|
🤖 rtlreviewbot audit metadata for this PR — auto-generated, please don't edit. |
|
/rtl re-review Updated the PR to address the requested accessibility feedback:
Local validation: |
|
Sorry, |
|
/rtl re-review |
|
✅ Re-review posted: #1603 (review) Prior findings: 4 addressed, 0 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
The new commit fully addresses all four prior findings. The decorative spinner is now hidden from assistive technology, the redundant explicit live-region attributes have been removed where the role already conveys the semantics, and every mat-error now uses aria-live="polite" rather than assertive. The role="alert" on loginErrorMessage and role="status" on logoutReason are appropriately retained as the events they describe warrant their respective announcement urgencies.
No new concerns are introduced by the updated diff.
Status of prior findings
- F1 addressed: Resolved in
src/app/app.component.html:32-33. The spinner now carriesaria-hidden="true"(and noaria-label), so the wrapper'srole="status"only exposes the<h4>Loading RTL...</h4>text — single announcement, no duplication. - F2 addressed: Every
mat-errorflagged in the prior review now usesaria-live="polite"rather thanassertive:is-authorized.component.html:14,login.component.html:22, andauth-settings.component.html:10,15,20. Validation feedback will queue instead of interrupting in-progress speech. - F3 addressed: The explicit
aria-live/aria-atomicduplicates onrole="alert"androle="status"elements were dropped in this revision.login.component.html:23now carries onlyrole="alert"andlogin.component.html:27onlyrole="status"; themat-errorelements no longer double-up role + explicit live attributes. - F4 addressed: The
mat-erroratlogin.component.html:22no longer carriesrole="alert"; it usesaria-live="polite". The autofocus + insert-on-invalid path will no longer interrupt the user, and the polite live region still complements thearia-describedbylinkage Angular Material wires up automatically.
|
/rtl approve |
There was a problem hiding this comment.
✅ Approved — all prior findings addressed.
Findings recap
Findings:
F1(major) — addressed: The loading region usesaria-atomic="true"and contains both<mat-spinner aria-label="Loading RTL" />and<h4>Loading RTL...</h4>. Bec…F2(minor) — addressed:aria-live="assertive"on inline form validation errors interrupts any in-progress screen reader speech, which is jarring while a user is t…F3(minor) — addressed: The login error path is a reasonable place to use assertive announcement (failed login is a high-signal event the user is waiting for), so t…F4(nit) — addressed: Onmat-error, the addedrole="alert"will cause the message to be announced whenever the element is inserted into the DOM. In Angular Ma…
Approved by @saubyk via /rtl approve. Last reviewed at b024ed5. Skill v0.1.0, model claude-opus-4-7.
2 participants
Summary
Accessibility impact
These changes help screen reader users hear important dynamic status and validation feedback when it appears, instead of needing to manually search the page after a failed login/authentication action.
Fixes #1561.
Checks
npm run lint -- --quietnpx ng build --configuration development --optimization falseNote:
npm cirequired--legacy-peer-depsbecause the current dependency tree has an Angular/zone.js peer dependency conflict.