bug(ngAria): ng-click adds keypress event to buttons which already fire a click event on "enter"

[kentcdodds]

Originally discussed here in pull #10288

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.

Fix suggested by @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

[marcysutton]

There is more work to do before I can submit a PR for this bug, but I want to post what I have so far to hopefully get more eyes on it.

In a failing unit test, <button> behaves like <div>: it won't execute an ngClick callback when triggered from a keypress or keydown event. In actual usage, binding ngClick to button fires two callbacks: one for click, and one for the keypress event added by ngAria for custom controls. In order to fix the bug, it must be reproduced in a test. But so far I have been unable to replicate it: https://github.com/marcysutton/angular.js/compare/ngaria-ngclick-bugfix

@caitp @kentcdodds

[marcysutton]

An alternative approach would be to try and keep the callback from firing more than once, rather than excluding button or anchor nodes.

[caitp]

@marcysutton from a glance, it looks like the spy API is being used incorrectly and not calling your original function --- I don't think jasmine will use the "andCallThrough" behaviour by default.

Try changing it to spyOn(scope, 'setValue').andCallThrough() and see if that works?

[marcysutton]

Hi @caitp! I've updated it, but it still doesn't fire the callback even with the andCallThrough() method added on. The method call count is still 0.

[dylanb]

@marcysutton look at my blog post here for how screen readers act when you add ARIA roles to things

http://unobfuscated.blogspot.com/2013/05/event-handlers-and-screen-readers.html

[caitp]

@marcysutton wait ---

So the code is saying "if we are binding keypresses, there's no keypress handler, AND we aren't a button/anchor tag, then add the event listener" --- and your test is creating a button tag and asserting that the keypress handler is used.

If you're wanting to test that the browser itself is sending the right event, that's harder --- we might need to write an E2E test for that, but I would see if browserTrigger() does the job for you

[marcysutton]

@caitp yes, but the test is just asserting that the ngClick handler is executed when triggered by keypress. I thought it should trigger a callback but I guess it's the click event that's firing even when you hit the enter key. There are definitely intricacies with events here, as @dylanb's post indicates. I want to avoid user-agent hell if at all possible, but I'm starting to worry this feature may be impossible to get right in all browsers. As much as I want to fix it, it also sort-of encourages bad development practices, IMO. I'll keep researching and playing around with it, though.

[caitp]

triggerHandler doesn't really dispatch any events through the browser, it only digs up the jquery event handlers and calls them, so it's not able to test anything related to how the browser actually behaves.

Yes, the browser will send a click event after getting the keypress event, but you won't be able to test that with triggerHandler()

[marcysutton]

So I still haven't figured out how to test this in unit or end-to-end tests, but I was able to repro the original bug and see it fixed in a live demo:

http://codepen.io/marcysutton/pen/ogYNPM

The fix proposed is to only bind the additional keypress event if the element is not a <button> or <a>. Are there any other elements we should be considering for this? Here is the proposed code change: https://github.com/marcysutton/angular.js/compare/ngaria-ngclick-bugfix

If we're okay with this change going in without tests repro'ing the bug, I can submit a PR.

[caitp]

there might be other elements that need to be considered, I'm not sure. I'll look at writing a test for this after le meeting

[kentcdodds]

Thanks for your work @marcysutton. I'll look forward to upgrading when this fix is released! :D

[marcysutton]

We could also do it as an opt-in instead of an opt-out, and give people a config. Start with <div> and <li>, and let people add others. There was a request to add a better config for ngMessages: #9834 (comment)

That said, anything that becomes interactive needs to be communicated as such. An open PR for roles, which I need to revisit: #10012

[marcysutton]

@caitp any update on that test? I've updated my code to be opt-in, so it only applies the keypress to DIV elements. https://github.com/marcysutton/angular.js/compare/ngaria-ngclick-bugfix

[caitp]

@caitp any update on that test? I've updated my code to be opt-in, so it only applies the keypress to DIV elements. https://github.com/marcysutton/angular.js/compare/ngaria-ngclick-bugfix

I think I've long lost the branch I was working on for that. I'll have another go at it tomorrow, making a note to do that.

...ok, I guess I didnt do it... tomorrow for sure though :<

[caitp]

so it only applies the keypress to DIV elements. https://github.com/marcysutton/angular.js/compare/ngaria-ngclick-bugfix

Wasn't it opt-in before? based on the bindKeypress flag and the lack of a custom keypress handler. My understanding was that this was only needed for non-buttons (and one other thing?)

What are the correct semantics for this?

[caitp]

anyways, test PR sent that fails without your patch and passes with it, that should be good enough

[marcysutton]

It is opt-in in terms of the flag, but the keypress event is also bound for all elements with ng-click regardless of whether they need it (resulting in two events on button). The common thing we're trying to fix with this is ngClick on <div>, which doesn't fire keypress by default. I think we should also consider adding <li> since people love to put :hover and ng-click on those.

If the PR for adding roles goes in, it should not add role="button" to <li> since those already have semantics. <div role="button"> is pretty safe.

[marcysutton]

@caitp do you have a PR I can look at somewhere? I have submitted my code here: #10766

I added an array to the config so developers can change which elements receive keypress. I considered making a truthy $aria.config('bindKeypress') value the array itself, but to preserve backwards compatibility I opted to make a second config value that holds the array of element names: $aria.config.keypressEls : ['DIV', 'LI']. It can totally be renamed or changed if there is something that works better. The way it works now, the array will simply be ignored if bindKeypress is set to false.

[caitp]

@marcysutton I have a PR open aganst your ng-click fix branch

[caitp]

you'd have to pick out parts of it manually, it's just a suggestion to get a test in there, but it looks like you've got some now

[jorupp]

Not wiring in the keypress handling for button makes complete sense. However, from my testing, an a tag with a href attribute triggers click when using the enter key, but not when using the spacebar. Additionally, an a tag without an href attribute doesn't trigger click for enter or spacebar.

Am I testing this wrong, or should there be some handling for a tags for ng-click to ensure a more-consistent behavior?

I'm using http://jsfiddle.net/8uu4kcq9/7/ (w/ angular-aria) and http://jsfiddle.net/bh2quo7c/5/ (w/o angular-aria) as my test scenarios, specifically the 4th a tag (which has an href and ng-click).

[marcysutton]

@jorupp a valid a already has a keypress event (but actually fires a click on enter). This code is wiring up keypress events for elements that don't already have them, such as div or some-button. By filtering out a and button, we won't get duplicate events, which is the reason this bug existed.

a tags require an href to be valid links, and they weren't the goal of this feature in ngAria. We could consider adding behavior to anchors without hrefs, but it should be in a separate Github issue.

[marcysutton]

Also you should really use ng-href and routing for a elements. If you're putting ng-click on an anchor and not using href, it should probably be a button.

[dylanb]

An anchor tag with an href (even an empty one) will fire a click upon an ENTER. That is accurate and expected. Button and input (type="submit", "button" or "image") will act the same as a button (fire click on ENTER or SPACE).