feat(ngAria): Bind keypress on ng-click w/ optional flag

[marcysutton]

This PR addresses a widespread accessibility issue: when ng-click is bound to non-interactive elements such as <div> or <some-button>, keyboard events are not fired by default even though ngAria dynamically adds tabindex="0". With this change, keypress events are dynamically bound alongside ng-click as long as ngAria's bindKeypress option is true and ng-keypress doesn't already exist.

This change overrides native DOM behavior in regards to click on custom controls like <div tabIndex="0"> (i.e. they don't fire from the keyboard). However, since ngAria adds tabindex by default it should further close the loop by eliminating the need to manually bind ng-keypress, since this is likely to be forgotten.

This is intended to be a starting point–there may be a better way to accomplish this. It should also be brought up that this implementation only interfaces with ng-keypress, not ng-keyup or ng-keydown.

Related to #9254

[googlebot]

CLAs look good, thanks!

[caitp]

Looks good to me --- there are some style bugs you'll want to fix first (you can check that the style is okay using grunt ci-checks locally) --- I think the E2E failure was a flake.

[marcysutton]

Sweet! I was seeing some memory leak complaints from the testabilityPatch util, but they were definitely pre-existing.

[caitp]

@petebacondarwin I think we can get this in today, sgty?

[caitp]

@marcysutton a good followup commit might be adding a bit more documentation to this --- there are some notes about it, but that whole section feels a bit cryptic. Maybe a passage about it in the accessibility guide?

[marcysutton]

@caitp sure, I can do that! Stay tuned for a PR.

[kentcdodds]

Huzzah! Thanks @marcysutton. Time to swap out my az-action with ng-click :-)

[kentcdodds]

Does bindKeypress default to true?

[marcysutton]

@kentcdodds yes it does!

[kentcdodds]

Hmmmm. Looks like this introduces a bug? Looks like this is a problem in Chrome, Firefox, and Safari. Tab to the button (turns red) and hit "Enter" and the alert pops up twice. This is because (I believe) that the aforementioned browsers will fire a "clicked" event when the "enter" key is pressed.

[marcysutton]

Ahh, I see that. Good catch! It should only bind on a custom control that doesn't already have keyboard support. The approach used for adding roles in this PR would work: https://github.com/angular/angular.js/pull/10318/files#diff-91e9129201746ba107348c9a0a7735edR333

[kentcdodds]

👍

[marcysutton]

@kentcdodds can you open a Github issue to track it?

[brandonburkett]

Is this part of ng-aria now? It would be awesome if the ng-keypress would auto-bind to anchors without an href.

[marcysutton]

@devourment77 This PR has been merged, yes. There is definitely something to fix for anchors without hrefs, good catch....but it may require more than just the keypress for assistive technology support.

[brandonburkett]

Thank you for the update. I could put href="" and it works, but I feel that an anchor w/o an href should do the same thing (trigger the keypress event).

[marcysutton]

"Should" is debatable. Native anchors without hrefs do not take focus by default. See this demo: http://codepen.io/marcysutton/pen/azNZrr

I plan to add to this demo so I can test all combinations of role, href, etc. The framework should support developers so their sites are more accessible by default. The flip-side of this is it encourages bad UI-development practices (e.g. adding tabIndex="0", etc. to <div ng-click="">). I still need to do more research on this particular issue, but if we can make Angular sites more accessible by solving common problems, it's often worth deviating from native DOM behavior.