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

WIP: [Accessibility] Make toast global list is an aria live region #1958

Closed
wants to merge 21 commits into from

Conversation

PhilippBaranovskiy
Copy link
Contributor

@PhilippBaranovskiy PhilippBaranovskiy commented May 23, 2019

Closes #1947

Summary

This PR aims to implement correctly live regions as a toast global list in Eui and Kibana app.
Toasts are stored as <p> nodes in an aria live region.
Every next toast replace a previous one with short "clearing-delay" which tells ScreenReader that there is a change that needs to be announced.

⚠️ Before

Every toast has aria-live attribute, there is no live region at all.

It does work: VoiceOver
It doesn't work: NVDA, JAWS.

✅ Now

A live region role="region" initializes at the component mounting,
next to globalToastList, — it is a div that is going to contains all upcoming notifications as a <p> nodes.
Thus the Screen Reader understands where it should wait for updates.

Have a look at #1947 issue's description, there are a couple of links to standard explanations.

It does work: VoiceOver, NVDA, JAWS
It doesn't work: n/a

Expected behaviour

Once a toast notification is rendered, it is getting be announced.
At the moment of the next toast coming, Screen Reader stops saying and is starting announce the coming one.

Tasks to do in this PR

  • Implemented a persistent stack of toasts in a very thin stick low memory usage way.
  • Tested with NVDA / JAWS / Narrator (Win)
  • Tested with VoiceOver (Mariya, Bhavya)
  • Tested memory usage while

Checking against ScreenReaders

✅ VoiceOver + Safari
✅ NVDA + Firefox
✅ NVDA + IE11
✅ Narrator + Edge
✅ NVDA + Chrome

Checklist

  • This was checked in dark mode
    - [ ] This was checked in mobile
    - [ ] Any props added have proper autodocs
  • This was checked in IE11
    - [ ] Documentation examples were added
  • A changelog entry exists and is marked appropriately
    - [ ] This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
    - [ ] This required updates to Framer X components

@PhilippBaranovskiy PhilippBaranovskiy self-assigned this May 23, 2019
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Small change. You need a changelog.

src/components/toast/global_toast_list.js Outdated Show resolved Hide resolved
@PhilippBaranovskiy PhilippBaranovskiy added accessibility accessibility - WCAG A Level A WCAG Accessibility Criteria labels May 28, 2019
@PhilippBaranovskiy PhilippBaranovskiy marked this pull request as ready for review May 28, 2019 11:51
@PhilippBaranovskiy PhilippBaranovskiy changed the title WIP [Accessibility] Make toast global list is an aria live region [Accessibility] Make toast global list is an aria live region May 28, 2019
@PhilippBaranovskiy PhilippBaranovskiy requested a review from snide May 30, 2019 08:11
Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

Looks good but please review my comments above.

@bhavyarm
Copy link
Contributor

bhavyarm commented Jun 3, 2019

@rockfield I tried to test this on Friday and you are correct. I am not sure whats happening with this on voiceover. Buzzing a developer today and then will update my review. Thanks!

@PhilippBaranovskiy
Copy link
Contributor Author

@bhavyarm, I've updated the algorithm with the following technique: after the 57 sec of idle, it clears the stack of toast notifications.
The assumption is that the live region overflow (that screen reader watches) may somehow impact the work of VoiceOver.
So, I'm testing this today and leaving feedback here.

@PhilippBaranovskiy PhilippBaranovskiy changed the title [Accessibility] Make toast global list is an aria live region WIP: [Accessibility] Make toast global list is an aria live region Jun 3, 2019
@PhilippBaranovskiy PhilippBaranovskiy changed the title WIP: [Accessibility] Make toast global list is an aria live region [Accessibility] Make toast global list is an aria live region Jun 3, 2019
@PhilippBaranovskiy PhilippBaranovskiy changed the title [Accessibility] Make toast global list is an aria live region WIP: [Accessibility] Make toast global list is an aria live region Jun 4, 2019
@PhilippBaranovskiy PhilippBaranovskiy changed the title WIP: [Accessibility] Make toast global list is an aria live region [Accessibility] Make toast global list is an aria live region Jun 4, 2019
@PhilippBaranovskiy
Copy link
Contributor Author

@maryia-lapata could you confirm that we checked with VoiceOver and it works perfectly?

@PhilippBaranovskiy
Copy link
Contributor Author

@bhavyarm I've re-implemented the algorithm of updating sound announcing, now as I see it works perfectly with VoiceOver as well as with NVDA and Jaws.
Could you have a look?

@maryia-lapata
Copy link
Contributor

@maryia-lapata could you confirm that we checked with VoiceOver and it works perfectly?

yes, we checked with VoiceOver (Mac 10.14.5). It works as expected (see "Expected behaviour" section on the description.)

// where one of them has number ID while the other -- string
// Also, documentation example uses numberic IDs
const toastIDS = toasts
.filter(({ id }) => typeof id === 'number')
Copy link
Contributor

Choose a reason for hiding this comment

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

@cjcenizal @snide @thompsongl @cchaos @rockfield
I think it makes sense to discuss this solution. This solution works only for numeric id's. I see some risk here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjcenizal @snide @thompsongl @cchaos
The main question and issue here are how could I catch the last came toast?

Copy link
Contributor

@cjcenizal cjcenizal Jun 5, 2019

Choose a reason for hiding this comment

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

Just so I'm sure I understand correctly: is the goal of lines 194-198 to determine the newest toast in the toasts array? If so, then does toasts[toasts.length - 1] give us that information?

I agree with @alexwizp that depending upon numeric IDs is risky. AFAIK, there's nothing enforcing numeric IDs, or even a meaningful relationship between IDs that are numeric (e.g. that higher numbers mean the toast is newer or anything else). I think relying on a convention will be very easy to break and difficult for consumers to debug if they stray from it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this over a zoom call and have a plan to move forward with a more nuanced approach.

@PhilippBaranovskiy PhilippBaranovskiy changed the title [Accessibility] Make toast global list is an aria live region WIP: [Accessibility] Make toast global list is an aria live region Jun 6, 2019
id: toast.id,
reactElement: (
<Fragment key={toast.id}>
<p>
Copy link

Choose a reason for hiding this comment

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

❔ For accessibility, would it make sense to give this an aria-title pointing to the thing below that has the toast.title ditto for aria-describedby and the toast text?

@PhilippBaranovskiy
Copy link
Contributor Author

This became too huge, complex and outdated.
I'm closing this in order to provide another tiny small update instead trying to cover all functional needs.

Please, have a look at that a few lines PR: #2055

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility - WCAG A Level A WCAG Accessibility Criteria accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Accessibility] NVDA does announce toasts, => toast global list is not an aria live region
8 participants