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

feat(Tooltip): add tooltip accessibility #1012 #1025

Merged
merged 16 commits into from
May 22, 2018

Conversation

RockinRonE
Copy link
Contributor

This adds shows a tooltip on focus and hides it when out of focus. Also adds the ability to close tooltip using the escape key. Adds role='tooltip' to the tooltip.

src/Tooltip.js Outdated
@@ -156,6 +168,11 @@ class Tooltip extends React.Component {
addTargetEvents() {
this._target.addEventListener('mouseover', this.onMouseOverTooltip, true);
this._target.addEventListener('mouseout', this.onMouseLeaveTooltip, true);
this._target.addEventListener('keydown', this.onEscKeyDown, true);
this._target.addEventListener('focusin', this.handleFocus, true);
Copy link
Member

Choose a reason for hiding this comment

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

Because this calls the same method as focusout which just blindly toggles the tooltip, it can get into a bad state where focusin hides the toggle and focusout shows it. We probably want to explicitly show on focusin and hide on focusout
tooltip-focus

Also, this is more evident when using esc to close the tooltip and then tabbing away.
tooltip-esc

@RockinRonE
Copy link
Contributor Author

@TheSharpieOne Thanks for the explanation!

Copy link
Member

@TheSharpieOne TheSharpieOne left a comment

Choose a reason for hiding this comment

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

Look good enough to merge. I'll probably change the focus handles to call show/hide functions directly and remove the explicit functions, but that is not a show stopper.

@RockinRonE
Copy link
Contributor Author

@TheSharpieOne Thanks! This is my first code contribution to open source. I encourage you to critique my PRs as I want to improve. Mind tagging me when you change the handles so I can see what you did?

@TheSharpieOne TheSharpieOne merged commit a2138a8 into reactstrap:master May 22, 2018
@TheSharpieOne
Copy link
Member

@RockinRonE 98dfa03

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

Successfully merging this pull request may close these issues.

None yet

2 participants