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

Commit

Permalink
fix(button): fix usages with ngDisabled
Browse files Browse the repository at this point in the history 
add unit tests

Closes #1074.
  • Loading branch information
@ThomasBurleson
ThomasBurleson committed Jan 10, 2015
1 parent 95ab29a commit 416079b
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 31 deletions.
12 changes: 5 additions & 7 deletions src/components/button/button.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,9 @@ function MdButtonDirective($mdInkRipple, $mdTheming, $mdAria) {
}

function getTemplate(element, attr) {
if (isAnchor(attr)) {
return '<a class="md-button" ng-transclude></a>';
} else {
return '<button class="md-button" ng-transclude></button>';
}
return isAnchor(attr) ?
'<a class="md-button" ng-transclude></a>' :
'<button class="md-button" ng-transclude></button>';
}

function postLink(scope, element, attr) {
Expand All @@ -78,8 +76,8 @@ function MdButtonDirective($mdInkRipple, $mdTheming, $mdAria) {

// For anchor elements, we have to set tabindex manually when the
// element is disabled
if (isAnchor(attr)) {
scope.$watch(function() {return attr.ngDisabled;}, function(isDisabled) {
if (isAnchor(attr) && angular.isDefined(attr.ngDisabled) ) {

This comment has been minimized.

Sign in to view

Copy link
@gkalpak

gkalpak Jan 10, 2015

Member

Not sure if this is an issue, but previously the watcher would run at least once and set tabindex to 0 (if ngDisabled wasn't present). With the modification, it will not set the tabindex at all if ngDisabled is not present.

(Not sure if this is an issue though, or if it is taken care of somewhere else. Maybe @marcysutton has some insight ?)

This comment has been minimized.

Sign in to view

Copy link
@ThomasBurleson

ThomasBurleson Jan 10, 2015

Author Contributor

@gkalpak - I deliberately attempted to reduce the number of .$watch( ) usages if ngDisabled was not used. I did not think, however, about the default tabindex value.

@marcysutton - do we need to set/assign a default tabindex for anchors without ngDisabled?

This comment has been minimized.

Sign in to view

Copy link
@marcysutton

marcysutton Jan 10, 2015

Contributor

Not anymore, because they are natively interactive <a> and <button> elements. If it was <md-button role="button">, the answer would be yes. ngAria usually adds tabindex to anything with ngClick but I think it would be removed with this change. Not a worry with native elements, though.

This comment has been minimized.

Sign in to view

Copy link
@ThomasBurleson

ThomasBurleson Jan 10, 2015

Author Contributor

So what if it is not an anchor with only <md-button role="button">, then we should inject tabindex="0" right ?

This comment has been minimized.

Sign in to view

Copy link
@marcysutton

marcysutton Jan 10, 2015

Contributor

That is correct. Does that ever occur? I thought we replaced the directive with the template code above.
scope.$watch(attr.ngDisabled, function(isDisabled) {
element.attr('tabindex', isDisabled ? -1 : 0);
});
}
Expand Down
89 changes: 65 additions & 24 deletions src/components/button/button.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,20 @@ describe('md-button', function() {
beforeEach(TestUtil.mockRaf);
beforeEach(module('material.components.button'));

it('should be anchor if href attr', inject(function($compile, $rootScope) {
var button = $compile('<md-button href="/link">')($rootScope.$new());
it('should convert attributes on an md-button to attributes on the generated button', inject(function($compile, $rootScope) {
var button = $compile('<md-button hide hide-sm></md-button>')($rootScope);
$rootScope.$apply();
expect(button[0].tagName.toLowerCase()).toEqual('a');
expect(button[0].hasAttribute('hide')).toBe(true);
expect(button[0].hasAttribute('hide-sm')).toBe(true);
}));

it('should be anchor if ng-href attr', inject(function($compile, $rootScope) {
var button = $compile('<md-button ng-href="/link">')($rootScope.$new());
$rootScope.$apply();
expect(button[0].tagName.toLowerCase()).toEqual('a');
it('should only have one ripple container when a custom ripple color is set', inject(function ($compile, $rootScope, $timeout) {
var button = $compile('<md-button md-ink-ripple="#f00">button</md-button>')($rootScope);
var scope = button.eq(0).scope();
scope._onInput({ isFirst: true, eventType: Hammer.INPUT_START, center: { x: 0, y: 0 } });
expect(button[0].getElementsByClassName('md-ripple-container').length).toBe(1);
}));

it('should be button otherwise', inject(function($compile, $rootScope) {
var button = $compile('<md-button>')($rootScope.$new());
$rootScope.$apply();
expect(button[0].tagName.toLowerCase()).toEqual('button');
}));

it('should expect an aria-label if element has no text', inject(function($compile, $rootScope, $log) {
spyOn($log, 'warn');
Expand All @@ -33,18 +30,62 @@ describe('md-button', function() {
expect($log.warn).not.toHaveBeenCalled();
}));

it('should convert attributes on an md-button to attributes on the generated button', inject(function($compile, $rootScope) {
var button = $compile('<md-button hide hide-sm></md-button>')($rootScope);
$rootScope.$apply();
expect(button[0].hasAttribute('hide')).toBe(true);
expect(button[0].hasAttribute('hide-sm')).toBe(true);
}));

it('should only have one ripple container when a custom ripple color is set', inject(function ($compile, $rootScope, $timeout) {
var button = $compile('<md-button md-ink-ripple="#f00">button</md-button>')($rootScope);
var scope = button.eq(0).scope();
scope._onInput({ isFirst: true, eventType: Hammer.INPUT_START, center: { x: 0, y: 0 } });
expect(button[0].getElementsByClassName('md-ripple-container').length).toBe(1);
}));
describe('with href or ng-href', function() {

it('should be anchor if href attr', inject(function($compile, $rootScope) {
var button = $compile('<md-button href="/link">')($rootScope.$new());
$rootScope.$apply();
expect(button[0].tagName.toLowerCase()).toEqual('a');
}));

it('should be anchor if ng-href attr', inject(function($compile, $rootScope) {
var button = $compile('<md-button ng-href="/link">')($rootScope.$new());
$rootScope.$apply();
expect(button[0].tagName.toLowerCase()).toEqual('a');
}));

it('should be button otherwise', inject(function($compile, $rootScope) {
var button = $compile('<md-button>')($rootScope.$new());
$rootScope.$apply();
expect(button[0].tagName.toLowerCase()).toEqual('button');
}));

});


describe('with ng-disabled', function() {

it('should not set `tabindex` when used without anchor attributes', inject(function ($compile, $rootScope, $timeout) {
var scope = angular.extend( $rootScope.$new(), { isDisabled : true } );
var button = $compile('<md-button ng-disabled="isDisabled">button</md-button>')(scope);
$rootScope.$apply();

expect(button[0].hasAttribute('tabindex')).toBe(false);
}));

it('should set `tabindex == -1` when used with href', inject(function ($compile, $rootScope, $timeout) {
var scope = angular.extend( $rootScope.$new(), { isDisabled : true } );
var button = $compile('<md-button ng-disabled="isDisabled" href="#nowhere">button</md-button>')(scope);

$rootScope.$apply();
expect(button.attr('tabindex')).toBe("-1");

$rootScope.$apply(function(){
scope.isDisabled = false;
});
expect(button.attr('tabindex')).toBe("0");

}));

it('should set `tabindex == -1` when used with ng-href', inject(function ($compile, $rootScope, $timeout) {
var scope = angular.extend( $rootScope.$new(), { isDisabled : true, url : "http://material.angularjs.org" });
var button = $compile('<md-button ng-disabled="isDisabled" ng-href="url">button</md-button>')(scope);
$rootScope.$apply();

expect(button.attr('tabindex')).toBe("-1");
}));

})

});

0 comments on commit 416079b

Please sign in to comment.