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.

Sign up for GitHub

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

Jump to bottom

fix(tooltip): add aria-labelledby #3011

Closed
wants to merge 13 commits into from
Closed

fix(tooltip): add aria-labelledby #3011

wants to merge 13 commits into from

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented Jun 11, 2019

Closes #2736

Updates Tooltip. If a the user provides a triggerText prop aria-labelledby is set to that id -- if they don't we set aria-label to iconDescription.

Changelog

  • replaced aria-describedby with aria-labelledy on l372.

cc @emyarod @snidersd, I know you were working in this space and want to make sure I have #2736's intent right.

@dakahn dakahn requested a review from snidersd June 11, 2019 05:50
@dakahn dakahn requested a review from emyarod June 11, 2019 05:50
@netlify
Copy link

netlify bot commented Jun 11, 2019 •
edited
Loading

Deploy preview for the-carbon-components ready!

Built with commit e71dcaa

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

@netlify
Copy link

netlify bot commented Jun 11, 2019 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit e71dcaa

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

@netlify
Copy link

netlify bot commented Jun 11, 2019 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit e71dcaa

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

emyarod
emyarod reviewed Jun 11, 2019
View reviewed changes
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.

shouldn't aria-label also support taking the value from the user setting the attribute directly?

@snidersd
Copy link

Confirmed DAP does not have any violations.

@dakahn
Copy link
Contributor Author

dakahn commented Jun 11, 2019

If they wanted to specify an aria-label wouldn't they be doing that via triggerText anyway? @emyarod If we allowed them to set the aria-label directly wouldn't we be possibly back where we started with redundant labels?

@emyarod
Copy link
Member

emyarod commented Jun 11, 2019

what I mean is, if the user does not use triggerText and does not use iconDescription, aria-label can still be directly passed in

if the user sets aria-label directly, it will go here rather than here right? is that the correct location for the attribute?

@dakahn
Copy link
Contributor Author

dakahn commented Jun 11, 2019

@emyarod it looks like we require one or the other here.

@emyarod
Copy link
Member

emyarod commented Jun 11, 2019

right but since aria-label can still be directly passed in, do we want to control which element it's applied to or leave it as it is now?

@dakahn
Copy link
Contributor Author

dakahn commented Jun 11, 2019

Oooh I see. Okay, thanks for clearing that up!

@snidersd
Copy link

After retesting all the Tooltip components, there are still DAP violations in the TooltipDefinition as follows:
Screen Shot 2019-06-12 at 2 51 31 PM

And the TooltipIcon:
Screen Shot 2019-06-12 at 2 52 20 PM

@dakahn dakahn requested review from emyarod and joshblack June 14, 2019 00:31
@dakahn
Copy link
Contributor Author

dakahn commented Jun 14, 2019 •
edited
Loading

Unless I'm wrong here it seems like if the user doesn't provide a custom ID for the tooltip, we generate one for them, and then all we do with it is set it to our aria-describedby attribute -- which throws a DAP error because that's not actually an ID for any real element? Feel like I might be missing something @asudoh can I ping you for another set of eyes on TooltipDefinition.js? I went ahead and removed the ID generation with getInstanceOf() -- so you can reference the diff.

@snidersd Since the aria-label is the tooltip's text do we also need aria-describedby?

emyarod
emyarod reviewed Jun 14, 2019
View reviewed changes
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.

I think it's fine to remove the id generator for definition tooltip. seems it was carried over from the interactive tooltip? but the two tooltips are built differently

packages/react/src/components/Tooltip/Tooltip.js
return { 'aria-labelledby': triggerId };
}
// if the user provides neither of those set aria-label to icon-description
return { 'aria-label': iconDescription };
Copy link
Member

Choose a reason for hiding this comment

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

how are you getting iconDescription here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, getting back to this now. :D That's just a prop. Are you thinking it should be required if we don't have either ariaLabel or triggerText?

Copy link
Member

Choose a reason for hiding this comment

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

what I mean is it looks like it's undefined. you would need to get the value from this.props.iconDescription first

joshblack
joshblack reviewed Jun 17, 2019
View reviewed changes
Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Seems great after @emyarod's suggestions 👍

@snidersd
Copy link

@dakahn I'm only seeing a DAP issue with the TooltipDefintion and the TooltipIcon. However, the Tooltip, which is actually a Modal does not restrict tab focus to the modal.

emyarod
emyarod reviewed Jun 28, 2019
View reviewed changes
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.

the DAP issues are for TooltipDefinition and TooltipIcon only right? and the issue for the interactive tooltip is the lack of a focus trap? I'm still seeing all of these issues unless this PR is addressing something different

@joshblack
Copy link
Contributor

@dakahn do we want to get this in for 10.4 or should we close/revisit it at a later time?

@dakahn
Copy link
Contributor Author

dakahn commented Jul 8, 2019

This isn't 2.4 -- seems like this devolved into weird unnecessary refactor work. I'll address #2736 in another PR 😕

@dakahn dakahn closed this Jul 8, 2019
This was referenced Jul 13, 2021
Open
Open
Open
Open
Open
This was referenced Aug 4, 2021
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Sep 1, 2021
Open
Open
Open
Open
Open
Open
@snyk-bot snyk-bot mentioned this pull request Sep 15, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshblack joshblack joshblack approved these changes

@emyarod emyarod emyarod left review comments

@snidersd snidersd Awaiting requested review from snidersd

Assignees
No one assigned
Labels
status: one more review 👀 type: a11y ♿
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AVT 1 &AVT 2 - Default Tooltip has keyboard issue and TooltipDefinition and Icon have DAP violations
4 participants
@dakahn @snidersd @emyarod @joshblack