Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(alert): not announced correctly in screen reader (VIV-1970) #2235

Merged
merged 7 commits into from
Mar 27, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libs/components/src/lib/alert/VARIATIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ Use the `placement` attribute to set the location of the Alert.
Use the `removable` attribute to add a close button to the Alert.

```html preview 100px
<vwc-alert text="Some important information for you" removable open></vwc-alert>
<vwc-alert removable text="Some important information for you" open></vwc-alert>

<vwc-button
appearance="outlined"
Expand Down
30 changes: 13 additions & 17 deletions libs/components/src/lib/alert/alert.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
import type { Icon } from '../icon/icon';
import type { Button } from '../button/button';
import { Connotation } from '../enums';
import { Button } from '../button/button';
import { Alert } from './alert';
import type { AlertConnotation } from './alert';
import '.';
Expand Down Expand Up @@ -55,6 +56,7 @@ describe('vwc-alert', () => {
const alertHeadline = 'headline text';

element.headline = alertHeadline;
element.open = true;
await elementUpdated(element);
const fromProptoDOM = getHeadline();

Expand All @@ -81,6 +83,7 @@ describe('vwc-alert', () => {
const alertText = 'alert text';

element.text = alertText;
element.open = true;
await elementUpdated(element);
const fromProptoDOM = getText();

Expand All @@ -95,13 +98,14 @@ describe('vwc-alert', () => {
});

describe('focus', () => {
it('should focus when opened', async () => {
const spy = vi.fn();
const alertText: HTMLElement = element.shadowRoot?.querySelector(
'.alert-text'
) as HTMLElement;
alertText.focus = spy;
it('should focus on dismiss button when opened', async () => {
element.removable = true;
await elementUpdated(element);
const spy = vi.fn();
const closeBtn = element.shadowRoot?.querySelector(
'.dismiss-button'
) as Button;
closeBtn.addEventListener('focus', spy);

element.open = false;
await elementUpdated(element);
Expand Down Expand Up @@ -394,17 +398,9 @@ describe('vwc-alert', () => {
await elementUpdated(element);
});

it('should set alertdialog on the control when removable', async () => {
element.removable = true;
await elementUpdated(element);

const control = getControlElement(element);
expect(control.getAttribute('role')).toBe('alertdialog');
});

it('should set a role of alert on the control', async () => {
const control = getControlElement(element);
expect(control.getAttribute('role')).toBe('alert');
it('should set a role of alert on the base element', async () => {
const baseEl = getBaseElement(element);
expect(baseEl!.getAttribute('role')).toBe('alert');
});
});

Expand Down
15 changes: 9 additions & 6 deletions libs/components/src/lib/alert/alert.template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ function renderDismissButton(buttonTag: string) {
aria-label="${(x) =>
x.dismissButtonAriaLabel || x.locale.alert.dismissButtonLabel}"
size="condensed"
type="button"
class="dismiss-button"
icon="close-line"
@click="${(x) => (x.open = false)}">
Expand All @@ -44,12 +45,14 @@ 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')}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for alertdialog?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

aria-live="assertive"
>
<div part="vvd-theme-alternate" class="${getClasses}">
<div class="${getControlClasses}">
<div
part="vvd-theme-alternate"
class="${getClasses}"
role="alert"
aria-hidden="${(x) => (x.open ? 'false' : 'true')}"
${(x) => (!x.open ? 'hidden' : '')}
>
${renderIcon(context)}
<div class="alert-text">
${when(
Expand Down
10 changes: 4 additions & 6 deletions libs/components/src/lib/alert/alert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,11 @@ export class Alert extends VividElement {
this.#setupTimeout();
if (newValue) {
this.style.display = 'contents';
const alertText = this.shadowRoot!.querySelector(
'.alert-text'
const closeBtn = this.shadowRoot!.querySelector(
'.dismiss-button'
) as HTMLElement;
if (this.removable && alertText) {
alertText.setAttribute('tabindex', '0');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tabindex is not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

alertText.focus();
alertText.removeAttribute('tabindex');
if (this.removable && closeBtn) {
closeBtn.focus();
}
} else {
this.style.display = 'none';
Expand Down
Loading