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

feat(ngAria[ngClick]): anchor should treat keypress as click #13996

Closed
wants to merge 8 commits into from
19 changes: 19 additions & 0 deletions src/ngAria/aria.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')) {
Copy link
Member

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).

elem.on('keypress', function(event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'd be better off with keydown here (and elsewhere, actually)

Copy link
Member

@gkalpak gkalpak Feb 16, 2016

Choose a reason for hiding this comment

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

Indeed switching to keydown seems more consistent with how native buttons work.
Yet, I suggest we make the change from keypress to keydown in another PR (since it's used to several places) and possibly tests.

Also, does it mean that bindKeypress should be renamed bindKeydown (which is a breaking change) ?
Leaving the name bindKeypress but binding to keydown isn't ideal either 😃
Maybe we could deprecate bindKeypress in favor of bindKeydown and remove it in v1.6.x...

Copy link
Member

Choose a reason for hiding this comment

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

I opened #14063 to track this.

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like callback is never used inside this code path.

fn(scope, { $event: event });
}
});
}
}
};
}
};
Expand Down
32 changes: 32 additions & 0 deletions test/ngAria/ariaSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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>');
Copy link
Contributor

Choose a reason for hiding this comment

The 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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do people really use ng-href and ng-click together? 👀

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() {
Expand Down