-
Notifications
You must be signed in to change notification settings - Fork 361
fix(LEMS-3666): Adds roles to ul and li #3029
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
base: main
Are you sure you want to change the base?
Conversation
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +12 B (0%) Total Size: 498 kB
ℹ️ View Unchanged
|
492b657 to
ad3c416
Compare
…n radio to enable group label to be read
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (4858faf) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3029If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3029If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3029 |
SonicScrewdriver
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great to me, and it looks like it already passed QA! Thank you :)
packages/perseus/src/widgets/radio/multiple-choice-component.new.tsx
Outdated
Show resolved
Hide resolved
mark-fitzgerald
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The writeup that you did for this PR is super helpful! I think it should also be added to these two files in order to explain why the redundant role information is added in the first place. That way, our future selves won't remove them (thinking we are cleaning things up).
ivyolamit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks logical to me 👍🏼
SonicScrewdriver
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great, thank you!
| // eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-noninteractive-element-interactions | ||
| <li className={classes} onClick={clickHandler}> | ||
| // eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-noninteractive-element-interactions, jsx-a11y/no-redundant-roles | ||
| <li className={classes} onClick={clickHandler} role="listitem"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if some of these rules are helpful if we need to keep overriding them in our efforts to have a good a11y experience. 🤔
Summary:
Different screen readers handle
<fieldset>and<legend>elements inconsistently, causing unpredictable announcement behavior:This inconsistency means some users hear the critical instructions ("Choose 1 answer", "Choose 3 answers", etc.) while others miss them entirely, depending on their assistive technology and navigation method.
Solution
Add redundant
role="list"androle="listitem"attributes to preserve list semantics, as some screen readers may remove the implicit list role whenlist-style: noneis applied via CSSIssue: LEMS-3666
Test plan:
aria-hiddenremovedaria-hiddenandaria-labelledbyremoved