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

Add live region to announce "Copied!" on ClipboardCopy component #2843

Merged
merged 17 commits into from
May 16, 2024

Conversation

lindseywild
Copy link
Contributor

@lindseywild lindseywild commented May 15, 2024

What are you trying to accomplish?

Adds a live region to the ClipboardCopy component so that by default, "Copied!" is announced when a user successfully copies something.

Screenshots

Before

Screen recording of the ClipboardCopy button on Lookbook, when pressing "Enter" or "Control + Option + Space", no feedback is announced but there is a visual icon change from 2 pages stacked on top of each other to a green check mark

CleanShot.2024-05-15.at.11.43.23.mp4

After

Screen recording of the ClipboardCopy button on Lookbook, when pressing "Enter" or "Control + Option + Space", "Copied!" is announced

CleanShot.2024-05-15.at.11.42.25.mp4

Integration

No

List the issues that this change affects.

Related to https://github.com/github/accessibility-audits/issues/6469 & https://github.com/github/accessibility/issues/6096.

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?

I am adding a live region to the ClipboardCopy button so that, by default, screen reader feedback will be announced.

I considered adding a live region to the clipboard-copy-element web component which is used under the hood, but changing that component to include a live region would be a breaking change since we'd need to wrap the current inner text with another element so that we can have a live region as a sibling. Nesting a live region inside of a <button> element has poor support and is not recommended.

Anything you want to highlight for special attention from reviewers?

  • Is there a better way to wrap the component?
  • Is there a better way than looking for the parentNode of the target element to target the specific aria-live region for one component (thinking if there are multiple ClipboardCopy elements on the page and not wanting them to conflict)? I didn't want to change the API too much by making the wrapper I added the new <clipboard-copy> component but maybe it's ok?

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.

Copy link

changeset-bot bot commented May 15, 2024

🦋 Changeset detected

Latest commit: 59e8e0a

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 Minor

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

@lindseywild lindseywild marked this pull request as ready for review May 15, 2024 17:34
@lindseywild lindseywild requested a review from a team as a code owner May 15, 2024 17:34
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! 🎉

app/components/primer/beta/clipboard_copy.ts Show resolved Hide resolved
app/components/primer/beta/clipboard_copy.ts Outdated Show resolved Hide resolved
test/system/beta/clipboard_copy_button_test.rb Outdated Show resolved Hide resolved
app/components/primer/beta/clipboard_copy.ts Outdated Show resolved Hide resolved
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.

2 participants