fix(alert): not announced correctly in screen reader (VIV-1970) #2235
Conversation
TaylorJ76
changed the title
View Diff |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2235 +/- ##
============================================
Coverage 100.00% 100.00%
============================================
Files 123 375 +252
Lines 1562 16688 +15126
Branches 108 2928 +2820
============================================
+ Hits 1562 16688 +15126
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ${renderIcon(context)} | ||
| <div class="alert-text"> | ||
| ${when( | ||
| (x) => x.headline, | ||
| html`<header class="headline">${(x) => x.headline}</header>` | ||
| html`<div class="headline">${(x) => x.headline}</div>` |
There was a problem hiding this comment.
isn't it an issue for readability of the alert?
|
|
||
| // export const AlertTemplate = (context: VividElementDefinitionContext) => { | ||
| // return html<Alert>` | ||
| // <div role="alert"> | ||
| // ${(x) => x.open ? x.text : null} | ||
| // </div> | ||
| // `; | ||
| // }; |
There was a problem hiding this comment.
| // export const AlertTemplate = (context: VividElementDefinitionContext) => { | |
| // return html<Alert>` | |
| // <div role="alert"> | |
| // ${(x) => x.open ? x.text : null} | |
| // </div> | |
| // `; | |
| // }; |
| ) as HTMLElement; | ||
| if (this.removable && alertText) { | ||
| alertText.setAttribute('tabindex', '0'); |
There was a problem hiding this comment.
the tabindex is not needed anymore?
There was a problem hiding this comment.
No, because focus is set to the close button.
Plus, setting focus to the alert-text element didn't make the screen-reader announce the text.
| @@ -44,17 +45,19 @@ export const AlertTemplate = (context: VividElementDefinitionContext) => { | |||
|
|
|||
| return html<Alert>` | |||
| <${elevationTag} class="elevation" dp='8' exportparts="vvd-theme-alternate"> | |||
| <div | |||
| class="${getControlClasses}" | |||
| role="${(x) => (x.removable ? 'alertdialog' : 'alert')}" | |||
There was a problem hiding this comment.
No. It's not an alertdialog (like a confirmation dialog), it is a closable alert.
I checked how other design system's tackled it and this seemed to be the most common.
Also, it works as expected.
When the dismiss button is present, it receives focus when the alert is opened.
This is beneficial to keyboard only and screen reader users, because they don't have to tab around to find the button.