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.

Sign up for GitHub

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: aria role behavior #2171

Merged
merged 9 commits into from
Nov 30, 2023
Merged

fix: aria role behavior #2171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
49c7cb2
fix: aria role behavior
Oct 20, 2023
a848629
style: format
Oct 20, 2023
ee7bf06
fix: use slot for heading, implement a11y workaround for role=status
Oct 23, 2023
0faf96b
test(visual): update snapshots (#2172)
github-actions[bot] Oct 30, 2023
e81901b
style: format
Oct 30, 2023
c57080d
fix: a11y remove unnecessary custom workaround
Nov 3, 2023
7ee93be
fix: implement change requests
Nov 7, 2023
93d5169
style: format
Nov 22, 2023
6c42d60
docs: document prop change
Nov 24, 2023
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
4 changes: 3 additions & 1 deletion packages/components/src/components/notification/notification.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,11 @@
padding-left: var(--spacing-y);
}

[part='heading'] {
[part='heading'],
[part='heading'] ::slotted(*) {
font: var(--telekom-text-style-heading-6);
line-height: var(--telekom-typography-line-spacing-tight);
margin: 0;
}

[part='text'] {
Expand Down
22 changes: 11 additions & 11 deletions packages/components/src/components/notification/notification.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,10 @@ export class Notification {
@Prop() dismissible?: boolean = false;
/** (optional) Time in milliseconds until it closes by itself */
@Prop() delay?: number;
/** (optional) `aria-live` of element */
/** @deprecated - ariaRole should replace innerAriaLive */
@Prop() innerAriaLive?: string = 'assertive';
/** (optional) string prepended to the heading */
@Prop() innerRole?: 'alert' | 'status' = 'alert';
/** (optional) Label for close button */
@Prop() closeButtonLabel?: string = 'Close';
/** (optional) `title` for close button */
Expand All @@ -69,13 +71,13 @@ export class Notification {
@Prop() headingLevel: number = 2;
/** (optional) string prepended to the heading */
@Prop() ariaHeading?: string = 'Information';

/** (optional) Injected styles */
@Prop() styles?: string;

/** What actually triggers opening/closing the notification */
@State() isOpen: boolean = this.opened || false;
@State() animationState: 'in' | 'out' | undefined;
@State() role: string = 'alert';
@State() hasTextSlot: boolean = false;
// @State() hasActionSlot: boolean = false; // unused for now

Expand All @@ -91,8 +93,9 @@ export class Notification {

connectedCallback() {
if (this.hostElement.hasAttribute('opened')) {
// Do not use `role="alert"` if opened/visible on page load
this.role = undefined;
Copy link

Choose a reason for hiding this comment

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

Is it still guaranteed that notifications which are not initially opened are not announced by screen readers immediately? Since the default value of this.innerRole is 'alert' even closed notifications would initially be rendered with role="alert" if not specifying innerRole and innerAriaLive. So, if the element with that role is part of the accessibility tree it would be announced right away. Could be the element is hidden via CSS in a way that detaches it from accessibility tree. Otherwise, we might need a workaround for that. I don't know if setting aria-hidden="true" if this.isOpened === false could help.

Copy link

Choose a reason for hiding this comment

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

Sorry if that comment is confusing. I mixed two things up:

  • Announcement on initial opened render (which was supposed to be prevented by the deleted code)
  • Announcement in closed state

Announcement while being closed should not be an issue because this seemed to work before. If attribute opened is not set the inner role was 'alert' before and that seemed to work fine.

The change with this code deletion is that notifications which are initially opened will be announced because they have this.innerRole === 'alert' by default. If preventing initial announcement of visible notifications is desired by some projects they could set innerRole to something other than 'alert' or 'status' to prevent the ARIA live region. Maybe set innerRole="note".

Copy link
Collaborator Author

@felix-ico felix-ico Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is it still guaranteed that notifications which are not initially opened are not announced by screen readers immediately?

I think so, since they have display: none in css, so it should not be read out at all, even if it has a status of alert.

The change with this code deletion is that notifications which are initially opened will be announced because they have this.innerRole === 'alert' by default.

Yes that is one effect, and the difference between alert and status is that alert will interrupt anything else that is being read by the screenreader, while status should wait until the screen reader is idle.

The current implementation seems to work when inner-role="status" and opened=true (the element has display:flex, role=status, headline is read out).

THIS WORKS:

<scale-notification
    type="success"
    opened
    inner-role="status"
  >
  <h2 slot="heading"> TEST</h2>
  <span slot="text"> HELLO</span>
  </scale-notification>

However when dynamically opening a notification with inner-role="status", it will not be read out - as far as i understand it's because when scale-notification goes from display:none to display:flex it already has its text content (heading and text slots).

DOESN'T GET ANNOUNCED:

<scale-notification
  type="success"
  inner-role="status"
>
<h2 slot="heading"> TEST</h2>
<span slot="text"> HELLO</span>
</scale-notification>
<script>
  const notification = document.querySelector('scale-notification');
  setTimeout(() => {
    notification.opened = true;
  }, 3000);
</script>

I found that one way around it, though not ideal, is to first set opened=true and then set/change the text contents, that will trigger the screen reader (role="status" only reads when some content changes are detected).

HACKY WORKAROUND:

  <body>
    <h1 class="scl-font-variant-body">PAGE TITLE</h1>
    <scale-notification
      type="success"
      inner-role="status"
    >
    </scale-notification>
  </body>
  <script>
    const notification = document.querySelector('scale-notification');
    setTimeout(() => {
      notification.opened = true;
      setTimeout(() => {
        notification.innerHTML = `
          <h2 slot="heading"> TEST</h2>
          <span slot="text"> HELLO</span>

      }, 10);
    }, 8000);
  </script>

this is what i tried to do previously, in the component itself, but without any good result

Copy link

Choose a reason for hiding this comment

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

Yes, we use such "delayed text insertion" in our project in another ARIA live region with aria-live="polite" to have NVDA read that text right after component render. Very unintuitive, but well... alright.

if (this.innerAriaLive === 'polite' || this.innerRole === 'status') {
this.innerRole = 'status';
}
this.isOpen = true;
}
if (this.delay !== undefined) {
Expand All @@ -115,7 +118,6 @@ export class Notification {

open = () => {
this.isOpen = true;
this.role = 'alert';
this.animationState = 'in';
requestAnimationFrame(async () => {
await animationsFinished(this.hostElement.shadowRoot);
Expand Down Expand Up @@ -164,24 +166,22 @@ export class Notification {
`variant-${this.variant}`,
this.isOpen && 'open'
)}
role={this.role}
role={this.innerRole}
>
<div part="icon" aria-hidden="true">
<slot name="icon">
<IconTag size={ICON_SIZE} selected={this.type === 'toast'} />
</slot>
</div>
<div
part="body"
aria-live={this.role === undefined ? undefined : this.innerAriaLive}
>
<div part="body">
<div
part="heading"
role="heading"
aria-level={this.headingLevel}
aria-label={`${this.ariaHeading} ${this.heading}`}
>
<span>{this.heading}</span>
{this.heading ? <span>{this.heading}</span> : null}
<slot name="heading"></slot>
</div>
{this.hasTextSlot && (
<div part="text">
Expand Down
29 changes: 15 additions & 14 deletions packages/components/src/components/notification/readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,21 @@

## Properties

| Property | Attribute | Description | Type | Default |
| ------------------ | -------------------- | --------------------------------------------------------- | ------------------------------------------------------- | ----------------- |
| `ariaHeading` | `aria-heading` | (optional) string prepended to the heading | `string` | `'Information'` |
| `closeButtonLabel` | `close-button-label` | (optional) Label for close button | `string` | `'Close'` |
| `closeButtonTitle` | `close-button-title` | (optional) `title` for close button | `string` | `'Close'` |
| `delay` | `delay` | (optional) Time in milliseconds until it closes by itself | `number` | `undefined` |
| `dismissible` | `dismissible` | (optional) Show the close button | `boolean` | `false` |
| `heading` | `heading` | Heading | `string` | `undefined` |
| `headingLevel` | `heading-level` | Default aria-level for heading | `number` | `2` |
| `innerAriaLive` | `inner-aria-live` | (optional) `aria-live` of element | `string` | `'assertive'` |
| `opened` | `opened` | (optional) Visible | `boolean` | `undefined` |
| `styles` | `styles` | (optional) Injected styles | `string` | `undefined` |
| `type` | `type` | (optional) Type | `"banner" \| "inline" \| "toast"` | `'inline'` |
| `variant` | `variant` | (optional) Variant | `"danger" \| "informational" \| "success" \| "warning"` | `'informational'` |
| Property | Attribute | Description | Type | Default |
| ------------------ | -------------------- | ------------------------------------------------------------------------------------------------- | ------------------------------------------------------- | ----------------- |
| `ariaHeading` | `aria-heading` | (optional) string prepended to the heading | `string` | `'Information'` |
| `closeButtonLabel` | `close-button-label` | (optional) Label for close button | `string` | `'Close'` |
| `closeButtonTitle` | `close-button-title` | (optional) `title` for close button | `string` | `'Close'` |
| `delay` | `delay` | (optional) Time in milliseconds until it closes by itself | `number` | `undefined` |
| `dismissible` | `dismissible` | (optional) Show the close button | `boolean` | `false` |
| `heading` | `heading` | Heading | `string` | `undefined` |
| `headingLevel` | `heading-level` | Default aria-level for heading | `number` | `2` |
| `innerAriaLive` | `inner-aria-live` | <span style="color:red">**[DEPRECATED]**</span> - ariaRole should replace innerAriaLive<br/><br/> | `string` | `'assertive'` |
| `innerRole` | `inner-role` | (optional) string prepended to the heading | `"alert" \| "status"` | `'alert'` |
| `opened` | `opened` | (optional) Visible | `boolean` | `undefined` |
| `styles` | `styles` | (optional) Injected styles | `string` | `undefined` |
| `type` | `type` | (optional) Type | `"banner" \| "inline" \| "toast"` | `'inline'` |
| `variant` | `variant` | (optional) Variant | `"danger" \| "informational" \| "success" \| "warning"` | `'informational'` |


## Events
Expand Down
3 changes: 2 additions & 1 deletion packages/storybook-vue/stories/components/notification/Notification.stories.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ import ScaleNotification from './ScaleNotification.vue';
innerAriaLive: {
control: {
type: 'text'
}
},
description: 'Deprecated! innerAriaLive has been replaced by innerRole'
},
headingLevel: {
control: {
Expand Down
1 change: 1 addition & 0 deletions packages/storybook-vue/stories/components/notification/ScaleNotification.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export default {
dismissible: { type: Boolean },
delay: { type: Number },
innerAriaLive: { type: String },
innerRole: { type: String, default: 'alert' },
closeButtonLabel: { type: String },
closeButtonTitle: { type: String },
styles: { type: String },
Expand Down
Loading