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.

Sign up for GitHub

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

Fixes accessibility issue created by incorrect dialog role #285

Merged
merged 6 commits into from
Apr 18, 2022
Merged

Fixes accessibility issue created by incorrect dialog role #285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3d80787
Fixes roles
mellis481 Apr 11, 2022
e55652d
Adds aria-modal attribute
mellis481 Apr 11, 2022
75c2a63
Updates snapshot
mellis481 Apr 12, 2022
581b8d3
Adds aria-modal to snapshot
mellis481 Apr 12, 2022
23999f2
Moves aria-modal
mellis481 Apr 12, 2022
6045afa
Merge branch 'master' into fixes-roles
mellis481 Apr 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/Dialog/Content/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ export type ContentProps = {
onVisibleChanged: (visible: boolean) => void;
onMouseDown: React.MouseEventHandler;
onMouseUp: React.MouseEventHandler;
} & IDialogChildProps
} & IDialogChildProps;

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

Choose a reason for hiding this comment

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

Thanks prettier


const Content = React.forwardRef<ContentRef, ContentProps>((props, ref) => {
const {
Expand Down Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@hedaukartik hedaukartik May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your understanding of those aria attributes is completely incorrect.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

It looks like this PR fixes the oversight: #290

Copy link

@dgreene1 dgreene1 May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link

@JeffreyATW JeffreyATW May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

aria-modal="true"
ref={motionRef}
style={{ ...motionStyle, ...style, ...contentStyle }}
className={classNames(prefixCls, className, motionClassName)}
Expand Down
1 change: 0 additions & 1 deletion src/Dialog/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ export default function Dialog(props: IDialogChildProps) {
className={classNames(`${prefixCls}-wrap`, wrapClassName)}
ref={wrapperRef}
onClick={onWrapperClick}
role="dialog"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

aria-labelledby={title ? ariaId : null}
style={{ zIndex, ...wrapStyle, display: !animatedVisible ? 'none' : null }}
{...wrapProps}
Expand Down
8 changes: 4 additions & 4 deletions tests/__snapshots__/index.spec.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ exports[`dialog add rootClassName should render correct 1`] = `
/>
<div
class="rc-dialog-wrap"
role="dialog"
style="font-size: 10px;"
tabindex="-1"
>
<div
aria-modal="true"
class="rc-dialog"
role="document"
role="dialog"
style="width: 600px; height: 903px;"
>
<div
Expand Down Expand Up @@ -59,12 +59,12 @@ exports[`dialog should render correct 1`] = `
<div
aria-labelledby="test-id"
class="rc-dialog-wrap"
role="dialog"
tabindex="-1"
>
<div
aria-modal="true"
class="rc-dialog"
role="document"
role="dialog"
>
<div
aria-hidden="true"
Expand Down