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

fix(skeleton-components): update labels & attributes for Checkbox, NumberInput, Select, Slider, TextArea, and TextInput #4307

Merged
merged 25 commits into from
Oct 22, 2019

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Oct 11, 2019

Closes #4176
Also partially addresses #4310

The components flagged in #4176 had label elements that weren't tied to any particular input, because these are all non-interactive skeleton components. So I changed those label elements to spans

Changelog

Changed

  • For each component skeleton, I changed the label element to a span
  • For each component's skeleton story, I added aria-label="loading [component name]", aria-live="assertive", role="status", and tabindex="0" to the wrapper element

Testing / Reviewing

Open the carbon-components-react deployment review and check these skeleton components against the latest Sept 2019 ruleset with the DAP tool.

@jendowns jendowns requested a review from a team as a code owner October 11, 2019 16:16
@ghost ghost requested review from emyarod and jnm2377 October 11, 2019 16:16
@jendowns
Copy link
Contributor Author

Tagging @dakahn for this one as well ☝️

@netlify
Copy link

netlify bot commented Oct 11, 2019

Deploy preview for the-carbon-components ready!

Built with commit 095f70e

https://deploy-preview-4307--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Oct 11, 2019

Deploy preview for carbon-components-react ready!

Built with commit 095f70e

https://deploy-preview-4307--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Oct 11, 2019

Deploy preview for carbon-elements ready!

Built with commit 095f70e

https://deploy-preview-4307--carbon-elements.netlify.com

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Our requirements around skeletons (loading states) for components are changing. Referencing two things here:
https://www.w3.org/WAI/WCAG21/Understanding/status-messages.html
https://www.ibm.com/able/guidelines/ci162/status_messages_71.html

There's a lot in there, but relevant to this PR is our skeletons do actually need to be interactable in so far as they can receive focus (be tabbable) and communicate the status via aria of that particular component.

@jendowns
Copy link
Contributor Author

Thanks @dakahn that's very interesting 🤔 I could add a tabindex to these elements for now?

@jendowns jendowns changed the title fix(skeleton-components): update labels for Checkbox, NumberInput, Select, Slider, TextArea, and TextInput fix(skeleton-components): update labels & attributes for Checkbox, NumberInput, Select, Slider, TextArea, and TextInput Oct 11, 2019
@jendowns
Copy link
Contributor Author

Thanks again @dakahn -- I just added tabindex='0', role='status', aria-live="assertive" to each of these skeleton components in my latest commit 👍

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @jendowns!

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

hideLabel logic for the TextInput skeleton is reversed, otherwise looks good to me

@jendowns
Copy link
Contributor Author

Thanks @emyarod! Just committed your suggestion 👍

Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

LGTM! I'm not seeing the DAP violation anymore 👍

@jendowns
Copy link
Contributor Author

jendowns commented Oct 15, 2019

@joshblack @asudoh @dakahn @emyarod @jnm2377 I just pushed a commit that adds aria-label attributes to these skeleton components, per #4310

@joshblack
Copy link
Contributor

@dakahn I would think we'd want folks to specify that a region is loading versus specific components, or at the very least incorporate useAnnouncer such that the messages are delayed. If we had 5 input elements in a form that is loading, wouldn't only the last one announce completely as the other ones would be interrupted by the next message in the source order? Also, what happens once the part of the page is loaded? Should there be another aria-live message communicated?

@joshblack
Copy link
Contributor

Quick demo of what I mean:

Screen Recording 2019-10-15 at 11.55.29 AM.zip

Codepen: https://codepen.io/joshblack/pen/ZEEWZYr?editors=1011

The way it works is that clicking the button would change the attributes of each of the outlined spans so that aria-live is set and an appropriate aria label is set. You almost don't even realize in VO that multiple things have aria-live as they are never announced

@jendowns
Copy link
Contributor Author

@dakahn @joshblack Oh whoa, that's interesting... so maybe I should remove aria-live from these components for now until this is investigated further?

@joshblack
Copy link
Contributor

@jendowns yeah, my guess is that if we want to remove the violations we should just treat these as decorative content until we figure out a live region example. This would mean removing those aria attributes, I believe, in the skeletons.

The only follow-up would be that the stories should most likely be updated to be in a live region example that does have these attributes, right @dakahn? I think if we can figure that out then this should be good to go

@jendowns
Copy link
Contributor Author

@dakahn @joshblack I just removed the aria-live attributes from these skeleton components. Please let me know if you want me to remove the aria-label attributes as well.

@joshblack
Copy link
Contributor

I say go for it, what do you think @dakahn?

Only thing will be to make sure the skeleton stories do include a region that has all of these attributes on it.

@jendowns
Copy link
Contributor Author

@dakahn @joshblack in my last 2 comments, I removed aria-label from the skeleton components themselves, and then I added aria-label="loading [component name]" and aria-live="assertive" to each of these skeleton component's wrappers in storybook. Is that the desired direction here?

@dakahn
Copy link
Contributor

dakahn commented Oct 21, 2019

Hmm, tested with NVDA on latest Chrome and I'm not getting an announcement on page load that there is a component loading. I think your aria might be getting clobbered by the aria on the "main" Storybook element, but I'm not sure.

@joshblack
Copy link
Contributor

@dakahn hmm, seems to work in VO so maybe there is an additional requirement for NVDA that we don't know about?

@dakahn
Copy link
Contributor

dakahn commented Oct 21, 2019

That's fine. Green check from me. Let's get it in.

@joshblack
Copy link
Contributor

joshblack commented Oct 21, 2019

@dakahn @jendowns do we still need role and tabindex on the skeletons? or would these move to the regions?

@dakahn
Copy link
Contributor

dakahn commented Oct 21, 2019

Just to clarify what we're looking for here:

We want no aria on the skeletons themselves. This will clear the DAP violation. Then we want a wrapper div with the aria required to announce the loading state. This doesn't technically help our shipping skeletons be any more accessible, but the plan is to work on a region pattern users can implement around their skeletons in a coming work cycle.

@jendowns
Copy link
Contributor Author

@dakahn @joshblack I just pushed a commit that moves role="status" and tabindex="0" from the skeleton component's code to their wrapper elements in storybook.

@joshblack joshblack merged commit d99c3d9 into carbon-design-system:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AVT 1 - Skeleton: Checkbox, NumberInput, Select, Slider, TextArea & TextInput DAP violation for missing label
6 participants