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

Merged
merged 9 commits into from
Nov 30, 2023
Merged

fix: aria role behavior #2171

merged 9 commits into from
Nov 30, 2023

Conversation

felix-ico
Copy link
Collaborator

No description provided.

@netlify
Copy link

netlify bot commented Oct 20, 2023

Deploy Preview for marvelous-moxie-a6e2fe ready!

Name Link
🔨 Latest commit 6c42d60
🔍 Latest deploy log https://app.netlify.com/sites/marvelous-moxie-a6e2fe/deploys/6560bb9f729e6400088f4518
😎 Deploy Preview https://deploy-preview-2171--marvelous-moxie-a6e2fe.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

Also updating Storybook is missing (adding the new innerRole and deprecating innerAriaLive) 🤓

@@ -91,8 +93,13 @@ export class Notification {

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

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.

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

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

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.

Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

This should be documented in Storybook, other than that it looks good. I hope it works 🤞

@felix-ico felix-ico merged commit 59c57bc into main Nov 30, 2023
11 checks passed
@felix-ico felix-ico deleted the fix/notification-a11y-aria-role branch November 30, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants