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

Notification: Property innerAriaLive has no effect #2091

Open
georgwittberger-telekom-mms opened this issue Aug 3, 2023 · 16 comments
Open
Assignees
Labels
bug Something isn't working question Further information is requested

Comments

@georgwittberger-telekom-mms

Scale Version

3.0.0-beta.137

Framework and version

React 18.2.0 with @telekom/scale-components-react

Current Behavior

Notification is not announced by screen readers when it appears in the document even though attribute inner-aria-live is set on custom element. The attribute value does not seem to be rendered anywhere inside the shadow DOM.

Expected Behavior

Notification should be announced by screen readers when its attribute inner-aria-live is set to either polite or assertive. The inner element <div part="body"> should have the attribute aria-live with the given value.

Code Reproduction

<scale-notification
  heading="This should be announced by screen reader"
  inner-aria-live="polite"
  opened
></scale-notification>

Try it by setting prop innerAriaLive on this demo page: https://telekom.github.io/scale/?path=/docs/components-notification--standard

@georgwittberger-telekom-mms georgwittberger-telekom-mms added the bug Something isn't working label Aug 3, 2023
@felix-ico felix-ico self-assigned this Sep 5, 2023
@felix-ico
Copy link
Collaborator

this bugfix should be available as of beta.139

@georgwittberger-telekom-mms
Copy link
Author

Issue is not fixed with 3.0.0-beta.139. The attribute aria-live is not set, assuming because the internal this.role is always undefined.

@felix-ico felix-ico reopened this Sep 11, 2023
@felix-ico
Copy link
Collaborator

hi @georgwittberger-telekom-mms I just tested this again on a little custom site. I used the voiceover utility on macOS and the narrator on win11 - and the notification content was output correctly. I also checked on the deployed storybook page and got the correct screenreader output.

@georgwittberger-telekom-mms
Copy link
Author

Hi @felix-ico , thanks for investigating and testing. The expected behavior is that screen readers announce the notification text content as soon as the component appears in the DOM. It's not about having the content read when focusing the element.

This announcement on appearance did not work for me because the body part of the notification does not have the attribute aria-live, even if a set inner-aria-live on the component. This is reproducible in Storybook:

QG3k8YbOKS

@georgwittberger-telekom-mms
Copy link
Author

Hi @felix-ico , I have news on this one. The problem of notifications not being announced when they appear/change in the document was raised again in our recent accessibility audit. I have looked a little bit into it.

It seems like the inner attribute aria-live on the body part is only missing if the notification is initially rendered with opened="true". As soon as I set opened="false" on the first render and then change it to opened="true" the attribute aria-live is set as expected on the body part. Screen readers then announce the message correctly.

I worked around this behavior in our React project as follows:

const MessageBox = ({
  id,
  heading,
  variant,
  dismissible,
  children,
  onClose,
  ...props
}) => {
  const [opened, setOpened] = useState(false);

  const onMessageClose = (event) => {
    event.stopPropagation();
    setOpened(false);
    onClose?.(event);
  };

  useEffect(() => {
    // Workaround: Open notification after initial render to make aria-live work!
    setOpened(true);
  }, []);

  return (
    <ScaleNotification
      id={id}
      heading={heading}
      innerAriaLive={variant === "danger" ? "assertive" : "polite"}
      variant={variant}
      opened={opened}
      {...(dismissible && {
        dismissible: true,
        closeButtonLabel: "Close dialog",
        closeButtonTitle: "Close dialog"
      })}
      // Workaround: Prevent events from triggering behavior in parent components (e.g. scale-modal)!
      onScale-open={event => event.stopPropagation()}
      onScale-close={onMessageClose}
      {...props}>
      {children && <div slot="text">{children}</div>}
    </ScaleNotification>
  );
};

So, you may want to take a look why setting aria-live internally does not work if the notification is mounted with opened="true".

@felix-ico
Copy link
Collaborator

felix-ico commented Oct 11, 2023

hi @georgwittberger-telekom-mms thanks a lot for the update and the tip. That's interesting because I remember testing on the storybook canvas (that has opened set to true initially) and the screenreader outputting the text correctly. I will try it out again as you described, in a react app.

edit: read your last post again and realized that this should have nothing to do with the react context - testing now

@felix-ico
Copy link
Collaborator

Hi @georgwittberger-telekom-mms I just tested this again, and could nor reproduce your described bug. I made sure to have a notification with the opened attribute already set to true on the initial page load.

I did find some inconsistent behavior though:

Tested on Win11

Outside of react context (vanilla HTML):
Edge + NVDA: Notification headline and text gets read on first page load, on a page refresh contents do not get read ❓
Chrome + NVDA: Same as above ❓

Edge + JAWS: Notification headline and text gets read on first page load, and on subsequent page reloads ✅
Chrome + JAWS: Same as above ✅

In react context:
Edge + NVDA: Notification headline and text gets read on first page load, and on subsequent page reloads ✅
Chrome + NVDA: Same as above ✅

Edge + JAWS: Notification headline and text gets read on first page load, and on subsequent page reloads ✅
Chrome + JAWS: Same as above ✅

I also tested the storybook sample:

https://telekom.github.io/scale/iframe.html?id=components-notification--standard&args=&globals=locale:en;colorMode:dark&viewMode=story

on chrome & edge + NVDA and JAWS and found that all contents are output correctly (and there is no special logic there to set opened="true" after page load there) ✅ - could you test that link on your side?

I think that the innerAriaLive property should be removed in any case. As far as I understand elements with role="alert" have an implicit "assertive" aria-live value, so setting it to polite should not have any effect anyway. @acstll do you have any thoughts on this?

@felix-ico felix-ico added the question Further information is requested label Oct 18, 2023
@georgwittberger-telekom-mms
Copy link
Author

Hi @felix-ico , thanks for this extensive testing.

When I open that link the notification component does not render any role attribute or aria-live attributes. But the test page seems to be set up without using inner-aria-live on the component.

Generelly, it would be nice to have external control whether the notification needs to be announced as an alert or it is just a status. Maybe you can provide a prop like announce which can be set to either alert for assertive announcement or status for polite announcement.

@felix-ico
Copy link
Collaborator

any role attribute or aria-live attributes

That should be due to this code, that is executed on initial render:

    if (this.hostElement.hasAttribute('opened')) {
      // Do not use `role="alert"` if opened/visible on page load
      this.role = undefined;
      ...
    }
    
    ...
    
    aria-live={this.role === undefined ? undefined : this.innerAriaLive}

when the notification is initially set to open, it will not receive any role attribute, therefore not receiving and aria-live value, and screen readers will just follow the normal flow of the document.

I guess the issue is that the innerAriaLive value is never set on initial render (when opened=true) because the role is also undefined. Perhaps just changing the initial render role to status, as you suggested, is enough? I'll give this a shot locally

@georgwittberger-telekom-mms
Copy link
Author

georgwittberger-telekom-mms commented Oct 19, 2023

OK, I see why this could make sense. There might be cases where notifications should not be announced when they are rendered on initial page load, but only once the are displayed by setting opened=true.

I suppose this is the expected usage of the component: It is intended to be rendered always even if the message should not be visible yet. So it should be rendered with opened=false. Then, as soon as some condition is met, the opened prop is set to true to show the message. Then it needs to become an ARIA live region to be announced.

However, there are cases where we cannot always render the notification component. For example, if you have a tab panel which renders its content only when the tab is selected. In this case, a notification could be inserted into the DOM with opened initially set to true. It's debatable whether screen readers should announce the message in such a scenario when you select the tab. But assuming this is desired, it would be nice to have that option.

Propsal

Add new prop ariaRole: "alert" | "status" | undefined to allow external control over the role attribute.

Behavior without ariaRole

<scale-notification heading="abc"></scale-notification>

Current behavior for backwards compatibility. Notification is initially rendered without role attribute and without aria-live. Only when setting opened=true after initial render the role=alert attribute is added.

Behavior with ariaRole=alert

<scale-notification heading="abc" aria-role="alert"></scale-notification>

Notification is initially rendered with role=alert attribute and with aria-live=assertive if opened=true on initial render. Then once opened is set false (e.g. dismissible notification) it may remove role and aria-live until it is opened again.

Behavior with ariaRole=status

<scale-notification heading="abc" aria-role="status"></scale-notification>

Notification is initially rendered with role=status attribute and with aria-live=polite if opened=true on initial render. Then once opened is set false (e.g. dismissible notification) it may remove role and aria-live until it is opened again.

This would help in our use case where we render notifications only when needed, thus initially having opened=true and wanting them to be announced when we mount them. And it would give developers more control how important the message is (alert vs status).

@felix-ico
Copy link
Collaborator

felix-ico commented Oct 23, 2023

Hi @georgwittberger-telekom-mms, thanks a lot for the detailed suggestion. I've been trying to implement similar to how you described, but came across some issues with aria-live="polite"/role="status" - the notification text doesn't get read at all. After some reading i found that apparently an element with role="status"/aria-live="polite" will be announced only when the inner html changes, not when the role is set (different to role="alert", which gets announced when the role is set) - this is an issue here because the notification is already present in the dom and the text doesn't change.

My PR for reference #2171

possibly related issue nvaccess/nvda#14591
this stackoverflow answer describes how role="status" works https://stackoverflow.com/questions/75463994/why-the-role-status-attribute-doesnt-work-correctly

I am still trying to find a solution for this, will also ask colleagues internally for help.

@felix-ico
Copy link
Collaborator

felix-ico commented Oct 23, 2023

quick update: I may have found a workaround for that issue, by setting a small delay and "updating" (with the original text) the innerHTML, so that the screen readers announce the contents of role="status"- this seems to work in all scenarios in my tests.

I had to make one change, that seemed to make sense in general to me: "heading" is not a prop anymore but another slot (this was because otherwise the text slot would be read before the heading, due to light-dom having precedence over shadow dom)

I'm hoping to make a release for this tomorrow, so that you may test it too.

@georgwittberger-telekom-mms
Copy link
Author

Thank you so much, @felix-ico . I appreciate your hard work on this niche issue. 👍

Sad to hear that role="status" is causing so much inconvenience for developers. But good to know that your solution (delayed content change) works.

Regarding the heading change for the Scale Notification component: Will it continue to support heading prop for backwards compatibility? This could help developers when updating the Scale version in their projects. So, if heading prop is present it should render that heading text instead of the heading slot (old behavior). Otherwise, it would render the heading slot.

@felix-ico
Copy link
Collaborator

I'm about to push some more changes to the bugfix-branch and will keep your comment in mind

@felix-ico
Copy link
Collaborator

so actually i removed the whole "workaround" as it wasn't doing anything #2171

I realized that I have been testing wrong due to me misunderstanding aria-live property

Screenshot 2023-11-03 at 15 51 30

from my test "when the user is idle" means when the screen reader has stopped reading, and an element with role=status appears, it will be announced.

However, if the screen reader is announcing some content, and the role=status element appears in the meantime, it will NOT be announced.

I tested this by setting different timeouts and got consistent behavior, even without the custom element but just with some simple divs, so I am not sure at the moment if the spec is broken or the screen readers/browser have implemented it wrong.

In any case, from my tests the component should now be announced when it is mounted to the page and is already opened (the original issue). There may be inconsistent behavior when the opened prop is updated dynamically, in combination with role="status" (as mentioned above, it will be read if the screen reader is idle, otherwise not).

@felix-ico
Copy link
Collaborator

about backwards compatibility, the heading prop should also still work, there may be issues with how screen readers deal with the order of those elements so I'd recommend using the slot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants