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

fix(list): empty aria-label attributes for list-items with interpolation #10218

Merged
merged 2 commits into from
Jan 11, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
9 changes: 6 additions & 3 deletions src/components/list/list.js
Original file line number Diff line number Diff line change
Expand Up @@ -328,11 +328,14 @@ function mdListItemDirective($mdAria, $mdConstant, $mdUtil, $timeout) {
'<md-button class="md-button-wrap-executor md-no-style">'
);

// Expect the root element to have a label set. If not set, determine the label from the text content.
$mdAria.expectWithText(tEl, 'aria-label');

copyAttributes(tEl[0], buttonWrap[0]);

// If there is no aria-label set on the button (previously copied over if present)
// we determine the label from the content and copy it to the button.
if (!buttonWrap.attr('aria-label')) {
buttonWrap.attr('aria-label', $mdAria.getText(tEl));
}

// We allow developers to specify the `md-no-focus` class, to disable the focus style
// on the button executor. Once more classes should be forwarded, we should probably make the
// class forward more generic.
Expand Down
21 changes: 18 additions & 3 deletions src/components/list/list.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -563,15 +563,30 @@ describe('mdListItem directive', function() {
expect(buttonEl.attr('aria-label')).toBe('Content');
});

it('should determine the label from the bound content if aria-label is not set', function() {
var listItem = setup(
'<md-list-item ng-click="null">' +
'<span>{{ content }}</span>' +
'<span aria-hidden="true">Hidden</span>' +
'</md-list-item>'
);

$rootScope.$apply('content = "Content"');

var buttonEl = listItem.find('button');

// The aria-label attribute should be determined from the content.
expect(buttonEl.attr('aria-label')).toBe('Content');
});

it('should warn when label is missing and content is empty', inject(function($log) {
// Clear the log stack to assert that a new warning has been added.
$log.reset();

setup('<md-list-item ng-click="null">');

// Expect $log to have two warnings. A warning for the md-list-item and a second one for the later compiled
// button executor.
expect($log.warn.logs.length).toBe(2);
// Expect $log to have one $mdAria warning, because the button misses an aria-label.
expect($log.warn.logs.length).toBe(1);
}));

});
Expand Down
24 changes: 11 additions & 13 deletions src/core/services/aria/aria.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,15 @@ angular
*/
function MdAriaProvider() {

var self = this;

/**
* Whether we should show ARIA warnings in the console, if labels are missing on the element
* By default the warnings are enabled
*/
self.showWarnings = true;
var config = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have an specs for these Provider changes ?

Copy link
Member Author

@devversion devversion Jan 5, 2017

Choose a reason for hiding this comment

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

Since the changes won't be noticeable from outside, testing will be be difficult and IMO not necessary.

/** Whether we should show ARIA warnings in the console if labels are missing on the element */
showWarnings: true
};

return {
disableWarnings: disableWarnings,
$get: function($$rAF, $log, $window, $interpolate) {
return MdAriaService.apply(self, arguments);
$get: function($$rAF, $log, $q, $window, $interpolate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you inject $q when I do not see it being used [in these new changes].

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I had that for debugging the expectAsync function.

return MdAriaService.apply(config, arguments);
}
};

Expand All @@ -51,14 +48,14 @@ function MdAriaProvider() {
* @description Disables all ARIA warnings generated by Angular Material.
*/
function disableWarnings() {
self.showWarnings = false;
config.showWarnings = false;
}
}

/*
* @ngInject
*/
function MdAriaService($$rAF, $log, $window, $interpolate) {
function MdAriaService($$rAF, $log, $q, $window, $interpolate) {

// Load the showWarnings option from the current context and store it inside of a scope variable,
// because the context will be probably lost in some function calls.
Expand All @@ -68,7 +65,8 @@ function MdAriaService($$rAF, $log, $window, $interpolate) {
expect: expect,
expectAsync: expectAsync,
expectWithText: expectWithText,
expectWithoutText: expectWithoutText
expectWithoutText: expectWithoutText,
getText: getText
};

/**
Expand Down Expand Up @@ -110,7 +108,7 @@ function MdAriaService($$rAF, $log, $window, $interpolate) {
var content = getText(element) || "";
var hasBinding = content.indexOf($interpolate.startSymbol()) > -1;

if ( hasBinding ) {
if (hasBinding) {
expectAsync(element, attrName, function() {
return getText(element);
});
Expand Down