-
Notifications
You must be signed in to change notification settings - Fork 27.4k
feat(ngAria[ngClick]): anchor should treat keypress as click #13996
Changes from all commits
0ee5a4a
9a54562
9d2148f
d7b0eb2
368899f
827845b
56d66b5
7fd46db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -377,6 +377,25 @@ ngAriaModule.directive('ngShow', ['$aria', function($aria) { | |
}); | ||
} | ||
} | ||
|
||
if (elem[0].nodeName === 'A') { | ||
if ($aria.config('bindRoleForClick') && !attr.href && !attr.xlinkHref && !attr.role) { | ||
elem.attr('role', 'link'); | ||
} | ||
if ($aria.config('bindKeypress')) { | ||
elem.on('keypress', function(event) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we'd be better off with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed switching to Also, does it mean that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I opened #14063 to track this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @gkalpak, that can definitely happen in a separate PR. |
||
var keyCode = event.which || event.keyCode; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section looks an awful lot like https://github.com/LeeAdcock/angular.js/blob/ngclick-accessible-11053/src/ngAria/aria.js#L367 above. I wonder if we could combine those two sections somehow, perhaps with a reusable function? |
||
var hasHref = attr.href || attr.xlinkHref; | ||
if ((keyCode === 32 || keyCode === 13) && !hasHref && !attr.ngKeypress) { | ||
scope.$apply(callback); | ||
} | ||
|
||
function callback() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like |
||
fn(scope, { $event: event }); | ||
} | ||
}); | ||
} | ||
} | ||
}; | ||
} | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -755,6 +755,38 @@ describe('$aria', function() { | |
element.triggerHandler({ type: 'keypress', keyCode: 13 }); | ||
expect(element.text()).toBe(''); | ||
}); | ||
|
||
it('should apply role to anchor elements without href ', function() { | ||
compileElement('<a ng-click="event = $event">{{event.type}}{{event.keyCode}}</a>'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why wouldn't you use a button element for this? I don't understand the use case. |
||
expect(element.attr('role')).toBe('link'); | ||
}); | ||
|
||
it('should bind keypress to anchor elements when href is falsy', function() { | ||
compileElement('<a ng-click="event = $event">{{event.type}}{{event.keyCode}}</a>'); | ||
expect(element.text()).toBe(''); | ||
element.triggerHandler({ type: 'keypress', keyCode: 32 }); | ||
expect(element.text()).toBe('keypress32'); | ||
element.triggerHandler({ type: 'keypress', keyCode: 13 }); | ||
expect(element.text()).toBe('keypress13'); | ||
}); | ||
|
||
it('should not bind keypress to anchor elements when keypress is not falsy', function() { | ||
compileElement('<a ng-keypress="event = null" ngclick="event = $event">{{event.type}}{{event.keyCode}}</a>'); | ||
expect(element.text()).toBe(''); | ||
element.triggerHandler({ type: 'keypress', keyCode: 32 }); | ||
expect(element.text()).toBe(''); | ||
element.triggerHandler({ type: 'keypress', keyCode: 13 }); | ||
expect(element.text()).toBe(''); | ||
}); | ||
|
||
it('should not bind keypress to anchor elements when href is not falsy', function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do people really use |
||
compileElement('<a href="http://somwehere" ng-click="event = $event">{{event.type}}{{event.keyCode}}</a>'); | ||
expect(element.text()).toBe(''); | ||
element.triggerHandler({ type: 'keypress', keyCode: 32 }); | ||
expect(element.text()).toBe(''); | ||
element.triggerHandler({ type: 'keypress', keyCode: 13 }); | ||
expect(element.text()).toBe(''); | ||
}); | ||
}); | ||
|
||
describe('actions when bindRoleForClick is set to false', function() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid unnecessary work, we should check for
!attr.ngKeypress
here (instead of inside the listener below).