Accessibility fixes for copy button#313
Conversation
| if (btnEl) { | ||
| const label = btnEl.getAttribute('aria-label'); | ||
| btnEl.innerText = getI18nText('operations.copied'); | ||
| btnEl.setAttribute('aria-label', getI18nText('operations.copied')); |
There was a problem hiding this comment.
Is this really the correct thing to do for a screen reader? Setting the aria-label and changing it? That feels wrong to me, but I don't really understand how aria-labels work. What's your thinking here?
There was a problem hiding this comment.
It's a bit complicated because the button itself contains the status update text for the success ('copied'). We want to let users know the 'copied' success status, so there's an aria-live region on the button. But screen readers will never read the actual content of the button if aria-label is also set, they will always read that instead. So we have to update the aria-label as well for the status to be announced.
There was a problem hiding this comment.
I refuse to believe this is right way to solve this problem for screen readers. Why would a screen reader announce the button label change?
Multiple llms suggested to me to have a separate hidden div with the text that can be targeted for update instead of this here. Because actually I worry that since we are updating the button text twice which means that it could actually display both the "copied" message and then again the "label" message which not correct.
Is that accurate, IDK, but that feels better than this implementation.
| : html` | ||
| <div class="tab-content col m-markdown" style="flex:1; display:${this.activeResponseTab === 'response' ? 'flex' : 'none'};" > | ||
| <syntax-highlighter style="min-height: 60px" mime-type="${this.responseContentType}" .content="${this.responseText}"/> | ||
| <syntax-highlighter style="min-height: 60px" mime-type="${this.responseContentType}" .content="${this.responseText}" .label="Response text"/> |
There was a problem hiding this comment.
Almost forgot, all of these labels need to be i18n keys instead of hard coded text because we support multiple languages.
2 participants
Currently, there are several accessibility issues with the copy button:
This PR fixes these issues by:
Also note that scrollable regions on a page must be able to receive tab focus, and stuff that's focussable should be labeled and given a role if possible, so this PR also adds a label, role, and tabindex to the scrollable region that the copy button copies.