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: withTooltip accessibility #47

Merged
merged 5 commits into from
Jul 2, 2019
Merged

Conversation

jsomsanith
Copy link
Collaborator

@jsomsanith jsomsanith commented Jun 30, 2019

What is the problem this PR is trying to solve?
withTooltip component is not accessible

  1. no way to trigger the tooltip with keyboard because it’s a div
  2. the tooltip is not identified as tooltips by assistive technologies
  3. the tooltip is not associated with the trigger

What is the chosen solution to this problem?

  1. semantic
  • use button
  • buttons are problematic for svg, where we want to have a tooltip on different zone, previously mapped on <g> tags. We introduced as props, suggested by @kylesuss. It render a particular component instead of the button, adding button keyboard behavior manually in that case, and the role="button"
<svg>
      <WithTooltip as="g" trigger="hover" tooltip={<Tooltip />} aria-label="Empty box to hover for tooltip">
          <Circle cx="150" cy="50" r="50" />
      </WithTooltip>
</svg>
  • map the onMouseOver to onFocus
  • map the onMouseOut to onBlur
  1. add role="tooltip" on tooltip div

  2. trigger button has an aria-controls attribute to indicate that it opens the identified element, and aria-describedby so SR will read the tooltip content on open.

Bonus
REMOVE TOGGLE
Having 2 ways to trigger the tooltip (mouse and keyboard) revealed a problem with the state management because it was a toggle. Take this scenario:

  1. focus (keyboard): tooltip is shown
  2. mouseover: it toggles, tooltip is hidden —> weird
  3. blur (keyboard): it toggles, tooltip is shown —> weird

Instead of toggling the state value, we now really let TooltipTrigger component control it, by setting the isTooltipShown value.

MEMO
There are some function that are created at each render, creating a new reference everytime. Use React.memo to memoize them.

@kylesuss
Copy link
Collaborator

kylesuss commented Jul 1, 2019

I removed props.svg, because it’s totally ok to have a button as trigger, that contains an svg (children). Am I missing something about this prop/feature ?

@jsomsanith It is a good question. I just took a look and ultimately it looks like the places that used WithTooltip needed to know if the component was used like this:

const Rect = styled.rect`
  fill: rgb(255, 255, 0);
`;

<svg>
  <WithTooltip svg {// and a bunch more props}>
    <Rect x={left} y={top} width={width} height={height} />
  </WithTooltip>
</svg>

With that in mind, I tried applying the styles you used here to an element that was rendered that way and it doesn't seem to work. Maybe the svg needs the g tag? There is probably a discussion to be had about whether or not WithTooltip needs to know this much information, but we will have to refactor some old components if we update this. I think that's fine so long as we have a solution where we can:

  1. Use aWithTooltip inside of an svg
  2. Control the wrapper component for WithTooltip so that we can choose to render something like a g tag if needed

Maybe there is a way to just do number 1 above without number 2 using just a div as you are suggesting, though it appears a bit more might be needed based on my limited testing. Could you write a story with the component hierarchy shown above ☝️ so we can capture this use case and see what is required to support it?

Thanks & let me know if you have questions! 🙇

@jsomsanith
Copy link
Collaborator Author

jsomsanith commented Jul 1, 2019

@kylesuss I don’t think we should control the element that is rendered. And I don’t think we should use withTooltip inside an svg.
First, I changed the g/div to use a button, so it’s an action that we can trigger via keyboard. This button should have no significant style but it is the wrapper around anything you want to add a tooltip.

// text only
<WithTooptip>hover me</WithTooptip>

// svg
<WithTooptip>
    <svg>
        <g></g>
    </svg>
</WithTooptip>

// any other element
<WithTooltip>
    <div>lol</div>
</WithTooptip>

Using WithTooltip inside and svg seems wrong

<svg>
    <WithTooltip svg>
        <rect></rect>
    </WithTooptip>
</svg>

If you prefer to keep the old api for compatibility for the svg, I can reintroduce it, adding an extra <g> under the button. But that would break html, resulting with that invalid html markup:

<svg>
    <button>
        <g></g>
    </button>
</svg>

I added a story with a valid WithTooltip, wrapping an svg

Copy link
Collaborator

@kylesuss kylesuss left a comment

Choose a reason for hiding this comment

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

Amazing update. Thanks again for accommodating our svg use case 🎉

@kylesuss kylesuss merged commit 4f5c8c2 into master Jul 2, 2019
@kylesuss kylesuss deleted the jsomsanith/fix/tooltip_a11y branch July 2, 2019 21:55
@kylesuss
Copy link
Collaborator

kylesuss commented Jul 2, 2019

🚀 PR was released in v0.0.40 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants