-
Notifications
You must be signed in to change notification settings - Fork 7
Checkbox a11y fixes #90
Conversation
🦋 Changeset detectedLatest commit: ac8f7ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
}); | ||
|
||
it('does not add an error class by default', async () => { | ||
it('does not add an error class or renders aria-invalid equal to true by default', async () => { |
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.
it('does not add an error class or renders aria-invalid equal to true by default', async () => { | |
it('does not add an error class and renders aria-invalid equal to false by default', async () => { |
Is that right?
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.
Good catch @clintcs. I've updated this comment and the one for the next test. Thanks.
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.
Thank you! Thanks for your help.
- | ||
aria-invalid is set based on whether the validation feedback is displayed. This |
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.
🙏
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.
Nice work, @gfig-cs! Thanks for picking this work up!
One request before merging: could you add a changeset so that this gets included in our release? To do so:
- Run
pnpm changeset
from the root - Select the package
@crowdstrike/glide-core-components
when prompted on what changed - Do not select major, or minor changes (essentially just say no to everything)
- It should choose a
patch
at that point
- It should choose a
- Write a helpful summary. This summary goes in the release notes.
Happy to pair with you on it early next week if you want another pair of eyes going through this process.
is to handle an odd behavior with checkboxes where "Invalid Data" is announced | ||
on required unchecked inputs. While this is technically correct, it's | ||
inconsistent with how other form controls behave as their validity isn’t announced | ||
on screen readers by default before validation. |
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.
Excellent helpful comment!
db25281
to
ac8f7ef
Compare
Thanks @ynotdraw. I think I got it from the helpful script prompts. |
@gfig-cs yup looks great! Thank you. Feel free to merge away whenever you're ready |
Upgrade Storybook (#84) Add Toggle (#78) Add lit-plugin extension (#86) update modal stories update story and modal icon button a11y update story types Add input (#83) * Copy component over from previous repo and auto-fix * Update per new conventions. 100% test coverage Fix up class names per stylelint Fix up import paths Fix up icons Remove clickOnElement helper Sufficient to use native element's .click() method Allow passing of invalid types to input This matches native: The user can pass whatever string they want. TypeScript will properly message to the user if they use an invalid type. Hit 100% test coverage. Remove unnecessary type narrowing Remove unused state Close shadow root for Input, except in tests Add changeset for Input Prefer prefixing event handlers with "on" Fix up icons and icon spacing Restrict exports (#87) Switch Tree Item to dot notation (#89) Checkbox a11y fixes (#90) * Hide asterisk from screenreader announcement * Add aria-invalid attr and tests feat(Tag): move from glide to glide-core (#81) Adds tag Add docs + update usages targeting root element with CSS (#85) Remove label attribute from cs-menu (#92) Was not being used. Probably a leftover from a previous iteration in the old repo. Add a lint rule enforcing private fields and "ElementRef" suffixes for Lit refs (#95) style(ExampleIcon, Tag): adds "size" variable (#94) Adds a "size" css variable to `cs-example-icon` and is applied in `cs-tag` Docs(Tag story): correct typo (#98) Fixes typos in `cs-tag` story Button a11y fixes (#97) * Support enter keydown event * Hide icons from screen readers in button stories update modal and tests update modal, test and stories update tests update styles minor naming change Renamed cs-drawer-width to simply width (#99) Add "padding-line-between-statements" lint rule (#101) Add Dropdown "quiet" variant (#96) linting update modal and stories
🚀 Description
Adds two a11y related fixes to the Checkbox component:
*
when the input is required so it isn't announced by screen readers. I looked into using CSS content alternative text but it isn't fully supported by all browsers yet.aria-invalid
attribute and set it to true only when displaying a validation message to get around the screen reader announcing the control as invalid ("Invalid Data") when it was required and unchecked but not validated. The behavior before this change was inconsistent with how other controls behave and usingaria-invalid
seemed like a good compromise for consistency.aria-invalid
.📋 Checklist
🔬 How to Test
https://glide-core.crowdstrike-ux.workers.dev/?path=/docs/checkbox--overview
*
not announcing:*
label is no longer announced by VoiceOver"Invalid Data" only announced with error:
📸 Images/Videos of Functionality