-
Notifications
You must be signed in to change notification settings - Fork 77
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
new(Tooltip): Add optional accessibilityLabel for perf #357
Conversation
Size Changes
View raw build statsPrevious (master){
"apollo": {
"esm": 10832,
"lib": 14147
},
"app-shell": {
"esm": 12906,
"lib": 19874
},
"composer": {
"esm": 68247,
"lib": 101805
},
"core": {
"esm": 562515,
"lib": 705353
},
"forms": {
"esm": 35672,
"lib": 47577
},
"icons": {
"esm": 156355,
"lib": 205626
},
"layouts": {
"esm": 15298,
"lib": 20770
},
"metrics": {
"esm": 5467,
"lib": 7729
},
"test-utils": {
"esm": 4279,
"lib": 5937
}
} Current{
"apollo": {
"esm": 10832,
"lib": 14147
},
"app-shell": {
"esm": 12906,
"lib": 19874
},
"composer": {
"esm": 68247,
"lib": 101805
},
"core": {
"esm": 562671,
"lib": 705524
},
"forms": {
"esm": 35672,
"lib": 47577
},
"icons": {
"esm": 156355,
"lib": 205626
},
"layouts": {
"esm": 15298,
"lib": 20770
},
"metrics": {
"esm": 5467,
"lib": 7729
},
"test-utils": {
"esm": 4279,
"lib": 5937
}
} |
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.
Slick 👏
<Spacing bottom={1}> | ||
<Tooltip | ||
accessibilityLabel="Override accessibility content" | ||
content="I have an aria-labelledby instead of aria-label" |
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.
noob q: is this the other way around?
(i.e here because we added accessibelityLabel, this has an aria-label instead of an aria-labelledby)
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.
yes! good catch 😅 I swapped these around a couple times and tho I got the children
text right I failed to swap the content
🙏
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.
🔥
@@ -18,6 +18,8 @@ const EMPTY_TARGET_RECT: ClientRect = { | |||
}; | |||
|
|||
export type TooltipProps = { | |||
/** Accessibility label. If not specified, all tooltip content is duplicated in an accessibility portal rendered at all times. */ |
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.
small nit: to change the last part of the sentence to is duplicated, rendered in an off-screen element with a separate layer.
to: @milesj @stefhatcher @hayes @alecklandgraf
cc @avand @stopachka
Description
This PR adds an optional
accessibilityLabel
prop to theTooltip
, mostly to enable an opt-in (non-breaking) perf improvement.Motivation and Context
Currently the
content
prop ofTooltip
is rendered in an invisiblePortal
for a11y, to serve as anaria-labeledby
element that is rendered whether or not theTooltip
popup is visible. This can lead to performance issues on pages with 100s ofTooltips
ascontent
is rendered at all times.We discussed that one solution to ensure accessibility but allow for optimization is to override this behavior by specifying an
aria-label
(accessibilityLabel
prop) to be used instead.Note: there was some discussion about whether omission of
aria-labelledby
/duplication of content should be conditional on whetherprops.toggleOnClick === false
as you could potentially have functionality incontent
that would not be captured with a simple label. After thinking about this more I think it could be confusing to the developer but am still open to adding it if you want.Testing
aria-label
andaria-labelledby
work in new storyScreenshots
New example

Checklist