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

Remove aria-disabled from dialogs #2579

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Conversation

keithamus
Copy link
Member

Authors: Please fill out this form carefully and completely.

Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.

What are you trying to accomplish?

In #1891 we added aria-disabled=true to ::Dialog. This attribute makes the dialog and all children unavailable to ATs. This is far from ideal. In #1996 we changed this to ensure that the Dialog, when open, toggled its aria-disabled flag from true to false. This was better.

In #2364 we moved from our custom <modal-dialog> to native <dialog>, and somehow the aria-disabled=true fixed flag returned.

This PR removes it entirely. I do not know of a good reason to keep it, <dialog> should be AT friendly so we shouldn't need to do aria shenanigans.

Screenshots

N/A

Integration

N/A

List the issues that this change affects.

#2578

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

Remove the aria attribute.

Anything you want to highlight for special attention from reviewers?

If you can think of a good reason to keep this attribute I'd love to know it!

Accessibility

  • No new axe scan violation - This change does not introduce any new axe scan violations.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@keithamus keithamus marked this pull request as ready for review February 8, 2024 17:13
Copy link

changeset-bot bot commented Feb 8, 2024

🦋 Changeset detected

Latest commit: 0a1c8d5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

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

Copy link
Contributor

@camertron camertron left a comment

Choose a reason for hiding this comment

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

Thanks, and sorry for the trouble!

@camertron camertron merged commit c195fc5 into main Feb 8, 2024
13 of 15 checks passed
@camertron camertron deleted the Remove-aria-disabled-from-dialogs branch February 8, 2024 18:42
@primer primer bot mentioned this pull request Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants