-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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(Tooltip): don't add aria-labelled by if aria-label is passed to Tooltip or trigger #12555
fix(Tooltip): don't add aria-labelled by if aria-label is passed to Tooltip or trigger #12555
Conversation
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 2c98aa23ebc33ac6005ce2eaff949f8eb27b16a0 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
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.
@@ -10,30 +14,21 @@ import tooltipAsDescriptionBehavior, { TooltipBehaviorProps } from './tooltipAsD | |||
*/ | |||
const tooltipAsLabelBehavior: Accessibility<TooltipBehaviorProps> = props => { | |||
const behaviorData = tooltipAsDescriptionBehavior(props); | |||
const defaultAriaLabeledBy = getDefaultAriaLabelledBy(props); | |||
const triggerAriaLabel = props.triggerAriaLabel || props['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.
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.
if there is a aria-label on the tooltip, it should just stay on the tooltip and should not be applied on the trigger I think
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.
I am now considering aria-label from trigger when I am conditionally applying aria-labelledby.
As for that classname, I fixed it by removing the classname and using another selector in the test. should be fine now
<Ref innerRef={triggerRef}>{React.cloneElement(triggerElement, getA11Props('trigger', triggerProps))}</Ref> | ||
<Ref innerRef={triggerRef}> | ||
{React.cloneElement(triggerElement, { | ||
className: Tooltip.slotClassNames.trigger, |
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.
Let's avoid doing this as it will silently override existing className
on a trigger
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.
So what do you suggest? Doing a cx
composition of classes for that? I need to get that triggerElement in tests so I need the slot class.
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.
cx
can work, but in this case it should be done also in Popup
. However, I don't like this option as trigger
is not a real slot.
If it's only for tests I don't see any blocker to use Button.className
or use a specific classname on it.
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.
removed, using button class now.
…rigger (microsoft#12555) * include trigger aria-label to behavior props * send trigger aria-label to behavior * fix behavior to not add aria-labelledby * add unit tests * changelog * do not consider Tooltip aria-label for this fix * remove trigger slot classname * change changelog message
Pull request checklist
$ yarn change
Description of changes
Checks for
aria-labelled
in either theroot
props ortrigger
props. If found in any, will not addaria-labelledby
ontrigger
.Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow