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

[Tooltip] Make interactive default #22382

Merged
merged 5 commits into from
Oct 6, 2020
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Aug 27, 2020

Breaking changes

  • [Tooltip] Tooltips are now interactive by default.

    The previous default behavior failed success criterion 1.4.3 ("hoverable") in WCAG 2.1.
    To reflect the new default value, the prop was renamed to disableInteractive.
    If you want to restore the old behavior (thus not reaching level AA), you can apply the following diff:

    -<Tooltip>
    +<Tooltip disableInteractive>
    # Interactive tooltips no longer need the `interactive` prop.
    -<Tooltip interactive>
    +<Tooltip>

Preview: https://deploy-preview-22382--material-ui.netlify.app/components/tooltips/#main-content

With #22376 ("dismissable") and this PR ("hoverable") our Tooltip now no longer violates success criterion 1.4.13 in WCAG 2.1.

I removed the prop entirely. Let's wait for feedback during alpha before adding something like disableInteractive. This would allow making inaccessible sites with props which I don't want to allow. Though there may be good reasons to do so.

https://www.w3.org/WAI/WCAG21/Understanding/content-on-hover-or-focus.html has more context on why tooltips should be hoverable.

@eps1lon eps1lon added accessibility a11y component: tooltip This is the name of the generic UI component, not the React module! labels Aug 27, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Aug 27, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 717c973

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Exception: The visual presentation of the additional content is controlled by the user agent and is not modified by the author.

How is a tooltip not in this exception? Isn't the component meant to implement a customized version of a native title that isn't ugly?

Why do most custom tooltips don't follow this guideline even if less accessible (subjective)? I think that it's because it's disruptive and surprising. Consider this end-user flow: "I want to click on the action behind the tooltip, why is this tooltip covering it? Why is it still visible? Why would I need to copy the text inside the tooltip?"

At the minimum, I think that it should be configurable. At best, I don't think that we should change it and document this aspect.


Related discussion: reach/reach-ui#439. Maybe we should do that?

all components will be audited by WebAIM prior to our 1.0 release and we'll get clarity on our approach to tooltip at that time.

Related discussion: twbs/bootstrap#28411

Edit: this makes me think about the cursor: pointer thread, once everybody stop following the accessibility recommendations, it's probably no longer relevant: creating more frustration than intuitive UIs.

@eps1lon
Copy link
Member Author

eps1lon commented Aug 27, 2020

How is a tooltip not in this exception? Isn't the component meant to implement a customized version of a native title that isn't ugly?

The tooltip is not implemented by the user agent but author.

Edit: The statements in the reach-ui issue are outdated. WCAG 2.1 has been finalized for 2 years now.

@eps1lon
Copy link
Member Author

eps1lon commented Aug 27, 2020

I think that we should optimize what product teams

If they want to build harmful UIs then I don't want to help them. If they have good reasons for different behavior the alpha is the perfect time to ask them.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 27, 2020

If they want to build harmful UIs then I don't want to help them. If they have good reasons for different behavior the alpha is the perfect time to ask them.

It's a solid counter-argument to my previous message, we can't really know without trying it out (experimenting).

The tooltip is not implemented by the user agent but author.

I was using the perspective: the intent is to reproduce user-agent behavior, it's implemented by the author. At the end of the day intent > implementation.

@eps1lon
Copy link
Member Author

eps1lon commented Aug 27, 2020

It's a solid counter-argument to my previous message, we can't really know without experimenting with it.

I didn't claim I knew. You were the one asking rhetorical questions to which no one has an answer. The only constructive proposal is this PR: It's alpha time and our Tooltip is clearly violating 1.4.3. Let's experiment with a variant that doesn't. That's really not as controversial as you act it out. Especially since you seemed to have responded to this PR without reading the linked resources. They pretty clearly explain why title does not work this way and were 1.4.3 does apply: "custom tooltips". The harsh reaction is especially confusing since you asked none of these questions in #22376.

@eps1lon
Copy link
Member Author

eps1lon commented Aug 27, 2020

I was using the perspective: the intent is to reproduce user-agent behavior,

Why would that be the intent? The title attribute is a pretty awful attribute in all browsers I know and it's commonly accepted that it's insufficient just like the <select> element. Why would we want to re-implement? Might as well use the platform in these cases. The goal is to provider better primitives than what user agents currently offer. Otherwise we don't have a goal.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 27, 2020

Maybe I should have been more factual.

WCAG21 support the need for this behavior with two use cases:

When the added content is large, magnified views may mean that the user needs to scroll or pan to completely view it, which is impossible unless the user is able to move their pointer off the trigger without the additional content disappearing.

The tooltip is meant to display a short message, we always update the position of the tooltip to avoid scrolling. The first use case doesn't seem to apply.

the pointer can obscure a significant area of the additional content.

What about the 14 pixels margin + 3 pixels padding that is added + the size of the element the tooltip wraps, isn't that enough to solve the problem? So unless the cursor takes more than 45x45 pixels it should be fine (default cursor is 18x10 pixels on MacOS) (with the default MD theme).


Now, the specification doesn't mention the hidden cost of changing the behavior. What about the surprise it creates when you want to interact with an element that is hidden by the tooltip? e.g. on Gmail moving the cursor to reply all:

Capture d’écran 2020-08-27 à 23 06 31

This problem is worse in our case because we have a wider gap (14px instead of 0px) between the anchor element and the tooltip than Gmail.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 29, 2020

I have tried to collect more feedback on https://twitter.com/olivtassinari/status/1299493764768108544. How about?

  • we make interactive the default (experiment, hopefully, will go through to v5 stable) as initially proposed in the pull request
  • we add a prop to disable this option (the point I was trying to make)
  • we document the accessibility pros & cons of each option

@mbrookes
Copy link
Member

image

No strong opinion. I'm not sure I understand the practical value in applying success criterion 1.4.3 in WCAG 2.1 to a tooltip, but it's "mostly harmless".

However the behaviour where the hover indicator disappears when the mouse moves off the item the tooltip is attached to, but the tooltip stays open "feels" like a bug in use:

image

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 4, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Sep 25, 2020

In general I don't think it's very valuable to discuss the merits of WCAG criterions. There are definitely choices I would question but discussing these here is a bit "easy" so to speak since the original authors of these texts can't respond.

As far as I can tell the immediately actionable decision we can make is whether we support level A, AA (the level mostly cited in laws) or AAA by default. Starting from that we can make a decision how "easy" it should be to violate particular success criterion. Though I think we agree that it should be as easy as possible to reach a certain level.

Deciding on a level is useful to document accessibility of the library. I know that most libraries just say "a11y in mind" or "a11y as 1st class citizen" but these statements don't hold any facts. And without them you can't be held accountable which I think is important for building trust with users. Or at least document for each component the default level so that devs can choose a subset that fits their needs. Stating what parts are not accessible in one way or another is usually part of a11y statements

From these decisions the Tooltip evolves in one of three ways (SC 1.4.3 is synonymous with level AA, interactive naming is considered separately):

  1. We do nothing (reach level A, meeting 1.4.3 is trivial with interactive={true})
  2. We make interactive the default (reach Level AA, violating 1.4.3 is trivial with interactive={false}
  3. We make interactive the default and remove it (reach Level AA, violating 1.4.3 requires custom pointer handlers)

Point 2. and 3. are different with regard to how difficult we want violating 1.4.3 to be.

I'm not sure I understand the practical value in applying success criterion 1.4.3 in WCAG 2.1 to a tooltip, but it's "mostly harmless"

As far as I understood it it's about screen magnifier usage. Depending on the size of the tooltip and its distance to the original object you might not be able to magnify the tooltip content because the magnified view is restricted to the immediate surroundings of the pointer.

But again, I don't think there's much value in discussion whether you or I find this success criterion practical.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 25, 2020

@eps1lon This is a great summary!

  • I think that all the components should make it possible to reach AAA. They shouldn't be a blocker in the implementation that prevents it. For instance, I believe our color contrast reaches AA, but it's possible to customize the colors to reach AAA.
  • I think that all the components should come with, at least, level A by default. This seems to be an obvious bare minimum we should provide. We won't be taken seriously without.

I can tell the immediately actionable decision we can make is whether we support level

Regarding what should be the default level all the components should provide. Do we have enough data points to make this decision now? How about we set AAA as a target and in the journey of reaching it, we reevaluate?

We would start by tackling all the A levels, then AA, and finally AAA, in this order. Along the way, I imagine we will hit roadblocks, like this very AA 1.4.3 criteria on the tooltip.

Regarding the different options available. I would vote for 2.

Why?

@eps1lon
Copy link
Member Author

eps1lon commented Sep 25, 2020

AA is mostly interesting because it's what's required by most a11y related laws. I think mid-term this should be our goal for almost all core components. Not meeting AA should be the exception and include a rationale.

AAA is not recommended as a general goal for sites anyway:

It is not recommended that Level AAA conformance be required as a general policy for entire sites because it is not possible to satisfy all Level AAA Success Criteria for some content.

-- https://www.w3.org/TR/WCAG21/#cc1

I think the same can apply to component libraries. I wouldn't bother with it for now though definitely keep an eye out. We'll do this incidentally anyway since WCAG isn't structured by components anyway.

Regarding what should be the default level all the components should provide. Do we have enough data points to make this decision now?

What would data tell us here? If you mean your benchmarking methodology then I don't share this sentiment. What other component libraries are doing interests me little compared to why they are doing things. And I'm fairly certain most don't put in the effort due to lack of time or motivation.

The only data I have is that AA is recommended by WCAG itself and required by law.

With regards to Tooltip: I lean towards changing the default and renaming it to disableInteractive. For packed dashboard-like apps you can always disable it via theming. That particular success criterion is probably a minor a11y issues in these kind of apps.

@eps1lon
Copy link
Member Author

eps1lon commented Sep 25, 2020

However the behaviour where the hover indicator disappears when the mouse moves off the item the tooltip is attached to, but the tooltip stays open "feels" like a bug in use:

@mbrookes How did you reproduce this behavior? What is the "hover indicator" in that context?

@oliviertassinari
Copy link
Member

What would data tell us here?

@eps1lon I was thinking of trying the changes to understand the tradeoffs, #22382 is one data point. We have tried to reach AA, we see what we have to give up in exchange.

With regards to Tooltip: I lean towards changing the default and renaming it to disableInteractive. For packed dashboard-like apps you can always disable it via theming. That particular success criterion is probably a minor a11y issues in these kind of apps.

It sounds good.

@mbrookes
Copy link
Member

mbrookes commented Sep 25, 2020

@eps1lon

AA is recommended by WCAG itself and required by law.

Some qualification needed there I think.

However the behaviour where the hover indicator disappears when the mouse moves off the item the tooltip is attached to, but the tooltip stays open "feels" like a bug in use:

@mbrookes How did you reproduce this behavior?

Move the mouse off the item the tooltip is attached to (in the direction of the tooltip)

What is the "hover indicator" in that context?

image

@eps1lon
Copy link
Member Author

eps1lon commented Sep 25, 2020

Some qualification needed there I think.

Do you find it useful to get a list of countries that list that requirement? I've only seen level AA but never A or AAA. That's the important part here. Not what countries those are. Or is there a list of countries where you would only consider this important?

What is the "hover indicator" in that context?

You mean the box-shadow of the icon?

@eps1lon
Copy link
Member Author

eps1lon commented Sep 25, 2020

Just in case you mean the displayed text: The whole intention of this PR is to keep to tooltip open when the mouse is moved towards it. The PR is specifically about the "hoverable" part in 1.4.13:

If pointer hover can trigger the additional content, then the pointer can be moved over the additional content without the additional content disappearing;

Or do you mean we should implement behavior where the additional content is hidden if the mouse rests on the space in between? I think that would go against the intention of 1.4.13, pretty hard to implement and apply only to a few user intentions.

@mbrookes
Copy link
Member

I've only seen level AA but never A or AAA. That's the important part here.

Noted.

You mean the box-shadow of the icon?

Yes, in this particular case.

Just in case you mean the displayed text

I didn't.

Or do you mean we should implement behavior where the additional content is hidden if the mouse rests on the space in between?

I wasn't proposing a fix, just pointing out a perceived issue with having a disembodied tooltip.

@eps1lon
Copy link
Member Author

eps1lon commented Sep 28, 2020

You mean the box-shadow of the icon?

Yes, in this particular case.

~@mbrookes What browser/OS/device are you using? I can't reproduce this with Chrome 85/Firefox 80 on Ubuntu desktop: ~

So you're saying this is a bug? I wouldn't see this as one. If it's not hovered then we don't display the hover indicator. This was always the case.

recording of moving the pointer from the child to the tooltip showing that the hover indicator disappears

@mbrookes
Copy link
Member

If it's not hovered then we don't display the hover indicator. This was always the case.

Yes, I wasn't suggesting that it's a regression, rather that the new behavior of tooltip staying open when the mouse moves towards it while the focus indicator of the associated component disappears, "feels" like a bug in use. The tooltip is left without a visible "owner".

Consider for example a menu where the parent item retains its focus indicator when the mouse moves to the menu, but not when otherwise leaving the menu:

image

@eps1lon
Copy link
Member Author

eps1lon commented Sep 29, 2020

I don't see how we're ever able to implement this behavior. Since this isn't a regression can we keep this as an open issue? A possible workaround would be to use <Tooltip arrow /> which indicates the relationship.

@oliviertassinari
Copy link
Member

@eps1lon It sounds like we can move forward, #22382 (comment) addresses the last concern.

@eps1lon eps1lon force-pushed the fix/Tooltip/hoverable branch from 08fb0a9 to 62246d1 Compare October 5, 2020 08:55
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 5, 2020
@@ -19,7 +19,7 @@ export interface TooltipProps extends StandardProps<React.HTMLAttributes<HTMLDiv
classes?: {
/** Styles applied to the Popper component. */
popper?: string;
/** Styles applied to the Popper component if `interactive={true}`. */
/** Styles applied to the Popper component unless `disableInteractive={true}`. */
Copy link
Member

Choose a reason for hiding this comment

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

We have used the following pattern in the other components:

Suggested change
/** Styles applied to the Popper component unless `disableInteractive={true}`. */
/** Styles applied to the Popper component if `disableInteractive={false}`. */

Should we stick with it? cc @mbrookes

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is that you'll almost never see someBooleanProp={false} with our API design philosophy.

It's also incomplete since the styles are also applied without any code. My approach is covering false and undefined while your approach only covers false.

Copy link
Member

Choose a reason for hiding this comment

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

I personally prefer "unless true" over "if false".

@eps1lon
Copy link
Member Author

eps1lon commented Oct 6, 2020

Ok let's get this in and wait for angry reports from tooltip-heavy UIs.

@eps1lon eps1lon closed this Oct 6, 2020
@eps1lon eps1lon deleted the fix/Tooltip/hoverable branch October 6, 2020 07:09
@eps1lon eps1lon restored the fix/Tooltip/hoverable branch October 6, 2020 07:09
@eps1lon eps1lon reopened this Oct 6, 2020
@eps1lon eps1lon closed this Oct 6, 2020
@eps1lon eps1lon deleted the fix/Tooltip/hoverable branch October 6, 2020 07:09
@eps1lon eps1lon restored the fix/Tooltip/hoverable branch October 6, 2020 07:09
@eps1lon eps1lon reopened this Oct 6, 2020
@eps1lon eps1lon merged commit 67554dc into mui:next Oct 6, 2020
@eps1lon eps1lon deleted the fix/Tooltip/hoverable branch October 6, 2020 07:09
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Oct 17, 2020
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Oct 17, 2020
@qnkhuat
Copy link

qnkhuat commented Apr 22, 2021

Hi guys, I noticed this is not updated on the document.
Please update it or point me to how do I change the document, I'll create a PR for this ^^.
Thank you so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y breaking change component: tooltip This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants