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 attributes in order to read card body content #90

Merged
merged 4 commits into from
Feb 26, 2025

Conversation

jomcarvajal
Copy link

With NVDA, the card content wasn’t announced. The focus going to the include button causes that to be read, instead of reading “Question 1: option A, option B” etc.

Solution: remove role and aria-label from card content to allow Screen Reader to watch card's content.

@jomcarvajal jomcarvajal requested a review from a team as a code owner February 18, 2025 15:15
@jomcarvajal jomcarvajal requested review from bethshook and jivey and removed request for a team February 18, 2025 15:15
@@ -103,7 +103,7 @@ export const AnswersTable = (props: AnswersTableProps) => {
});

return (
<div role="radiogroup" aria-label="Answer choices" className="answers-table">
Copy link
Author

Choose a reason for hiding this comment

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

@jivey using NVDA I noticed that Answer choices appears always, no matter the content. Removing this aria attributes NVDA is capable to read inside card's content. I didn't see any other a11y issue after removing role="radiogroup" but let me know if you're fine with this approach

Copy link
Member

Choose a reason for hiding this comment

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

The label and role here were recently added for accessibility so they probably need to stay here... can you share a screenshot of the NVDA speech that you are seeing? When I test with focus first off, I get this speech which is reading the options:

Answer choices  grouping       radio button  unavailable  not checked    clickable  A  Option 1     radio button  unavailable  not checked    clickable  B  Option 2     radio button  unavailable  not checked    clickable  C  Option 3     radio button  unavailable  not checked    clickable  D  Option 4

Additionally, I'm seeing we have a new issue here I didn't see before: those are being read as clickable, can we disable them while in preview mode?

Copy link
Author

Choose a reason for hiding this comment

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

Using VoiceOver with the label and role I get this:
Screenshot 2025-02-19 at 10 05 26 AM

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay yes that seems like a problem, at least for our purposes. It's a newer change that I'm not familiar with: #78 My initial thought is that an aria label shouldn't be on that, as it has a role, but I'm not sure. Can you look into that? And if you determine the label needs to be there, we may have to make it conditional for preview cards...

Copy link
Author

Choose a reason for hiding this comment

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

For some reason role is removing Answers from screen reader. I removed the label just to test but Answers still missing as you can see in my previous screenshot. @TylerZeroMaster maybe you can help us to understand this a11y changes. Are we expecting that answers from a card only appears as "Answer choices" in screen readers?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jomcarvajal I think the aria-label and role can be removed from the AnswersTable if all the radio buttons have the same name. When they have the same name, they should implicitly be in a radiogroup.

When using HTML's native input radio button, , the radio buttons are grouped when each of input radio buttons in the group is given the same name.

I may have followed the original issue, which suggested adding the radiogroup role, too closely when I implemented those changes. After further reading:

It is recommended to create radio groups by using same-named HTML input radio buttons, but, if you must use ARIA roles and attributes instead of semantic HTML form controls, custom radio buttons can and should act like native HTML radio input buttons.

So if it works better to create an implicit radiogroup by using same-named radio buttons, then I am all for that solution.

link for both quotes

@jomcarvajal
Copy link
Author

@jivey @TylerZeroMaster Let's keep it disabled just for preview cards for now

@jivey
Copy link
Member

jivey commented Feb 24, 2025

@jomcarvajal The conditional look good, thanks for working on this. Before I approve can we make one more change, I see clickable gets read (at least by NVDA) probably because the radio inputs are not disabled - can you add that just for preview mode?

@jomcarvajal
Copy link
Author

@jivey I already added disabled: previewMode || !choicesEnabled, but I'll review what is going on

@jomcarvajal
Copy link
Author

@jivey I already added disabled: previewMode || !choicesEnabled, but I'll review what is going on

I forget to pass the previewMode prop my bad

Copy link
Member

@jivey jivey left a comment

Choose a reason for hiding this comment

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

Looks good, can you add it to the overlay preview card story too? It isn't reading right there, so the prop is probably missing.

@jomcarvajal jomcarvajal merged commit a2d2d97 into preview-card Feb 26, 2025
2 checks passed
@jomcarvajal jomcarvajal deleted the CORE-675-card-body-a11y branch February 26, 2025 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants