Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(tooltip): interpolate the tooltip text when adding it to the aria-label #8170

Closed
wants to merge 1 commit into from
Closed

fix(tooltip): interpolate the tooltip text when adding it to the aria-label #8170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/components/tooltip/tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ angular
* @param {string=} md-direction Which direction would you like the tooltip to go? Supports left, right, top, and bottom. Defaults to bottom.
*/
function MdTooltipDirective($timeout, $window, $$rAF, $document, $mdUtil, $mdTheming, $rootElement,
$animate, $q) {
$animate, $q, $interpolate) {

var TOOLTIP_SHOW_DELAY = 0;
var TOOLTIP_WINDOW_EDGE_SPACE = 8;
Expand Down Expand Up @@ -150,11 +150,22 @@ function MdTooltipDirective($timeout, $window, $$rAF, $document, $mdUtil, $mdThe
element.remove();
attributeObserver && attributeObserver.disconnect();
});

// Updates the aria-label when the element text changes. This watch
// doesn't need to be set up if the element doesn't have any data
// bindings.
if (element.text().indexOf($interpolate.startSymbol()) > -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about ngMaterial's aria.js features?

Copy link
Member Author

@crisbeto crisbeto Apr 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I followed the way it was done previously, but I'll look at the Material aria provider.

scope.$watch(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Huge performance impact with large lists where each item as a tooltip.
This is why we did the mutation observer... this $watch( ) here my re-introduce perf issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, I'll try to rework it and keep this one as a fallback for the case where a mutation observer isn't supported.

Copy link
Member Author

@crisbeto crisbeto Apr 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

A question about the mutation observer: In order to get it to fire for text changes, the characterData and subtree options need to be enabled. This fires the observer roughly twice as often as before. I think this might end up being slower than the watcher because we'll be calling the function that sets aria-label about 5 times on mouseover and about 5 times again on mouseleave.

return element.text().trim();
}, addAriaLabel);
}
}

function addAriaLabel () {
if (!parent.attr('aria-label') && !parent.text().trim()) {
parent.attr('aria-label', element.text().trim());
function addAriaLabel (override) {
Copy link
Contributor

Choose a reason for hiding this comment

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

See ^^ regarding aria.js

Copy link
Member Author

Choose a reason for hiding this comment

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

$mdAria doesn't seem to be useful in this case, because it doesn't allow for the aria-label to be overwritten once it is set. I believe the expected behaviour here is that the aria-label changes together with the tooltip's text.

if ((override || !parent.attr('aria-label')) && !parent.text().trim()) {
var rawText = override || element.text().trim();
var interpolatedText = $interpolate(rawText)(parent.scope());
parent.attr('aria-label', interpolatedText);
}
}

Expand Down
30 changes: 30 additions & 0 deletions src/components/tooltip/tooltip.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,36 @@ describe('<md-tooltip> directive', function() {
expect(element.attr('aria-label')).toEqual('Tooltip');
});

it('should interpolate the aria-label', function(){
buildTooltip(
'<md-button>' +
'<md-tooltip>{{ "hello" | uppercase }}</md-tooltip>' +
'</md-button>'
);

expect(element.attr('aria-label')).toBe('HELLO');
});

it('should update the aria-label when the interpolated value changes', function(){
buildTooltip(
'<md-button>' +
'<md-tooltip>{{ testModel.ariaTest }}</md-tooltip>' +
'</md-button>'
);

$rootScope.$apply(function() {
$rootScope.testModel.ariaTest = 'test 1';
});

expect(element.attr('aria-label')).toBe('test 1');

$rootScope.$apply(function() {
$rootScope.testModel.ariaTest = 'test 2';
});

expect(element.attr('aria-label')).toBe('test 2');
});

it('should not set parent to items with no pointer events', inject(function($window){
spyOn($window, 'getComputedStyle').and.callFake(function(el) {
return { 'pointer-events': el ? 'none' : '' };
Expand Down