+
Skip to content
This repository was archived by the owner on Oct 14, 2025. It is now read-only.

Conversation

gfig-cs
Copy link
Contributor

@gfig-cs gfig-cs commented May 3, 2024

🚀 Description

Adds two a11y related fixes to the Checkbox component:

  • Refactored how we display the * 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.
  • Added the 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 using aria-invalid seemed like a good compromise for consistency.
  • Updated the checkbox tests to cover the use of aria-invalid.

📋 Checklist

  • I have read and followed the Contributing Guidelines.
  • I have added tests to cover new or updated functionality.
  • I have created or updated stories in Storybook to document the new functionality.
  • I have included a changeset with this Pull Request if it adds/updates/removes functionality for consumers.
  • I have scheduled a Design Review for these changes, if one is required.
  • I have followed the ARIA Authoring Practices Guide and/or met with the Accessibility Team to ensure this functionality is accessible.

🔬 How to Test

https://glide-core.crowdstrike-ux.workers.dev/?path=/docs/checkbox--overview

* not announcing:

  1. Make the checkbox required on any of the checkbox stories.
  2. Verify that the * label is no longer announced by VoiceOver

"Invalid Data" only announced with error:

  1. With the checkbox required and unchecked on any stories with that don't display an error.
  2. Verify VoiceOver no does not include "Invalid Data" when announcing the control.
  3. With the checkbox required and unchecked on any stories with that do display an error.
  4. Verify VoiceOver no does include "Invalid Data" when announcing the control.

📸 Images/Videos of Functionality

Copy link

changeset-bot bot commented May 3, 2024

🦋 Changeset detected

Latest commit: ac8f7ef

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@crowdstrike/glide-core-components Patch

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

Copy link
Contributor

github-actions bot commented May 3, 2024

});

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 () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙏

Copy link
Collaborator

@ynotdraw ynotdraw left a 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:

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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent helpful comment!

@gfig-cs gfig-cs force-pushed the fix-checkbox-a11y-fixes branch from db25281 to ac8f7ef Compare May 6, 2024 17:47
@gfig-cs
Copy link
Contributor Author

gfig-cs commented May 6, 2024

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:

Happy to pair with you on it early next week if you want another pair of eyes going through this process.

Thanks @ynotdraw. I think I got it from the helpful script prompts.

@ynotdraw
Copy link
Collaborator

ynotdraw commented May 6, 2024

@gfig-cs yup looks great! Thank you. Feel free to merge away whenever you're ready

@gfig-cs gfig-cs merged commit e58d27f into main May 6, 2024
@gfig-cs gfig-cs deleted the fix-checkbox-a11y-fixes branch May 6, 2024 19:13
@github-actions github-actions bot mentioned this pull request May 3, 2024
dylankcrwd added a commit that referenced this pull request May 9, 2024
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载