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.