WB-1636: Add describedBy prop to announce the popover dialog contents to screen readers.
#2110
Conversation
🦋 Changeset detectedLatest commit: 9306f8a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +84 B (0%) Total Size: 92.8 kB
ℹ️ View Unchanged
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2110 +/- ##
=======================================
Coverage 97.07% 97.07%
=======================================
Files 243 243
Lines 28245 28286 +41
Branches 2466 2420 -46
=======================================
+ Hits 27418 27459 +41
Misses 827 827
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-zocviyfbxy.chromatic.com/ Chromatic results:
|
| @@ -233,12 +253,12 @@ export default class Popover extends React.Component<Props, State> { | |||
| {(props: PopperElementProps) => ( | |||
| <PopoverDialog | |||
| {...props} | |||
| aria-describedby={`${uniqueId}-anchor`} | |||
| aria-describedby={`${uniqueId}-${describedBy}`} | |||
There was a problem hiding this comment.
So it looks like this existing aria-describedby prop makes it so the popover is described by the content that it is pointing at. I think it would be best not to totally remove that, as we want some way to tie the popover to the content it points at. Adobe's react-aria-components library uses aria-labelledby for this, so maybe a solution would be:
| aria-describedby={`${uniqueId}-${describedBy}`} | |
| aria-labelledby={`${uniqueId}-anchor`} | |
| aria-describedby={`${uniqueId}-${describedBy}`} |
There was a problem hiding this comment.
In this case pointing labelledby to the anchor element would not work as we expect. If we make that, then the dialog would be announced with the contents of the button/anchor element and the intention would be to use that to announce the dialog title instead.
aria-labelledby
Use this attribute to label the dialog. Often, the value of the aria-labelledby attribute will be the id of the element used to title the dialog.
That makes me think that we can even use that instead of the describedBy solution. Let me try that alternative.
| * when it is opened. | ||
| * Defaults to "title". | ||
| */ | ||
| describedBy: "title" | "content" | "all-content"; |
There was a problem hiding this comment.
| describedBy: "title" | "content" | "all-content"; | |
| describedBy: "title" | "content" | "title-and-content"; |
I think this is slightly clearer
|
Closed as we are going with a different approach added in #2134 |
Summary:
The Popover dialog element is not assigning correctly the
aria-describedbyvalue, thus causing Screen Readers not being able to read the popover contents
correctly.
The current implementation is trying to reference
aria-describedbyto theanchor/trigger element (or button that triggers the popover to open/close). This
is incorrect, as the
aria-describedbyvalue should reference to a particularcontent inside the dialog itself.
This PR adds a new prop to the Popover component,
describedBy, which willallow the user to pass a string referencing the element that contains the
dialog's contents. This will allow Screen Readers to read the popover contents
correctly.
Issue: https://khanacademy.atlassian.net/browse/WB-1636
Test plan:
describedByprop to reference thecontentelement by changing thedescribedByprop tocontentcontrol in the Storybook.