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

Jump to bottom

Fix: Add missing accessibility aria-modal=true to the Modal and aria-label #588

Merged
merged 4 commits into from
Apr 20, 2020
Merged

Fix: Add missing accessibility aria-modal=true to the Modal and aria-label #588

merged 4 commits into from
Apr 20, 2020

Conversation

alexandrzavalii
Copy link
Contributor

@alexandrzavalii alexandrzavalii commented Apr 17, 2020 •
edited by NicholasBoll
Loading

Summary

Fixes #587

Checklist

  • branch has been rebased on the latest master commit
  • tests are changed or added
  • yarn test passes
  • all (dev)dependencies that the module needs is added to its package.json
  • code has been documented and, if applicable, usage described in README.md
  • module has been added to canvas-kit-react and/or canvas-kit-css universal modules, if
    applicable
  • design approved final implementation
  • a11y approved final implementation
  • code adheres to the API & Pattern guidelines

Additional References

@alexandrzavalii alexandrzavalii changed the title fix: Add missing accessibility aria-modal=true fix(Popup): Add missing accessibility aria-modal=true Apr 17, 2020
@alexandrzavalii alexandrzavalii changed the title fix(Popup): Add missing accessibility aria-modal=true Fix: Add missing accessibility aria-modal=true to the Popup Apr 17, 2020
@cypress
Copy link

cypress bot commented Apr 17, 2020 •
edited
Loading


Test summary

223 • 0 • 1 • 0

Run details
Project canvas-kit
Status Passed
Commit c9b703e ℹ️
Started Apr 20, 2020 4:48 PM
Ended Apr 20, 2020 4:50 PM
Duration 02:18 💡
OS Linux Ubuntu Linux - 18.04
Browser Electron 80

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@alexandrzavalii alexandrzavalii force-pushed the popup-missing-accessibility branch from 18e9545 to 6403472 Compare April 17, 2020 08:56
@alexandrzavalii alexandrzavalii changed the title Fix: Add missing accessibility aria-modal=true to the Popup Fix: Add missing accessibility aria-modal=true to the Modal Apr 17, 2020
@alexandrzavalii alexandrzavalii changed the title Fix: Add missing accessibility aria-modal=true to the Modal Fix: Add missing accessibility aria-modal=true to the Modal and aria-label Apr 17, 2020
@alexandrzavalii alexandrzavalii force-pushed the popup-missing-accessibility branch 2 times, most recently from 52aeeb1 to d19bcae Compare April 17, 2020 14:37
@alexandrzavalii alexandrzavalii force-pushed the popup-missing-accessibility branch from d19bcae to 6fdadd8 Compare April 17, 2020 15:46
@NicholasBoll NicholasBoll self-assigned this Apr 17, 2020
@lychyi
Copy link
Contributor

lychyi commented Apr 17, 2020

@alexandrzavalii Thanks for the contribution again!

@NicholasBoll
Copy link
Member

I'll add a Cypress test for the new aria label we added.

NicholasBoll
NicholasBoll reviewed Apr 17, 2020
View reviewed changes
modules/modal/react/lib/ModalContent.tsx
@@ -215,6 +220,8 @@ const ModalContent = ({
handleClose={handleClose}
padding={padding}
transformOrigin={transformOrigin}
aria-modal={true}
Copy link
Member

Choose a reason for hiding this comment

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

It looks like aria-modal=true is supposed to replace the functionality of setting aria-hidden=true on children, but isn't fully supported yet.

Do you know if this technique will work with multiple modal dialogs stacked? The aria-hidden should work in that case, but I'm not sure if there are multiple aria-modal=true. Should the previous modal get a set back to "false" or be removed?

Copy link
Member

Choose a reason for hiding this comment

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

I created a test: https://codesandbox.io/s/stacked-modal-qhcp6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lychyi Thanks for taking a look!:)
@NicholasBoll good question! I will ask Peter. I would assume since we have aria-hidden for the first popup aria-modal would be ignored.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe that will work as long as we apply aria-hidden to things. I assume broad support for aria-modal is a ways off, so we can worry about it then.

@NicholasBoll NicholasBoll merged commit 91b0513 into Workday:prerelease/v4 Apr 20, 2020
@NicholasBoll NicholasBoll linked an issue Apr 22, 2020 that may be closed by this pull request
Add missing accessibility attribute to the Modal #587
Closed
@anicholls anicholls mentioned this pull request May 5, 2020
Merged
@anicholls anicholls mentioned this pull request Jun 15, 2020
Merged
@NicholasBoll NicholasBoll mentioned this pull request Jun 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicholasBoll NicholasBoll NicholasBoll left review comments

Assignees

@NicholasBoll NicholasBoll

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add missing accessibility attribute to the Modal
3 participants
@alexandrzavalii @lychyi @NicholasBoll