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
Closed

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0ee5a4a
feat(ngAria[ngClick]): ngAria should make ng-click accessible for anc…
LeeAdcock Feb 10, 2016
9a54562
feat(ngAria[ngClick]): accessibility for space and enter keypress
LeeAdcock Feb 10, 2016
9d2148f
feat(ngAria[ngClick]): button, anchor should treat keypress as click
LeeAdcock Feb 12, 2016
d7b0eb2
feat(ngAria[ngClick]): fix javascript code style issue
LeeAdcock Feb 12, 2016
368899f
feat(ngAria[ngClick]): bind to enter keypres
LeeAdcock Feb 13, 2016
827845b
feat(ngAria[ngClick]): aria should trigger click on anchor keypress
LeeAdcock Feb 13, 2016
56d66b5
feat(ngAria[ngClick]): anchor should treat keypress as click
LeeAdcock Feb 18, 2016
7fd46db
fix(ngAria): correct code style issue
LeeAdcock Feb 18, 2016
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: 19 additions & 0 deletions src/ngAria/aria.js
View file
Open in desktop
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 •
edited
Loading

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);
Copy link
Member

Choose a reason for hiding this comment

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

This is not exactly what we are doing in the built-in htmlAnchorDirective (in core) and there's a very slight chance this will not do the expected thing. This bug is so unlikely to bite anyone though, that I'm fine leaving it as is (for the sake of simplicity).

if ((keyCode === 32 || (keyCode === 13 && !hasHref)) && !attr.ngKeypress) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should do anything (regardless the button clicked), if hasHref is truthy. What we are trying to do here is to "fix" accessibility on apps that (for whatever reason) are trying to use <a> elements as buttons (in which case they shouldn't provide an href value).

If someone applies both href and ngClick, I'm afraid they're on their own. Besides, different browsers handle the situation differently (e.g. IIRC Chrome first changes the URL and then executes the click handler and Firefox does the opposite (or it could be the other way around)).

ngAria should not be into that business. All we should be concerned with here is making <a ng-click="...">... behave like a "real" button wrt keyboard input.

elem[0].click();
Copy link
Member

Choose a reason for hiding this comment

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

I'm torn about this. I think I slightly lean towards keeping things consistent among all non-button elements (e.g. <div> or <li>). Not sure...

Copy link
Member

Choose a reason for hiding this comment

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

@LeeAdcock, what did you have in mind about this change ? In what way is this preferrable to just calling fn ?
BTW, if this ends up sticking around, then the callback below should be removed (as mentioned by @campersau).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was aiming to make the behavior the same for <a> elements across the varying types of input. So regardless if the user clicks with the mouse, or uses the keyboard or screen reader to press SPACE or ENTER, the behavior should be the same. If someone is using fn on anchor tags with the hrefs defined, then the two keys behave differently - one follows the link, while the other executes the ng-click and stops. Using .click resolves this inconsistency. I guess I leave it to you to tell me if this is a scenario we are not concerned with.

Perhaps as another feature enhancement, ngAria should be applying behavior to ng-href to ensure anchor keyboard input is consistent for SPACE and ENTER.

Copy link
Member

Choose a reason for hiding this comment

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

Taking #13996 (comment) into consideration, we should only concern ourselves with <a> without href/xlinkHref, which would remove the ambiguity.

And then, the only difference would be that fn(...) would not trigger other click handlers on the element, while .click() would. And since (1) the user has already specified ngClick on the element and (2) it's good to be consistent with other non-button elements (e.g. <div> or <li>), I think it's better to use fn(...).

}

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
28 changes: 28 additions & 0 deletions test/ngAria/ariaSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,34 @@ describe('$aria', function() {
element.triggerHandler({ type: 'keypress', keyCode: 13 });
expect(element.text()).toBe('');
});

it('should bind keypress to anchor elements when href is falsy', 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()).toContain('click');
});

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 ENTER keypress to anchor elements when href is not falsy', function() {
compileElement('<a href="http://somwehere" ng-click="event = $event">{{event.type}}{{event.keyCode}}</a>');
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