feat(TooltipDefinition): support different elements as tooltip trigger

[emyarod]

Closes #3985

This PR allows users to specify the element for the definition tooltip trigger button instead of hard coding it to be a button element

Changelog

New

• renderTrigger prop which takes a function and becomes the output element for the trigger button

Testing / Reviewing

Ensure the renderTrigger prop functions as expected

[netlify]

Deploy preview for the-carbon-components ready!

Built with commit 89de514

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

[netlify]

Deploy preview for carbon-elements ready!

Built with commit 89de514

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

[netlify]

Deploy preview for carbon-components-react ready!

Built with commit 89de514

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

[netlify]

Deploy preview for the-carbon-components ready!

Built with commit b41ce29

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

[netlify]

Deploy preview for carbon-components-react ready!

Built with commit b41ce29

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

[netlify]

Deploy preview for carbon-elements ready!

Built with commit b41ce29

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

[asudoh]

Good for flexibility @emyarod! To go further on that, I'd go for renderTrigger approach instead of as approach.

[dakahn]

Why do we want this? I looked at the referenced issue that references a closed PR and I'm still not sure.

Can you elaborate on why we'd want to do this?

Also worth noting in your PR description (to make reviewing and testing simpler) is that it's not any element. It's a button, div, and span.

[emyarod]

Can you elaborate on why we'd want to do this?

to remove the restriction on our React definition tooltip where the trigger element is a button

Also worth noting in your PR description (to make reviewing and testing simpler) is that it's not any element. It's a button, div, and span.

not sure what you mean by this since renderTrigger provides flexibility beyond the elements you listed

[dakahn]

aria-describedby is intended for interactive elements and will be ignored by NVDA, VoiceOver when applied to divs and spans. This would need an interactive role of some kind to function as intended. The implicit role we get with button covered us here, but now that we're allowing non-semantic markup maybe a role="tooltip" would make up the difference. I'd need to test further once the changes were made.

https://developer.paciellogroup.com/blog/2017/07/short-note-on-aria-label-aria-labelledby-and-aria-describedby/
https://www.w3.org/TR/using-aria/#label-support

Also worth noting is when using aria-describedby in Internet Explorer. MS requires the referenced element (via ID) be a Microsoft Accessible Element. Essentially it needs to have proper roles etc.

https://docs.microsoft.com/en-us/previous-versions/windows/internet-explorer/ie-developer/accessibility/gg701963(v=vs.85)?redirectedfrom=MSDN#Accessible_HTML_Elements

NOTE: this comment also applies to every other instance of this pattern throughout the rest of the PR. I'll keep it to one comment to avoid clutter/noise.

[dakahn]

@emyarod

to remove the restriction on our React definition tooltip where the trigger element is a button

Why?

[dakahn]

To me, we need a firm reason why we'd want to subvert the established pattern (tooltips thrown by buttons) and "added flexibility" is not good enough considering the increased complexity and accessibility concerns introduced.

[emyarod]

@dakahn it's not our intention to make trigger elements <button>s an established pattern as this is a restriction found only in the React implementation. we've architected the tooltip styles and vanilla tooltip behavior in such a way that they can be applied to different elements as needed

I'm not sure where the added concerns about complexity and accessibility come from since the default behavior is unchanged, and we can't account for modifications by the user. but this PR removes a restriction on our definition tooltips which was found only in the React implementation. does that clarify things?

[emyarod]

@joshblack just to clarify, the default behavior is not changing and the trigger element will still be a <button>. I think since we are dealing with definition tooltips there is a legitimate use case for using other interactive HTML elements as triggers like anchors which we can also see in the example provided by @dakahn

if the concern is about the render prop providing too much flexibility we can revert to using the as prop as I had in this PR initially. if there are concerns with the storybook examples that can also be modified to better illustrate the feature, but I think this feature still has its merits and does not modify the default behavior of the component in any way

[joshblack]

@emyarod all I’m trying to understand are what use-cases or problems are solved by this diff. I don’t think I understand yet what utility we gain for allowing flexible tags and naively it looks like an easy way to mess up the control (like in the storybook) in a way that seemingly is allowed by the library through the proposed changes.

[emyarod]

@joshblack the utility of this feature lies in allowing different elements launch tooltips, like links or other inputs. otherwise in those scenarios you end up with nested interactive elements and only the parent element launching the tooltip. in terms of implementation and documentation, if we are concerned about render props then we can revert to the as prop as I had initially

[joshblack]

@emyarod what kind of markup do these situations end up having? (roughly)

[emyarod]

@joshblack an example would be a tooltip attached to a link. currently with our React component it would be wrapped by a button creating 2 tab stops

<button {...tooltipClasses}>
  <a {...linkClasses}>Link</a>
</button>

the tooltip would also not be visible when the link itself is focused (the second tab stop), it would only be visible when the button is focused (the first tab stop)

[joshblack]

@emyarod how do the proposed changes change the markup you shared for the as-is?

[emyarod]

@joshblack prior to the latest PR commit you would be able to set the element for the tooltip trigger with the as prop, and currently with the render prop you would be able to customize the tooltip trigger even further as simply swapping the button for an anchor for instance may not be sufficient

[joshblack]

@emyarod it would help out a ton if you could share the full markup of what you're envisioning the example you shared will become, that's what is tripping me up right now.

The reason I ask is that changing the markup seems like it might not help as we are still dealing with two interactive nodes. If we change the tag to something non-interactive, then the tooltip itself will break due to either focus or aria-labelledby not working on non-interactive nodes.

If you have some examples to show how this could work, that would help me understand 👍

[emyarod]

@joshblack with the render prop (or alternatively the as prop) you would be able to make the trigger element the sole interactive node

for example the output of using <TooltipDefinition> would go from

<div class="bx--tooltip">
  <button class="bx--tooltip--trigger">
    <a href="/">Tooltip trigger text</a>
  </button>
  <div class="bx--assistive-text">Tooltip content</div>
</div>

where a button is always the trigger element to

<div class="bx--tooltip">
  <a class="bx--tooltip--trigger" href="/">Tooltip trigger text</a>
  <div class="bx--assistive-text">Tooltip content</div>
</div>

if the issue is with the demo code as I mentioned it can be revised to better illustrate the feature, but currently users are restricted to <button>s as the tooltip trigger element and the render prop would provide a way for the user to modify the trigger element

[joshblack]

Perfect, thanks for helping me understand! 🙏

I'm totally down if we limit the alternative tag to an <a> with an href prop, but don't think we should leave it unbounded. Would you agree @dakahn?

[joshblack]

As an aside, should we just have a general purpose tooltip component that works for the tags defined in: https://developer.paciellogroup.com/blog/2017/07/short-note-on-aria-label-aria-labelledby-and-aria-describedby/ ?

Surprised we'd be using definition tooltip as that seems specifically for defining an item or term

[emyarod]

we should probably reevaluate the 3 separate tooltips for the next release since the definition tooltip seems to be used as a general purpose tooltip when icon tooltips are too limited and when it is not desirable to bring in the normal interactive tooltip due to the lack of alignment options

[joshblack]

Agreed, @dakahn has some thoughts on this too where we have true tooltips and then non-modal dialogues which are toggle-able tooltips, or just toggletips.

[emyarod]

so do we want to revert to the as prop like I had in 74105d8?

[vpicone]

By being opinionated about the element used we can ensure the definition is accessible through keyboard navigation. If you set the element knob to div in the storybook the definition is now inaccessible to keyboard users.

This seems like a bit of a footgun if we're relying on developers to use elements that are focusable.

[emyarod]

@vpicone the examples can be modified after the implementation details have been nailed down. so it sounds like the as approach that we had would be more controllable than the latest render prop approach then?

[dakahn]

@emyarod to Vince's point can we limit the scope of acceptable triggering elements? Specifically if they're not traditional "interactable" elements we're gonna need to get tricky with our aria. Happy to kick the tires whenever we get some examples up though 👍

There is the other issue of our DefinitionTooltips being togglable (persistent on click) as a requirement. Meaning having a link as the triggering element isn't going to work, right?

[joshblack]

What is the end goal of this change? Is it to be used with the read-only input PR as the style for the tooltip? If that's the case, I think we need to circle back with design to see if that's the intended usage for TooltipDefinition

[emyarod]

it would be easier to limit the acceptable trigger elements if we revert back to the as prop which I had prior to the latest commit. our tooltips are activated by hover and focus, so they would function the same as they do currently in the definition and icon tooltip component

read only input is dependent on #4016 and not this PR. the end goal of this PR is to reduce the number of tab stops interactive/keyboard focusable elements (like links) launching tooltips. we can also include a guideline that while it is possible to make elements focusable with tabindex, it's recommended to use normal interactive elements as tooltip triggers

[vpicone]

the end goal of this PR is to reduce the number of tab stops interactive/keyboard focusable elements (like links) launching tooltips

Wouldn't this be creating a different experience for keyboard users vs mouse?

[emyarod]

not sure what you mean by this since the number of tab stops will not affect mouse users

[dakahn]

Hey -- sorry, was the work requested completed? What's the status of this PR?

[emyarod]

closing as this doesn't seem to be a use case we want to deal with