fix(tooltip): improve accessibility #4688
Conversation
andrewseguin
commented
May 19, 2017
andrewseguin
commented
- Show on trigger focus
- Hide on trigger blur
- Hide on escape when trigger has focus
- Add aria tooltip role for tooltip
- Add aria describedby role for trigger
googlebot
added
the
cla: yes
|
Looking into the test failures, not sure why the instance isnt being removed right. |
src/lib/tooltip/tooltip.ts
Outdated
| @@ -58,7 +58,11 @@ export function throwMdTooltipInvalidPositionError(position: string) { | |||
| selector: '[md-tooltip], [mdTooltip], [mat-tooltip], [matTooltip]', | |||
| host: { | |||
| '(longpress)': 'show()', | |||
| '(focus)': 'show()', | |||
| '(blur)': 'hide(0)', | |||
| '(keydown.esc)': 'hide(0)', | |||
There was a problem hiding this comment.
Can't use keydown.xxx syntax because it's not supported in Safari 9 (Angular uses KeyboardEvent.key for this). You'll need to just handle keydown and check the keycode.
You'll also need to check whether the tooltip is actually present, and call stopPropagation if it. This is necessary in cases where you have something like a tooltip inside of a dialog; you only want to dismiss the tooltip without closing the dialog.
src/lib/tooltip/tooltip.ts
Outdated
| '(touchend)': 'hide(' + TOUCHEND_HIDE_DELAY + ')', | ||
| '[attr.aria-describedby]': '_getTooltipId()' |
There was a problem hiding this comment.
This needs to avoid clobbering an existing aria-describedby
|
|
||
| /** Returns the trigger's aria-describedby attribute. */ | ||
| private _getAriaDescribedby(): string { | ||
| return this._elementRef.nativeElement.getAttribute('aria-describedby'); |
There was a problem hiding this comment.
This one is technically safe from throwing an error in Universal, because it's after a user interaction, but it might be better if we used an @Input anyway.
There was a problem hiding this comment.
I agree except I think we need would need this anyways since we're checking if the current tooltip matches the aria-describedby and an Input@ won't capture our setter's interaction.
| flushMicrotasks | ||
| flushMicrotasks, | ||
| TestBed, | ||
| tick |
There was a problem hiding this comment.
Needs a trailing comma here and on line 14.
| @@ -1,4 +1,6 @@ | |||
| <div class="mat-tooltip" | |||
| role="tooltip" | |||
There was a problem hiding this comment.
Looking at this example of an accessible tooltip, it looks like we need to toggle aria-hidden, in addition to the role and aria-describedby.
| } | ||
|
|
||
| /** Sets the trigger's aria-describedby attribute. */ | ||
| private _setAriaDescribedBy(ariaDescribedby: string) { |
There was a problem hiding this comment.
This one could be replaced with an attribute binding.
There was a problem hiding this comment.
I had that at first but we saw that it would clobber any existing aria-describedby attribute that the user may have set. We want to only apply the tooltip's id as aria-describedby if there isn't already one set.
jelbourn
changed the title
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
