Fixes accessibility issue created by incorrect dialog role #285

mellis481 · 2022-04-11T13:18:40Z

Issue

Dialog's boundaries are not communicated because of improper structure.

User Impact

When this is not done, it is unclear to screen readers when they are leaving or entering a dialog when using the virtual cursor.

Compliant Code Example

https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html

Additional Details

Role=document does not need to be used. role=dialog should be moved to where your role=document was.

vercel · 2022-04-11T13:18:44Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/react-component/dialog/G27qhEgFJsdzcc7jVeNSexDsvk9d
✅ Preview: https://dialog-git-fork-mellis481-fixes-roles-react-component.vercel.app

dgreene1 · 2022-04-11T16:29:09Z

src/Dialog/Content/index.tsx


 export type ContentRef = {
  focus: () => void;
  changeActive: (next: boolean) => void;
-}
+};


Thanks prettier

dgreene1 · 2022-04-11T16:30:51Z

@zombieJ we had an external accessibility audit conducted that discovered this finding.

zombieJ · 2022-04-12T02:30:26Z

Hi, CI breaks. Pls update test snapshot

codecov · 2022-04-12T14:06:45Z

Codecov Report

Merging #285 (6045afa) into master (b37ffd0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #285   +/-   ##
=======================================
  Coverage   98.05%   98.05%           
=======================================
  Files           6        6           
  Lines         154      154           
  Branches       46       46           
=======================================
  Hits          151      151           
  Misses          2        2           
  Partials        1        1

Impacted Files	Coverage Δ
src/Dialog/Content/index.tsx	`95.23% <ø> (ø)`
src/Dialog/index.tsx	`100.00% <ø> (ø)`

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

mellis481 · 2022-04-15T12:49:12Z

@zombieJ Any update on this?

vercel · 2022-04-18T12:29:25Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
dialog	✅ Ready (Inspect)	Visit Preview	Apr 18, 2022 at 0:32AM (UTC)

hedaukartik · 2022-05-05T09:56:56Z

src/Dialog/index.tsx

@@ -182,7 +182,6 @@ export default function Dialog(props: IDialogChildProps) {
        className={classNames(`${prefixCls}-wrap`, wrapClassName)}
        ref={wrapperRef}
        onClick={onWrapperClick}
-        role="dialog"


why did we remove role here but kept aria-labelledby?
This is a breaking change who are querying getByRole('dialog', { name: /title/i }) in react-testing-library

We have to workaround in every test to query as getByLabelText(/title/i) which is not the right way to query dialog. refer- https://testing-library.com/docs/queries/about#priority

The role property was moved because it was on the wrong html element and therefore was causing screen reader users to be unable to properly use it. This PR fixed that.

hedaukartik · 2022-05-05T09:58:46Z

src/Dialog/Content/index.tsx

@@ -141,7 +141,8 @@ const Content = React.forwardRef<ContentRef, ContentProps>((props, ref) => {
      {({ className: motionClassName, style: motionStyle }, motionRef) => (
        <div
          key="dialog-element"
-          role="document"
+          role="dialog"


Refering to the below comment, we can add aria-labelledby here to fix the breaking changes.

Can you explain why you are recommending the addition of an attribute whose purpose is purely for the benefit of screen reader users to solve a problem with how your specific app has decided to define RTL selectors?

@mellis481
For good accessibility, everything needs to be properly labelled. A modal dialog needs to inform screen reader users about is content and purpose so it needs to be labelled.
The aria-labelledby and aria-describedby attributes are “relationship” attributes that connect the dialog to its title and description explicitly. So when focus is moved to the dialog or inside it, the text within those two elements will be read in succession.
Consider your app wants to find a unique dialog in DOM, aria-labelledby will make it unique for reading.
Not talking for our specific app but documentation suggests how to query various role for better accessibility- https://testing-library.com/docs/queries/about#priority
Other refs-
https://accessibility.huit.harvard.edu/technique-accessible-modal-dialogs
http://ncamftp.wgbh.org/sp/tests/dialog/modalDialog.html
https://developer.yoast.com/blog/the-a11y-monthly-making-modals-accessible/ - Give the modal dialog a label section

Your understanding of those aria attributes is completely incorrect.

How are those aria attributes not helpful? Please help me understand.

It looks like this PR fixes the oversight: #290

@JeffreyATW “Oversight” is a bit of an over-exaggeration. The PR that broke your tests was created to improve the lives of screen readers after a professional 3rd party accessibility auditor reviewed the AntD library. And the fix in this PR did not break the tests in this repository. No library can be expected to release HTML changes without breaking test code. In fact that’s what RTL’s philosophy is actually saying right? Ie test it the way users use it which is by finding the role on the dialogue itself.

I hope that it’s clear that the role was previously on something that was not actually the modal (it was on the background if I recall correctly from @mellis481 ’s PR). So that change was necessary to improve the relationship with the screen readers and also to get this component to pass automated a11y auditing tools.

If tests need to change, they need to change so we can support users who need our help.

If #290 allows a workaround without creating new a11y problems then that’s great. But from my perspective, the test code is incorrect not the component itself. Let’s make sure that #290 doesn’t create extra hardship for screen reader users.

If you look at the page Mike linked in his first post, you'll see that aria-labelledby should be applied to the same element as role="dialog".

I agree that role="dialog" was applied to the wrong element prior to this PR. But aria-labelledby should have been moved along with it.

Please see this comment for my findings when using a screen reader: #290 (comment)

Thank you for confirming that #290 doesn’t create hardship. I’m pleased to know that it actually improves the accessibility. Thank you so much. It seems like the fix PR has the correct HTML as you mentioned. I commented asking Zombie to consider it.

@JeffreyATW It sounds like I did not correctly interpret the message @hedaukartik was trying to communicate. I'm glad that this PR and #290 that has been created will get the modal in a much better position a11y-wise.

mellis481 added 2 commits April 11, 2022 09:10

Fixes roles

3d80787

Adds aria-modal attribute

e55652d

vercel bot deployed to Preview April 11, 2022 13:18 View deployment

dgreene1 reviewed Apr 11, 2022

View reviewed changes

vercel bot deployed to Preview April 11, 2022 16:31 View deployment

mellis481 force-pushed the fixes-roles branch from 0bd94b4 to e55652d Compare April 11, 2022 16:32

Updates snapshot

75c2a63

vercel bot deployed to Preview April 12, 2022 11:37 View deployment

Adds aria-modal to snapshot

581b8d3

vercel bot deployed to Preview April 12, 2022 12:15 View deployment

Moves aria-modal

23999f2

vercel bot deployed to Preview April 12, 2022 13:05 View deployment

Merge branch 'master' into fixes-roles

6045afa

vercel bot deployed to Preview April 18, 2022 12:29 View deployment

zombieJ merged commit 8347e4f into react-component:master Apr 18, 2022

zombieJ mentioned this pull request May 5, 2022

Modal changes from v4.20.0 are breaking React-testing-library unit test cases ant-design/ant-design#35392

Closed

hedaukartik reviewed May 13, 2022

View reviewed changes

JeffreyATW mentioned this pull request May 13, 2022

fix: for react-testing-library to find dialog by findByRole with labe… #290

Merged

mellis481 deleted the fixes-roles branch February 21, 2023 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes accessibility issue created by incorrect dialog role #285

Fixes accessibility issue created by incorrect dialog role #285

mellis481 commented Apr 11, 2022 •

edited

Loading

vercel bot commented Apr 11, 2022 •

edited

Loading

dgreene1 Apr 11, 2022

dgreene1 commented Apr 11, 2022

zombieJ commented Apr 12, 2022

codecov bot commented Apr 12, 2022 •

edited

Loading

mellis481 commented Apr 15, 2022

vercel bot commented Apr 18, 2022 •

edited

Loading

hedaukartik May 5, 2022

dgreene1 May 13, 2022

hedaukartik May 5, 2022

mellis481 May 13, 2022

hedaukartik May 13, 2022 •

edited

Loading

mellis481 May 13, 2022

hedaukartik May 13, 2022

JeffreyATW May 13, 2022

dgreene1 May 13, 2022 •

edited

Loading

JeffreyATW May 13, 2022 •

edited

Loading

dgreene1 May 13, 2022

mellis481 May 13, 2022

Fixes accessibility issue created by incorrect dialog role #285

Fixes accessibility issue created by incorrect dialog role #285

Conversation

mellis481 commented Apr 11, 2022 • edited Loading

Issue

User Impact

Compliant Code Example

Additional Details

vercel bot commented Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

dgreene1 commented Apr 11, 2022

zombieJ commented Apr 12, 2022

codecov bot commented Apr 12, 2022 • edited Loading

Codecov Report

mellis481 commented Apr 15, 2022

vercel bot commented Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hedaukartik May 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgreene1 May 13, 2022 • edited Loading

Choose a reason for hiding this comment

JeffreyATW May 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mellis481 commented Apr 11, 2022 •

edited

Loading

vercel bot commented Apr 11, 2022 •

edited

Loading

codecov bot commented Apr 12, 2022 •

edited

Loading

vercel bot commented Apr 18, 2022 •

edited

Loading

hedaukartik May 13, 2022 •

edited

Loading

dgreene1 May 13, 2022 •

edited

Loading

JeffreyATW May 13, 2022 •

edited

Loading