Skip to content

fix(tooltip): tooltip sample not working with keyboard navigation. #15111

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

Merged
merged 1 commit into from
Mar 21, 2019

Conversation

MartinKamenov
Copy link
Contributor

@MartinKamenov MartinKamenov commented Feb 7, 2019

Tooltip showing/hiding in sample when buttons are focused.
Fixes: #15044

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 7, 2019
@jelbourn
Copy link
Member

jelbourn commented Feb 7, 2019

I think a more appropriate behavior here would be to show the tooltip on focus, since that's roughly the same as mouseenter for keyboard.

@jelbourn jelbourn added docs This issue is related to documentation pr: merge safe target: patch This PR is targeted for the next patch release labels Feb 7, 2019
@MartinKamenov MartinKamenov force-pushed the keyboard-tooltip-sample branch from e8a74bb to c77fd22 Compare February 8, 2019 08:13
@MartinKamenov
Copy link
Contributor Author

MartinKamenov commented Feb 8, 2019

@jelbourn Thank you for the feedback.
I made some changes. Feel free to check them out.

@kdinev
Copy link

kdinev commented Feb 8, 2019

@jelbourn @MartinKamenov Hooking the API call on focus was the first thing that was tried, but due to the issue being logged with a11y concerns, we thought it's better for the button to perform the action after the user has gotten feedback from the screen-reader, or another accessibility technology about what it would do. With focus the screen reader would read what the action (showing/hiding/toggling the tooltip) would be, but the action would've been performed already, while with keyup.enter the action would be performed after the assisting technology has indicated what it would be.

If the tooltip was for the button that triggers it, then focus would be right choice IMO.

@jelbourn
Copy link
Member

@kdinev ah, I think you're right. I did not originally look at how this example actually worked. It should in fact just be a click handler. Looking at it more closely, we should:

  • Change the text "Mouse over to" to its own paragraph that says "Mouse over or click the following buttons to show or hide the tooltip on the button at the end of this section."
  • Change the buttons' content to "Show tooltip", "Hide tooltip", and "Toggle tooltip"

@kdinev
Copy link

kdinev commented Feb 13, 2019

@jelbourn I'm thinking about how this would behave and it seems to me that having both mouseover and click would be fine for the toggle button, but would be weird for the show and hide buttons. The click with the mouse would just do nothing as the tooltip has already been shown/hidden on mouseover.

How about remove the mouseover all together and leave only a click handler, with the text changed to "Click the following buttons to..."?

@jelbourn
Copy link
Member

Agreed, we should just remove the mouseover behaviors entirely and make them all clicks.

@MartinKamenov MartinKamenov force-pushed the keyboard-tooltip-sample branch from 9cf5406 to c77fd22 Compare February 21, 2019 14:53
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Feb 21, 2019
<button mat-button
(mouseenter)="tooltip.show()"
(click)="tooltip.show()"
Copy link
Member

Choose a reason for hiding this comment

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

Based on the new behavior, I think the aria-label attributes should be updated to just say, e.g.
"Show tooltip on the button at the end of this section"

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Could you squash the commits in the PR? The googlebot always flags PRs with multiple authors.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Feb 26, 2019
@MartinKamenov MartinKamenov force-pushed the keyboard-tooltip-sample branch from ceffe92 to a99e601 Compare February 26, 2019 08:41
@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Mar 21, 2019
@andrewseguin andrewseguin merged commit 76c6fa5 into angular:master Mar 21, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement docs This issue is related to documentation target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Mouse over" tooltip example is not keyboard accessible
6 participants