bug(ngAria): native controls fire a single click event #10766

marcysutton · 2015-01-15T20:18:52Z

To fix the widespread accessibility issue of ngClick on divs, ngAria dynamically binds keypress in addition to tabIndex. As first implemented, this introduced a bug where <button> elements would have both events bound, causing multiple callbacks to fire. This PR makes limits the addition of keypress to <div> and <li>, which can be overridden with the bindKeypress config option.

Closes #10388. Thanks to @caitp for the help & unit test on native elements.

gkalpak · 2015-01-16T08:07:56Z

src/ngAria/aria.js

@@ -83,7 +83,8 @@ function $AriaProvider() {
    ariaMultiline: true,
    ariaValue: true,
    tabindex: true,
-    bindKeypress: true
+    bindKeypress: true,
+    keypressEls: ['DIV', 'LI']


Rather trivial and probably a matter of personal style, but I would prefer a non- abbreviated name (keypressElements, keypressNodeNames etc).

I like keypressNodeNames. I've changed it to that. Thanks for the suggestion!

marcysutton · 2015-01-20T23:58:31Z

@caitp this is ready for another review.

caitp · 2015-01-21T00:34:31Z

test/ngAria/ariaSpec.js

@@ -526,6 +538,31 @@ describe('$aria', function() {
      element.triggerHandler({ type: 'keypress', keyCode: 13 });
      expect(element.text()).toBe('keypress13');
    });
+
+    it('should not bind keypress to non-div elements', function() {


Might be good to note in the spec title that this is a symptom of using the default config. (This might be spelled out clearly higher up, can't tell on mobile view)

Good call. I've updated it.

caitp · 2015-01-21T00:35:00Z

Still LGTM

caitp · 2015-01-23T17:00:27Z

src/ngAria/aria.js

@@ -83,7 +83,8 @@ function $AriaProvider() {
    ariaMultiline: true,
    ariaValue: true,
    tabindex: true,
-    bindKeypress: true
+    bindKeypress: true,
+    keypressNodeNames: ['DIV', 'LI']


I still kind of feel like this is a really weird API. Chances are, you're going to want emulated clicks for any element that doesn't produce them on their own, right? if this array gets out there and people start using it, we won't be able to get rid of it later. Lets make sure this is needed before doing it

Fair point. <button> is the biggest issue we had, since it gets both click and keypress. <a> should not be used with ng-click (that's what buttons are for) but rather ng-href and routing...although I suppose that could be abused. There's also the <details>/<summary> elements, <dialog>, form controls, and a few more. An interesting thing to consider, as well, is the work being done on Custom Elements and tabindex: https://docs.google.com/document/d/1k93Ez6yNSyWQDtGjdJJqTBPmljk9l2WS3JTe5OHHB50/edit#heading=h.tf9twftfr65h

So, we need some mechanism of checking whether or not to apply keypress–I'm personally in favor of opt-in for <div> and <li> so that it limits the scope of this behavior. It's just a question of whether we want that list to be configurable.

configurable is good, but I think we shouldn't expose an array for it at least.

If we decide we don't want it to be configurable and just "do the right thing always" later, it's trivial to replace configuration methods with noop, and not have anything unnecessary taking up space in the framework. So I see that as being preferable, tbh

Something like

// enable keypress on all elements! $ariaProvider.keypressClickElements('*');

Would that allow all elements by default? How would we discern between <div> and <button> (and fix the issue this is trying to address) out of the box?

@caitp revisiting this so we can get a fix in to the next release. I'm still unclear how we would fix the bug with a wildcard for all elements.

wildcard case would just not filter, or only blacklist known-issue elements, I guess.

Maybe it's not worth worrying about it and just land it, @lgalfaso what do you think?

What if we just do away with the config for elements and limit dynamic binding of keypress to div and li? We could wait and see if anyone actually needs it to be configurable. You'd still be able to disable it with the bindKeypress flag.

Closes angular#10388

marcysutton · 2015-02-06T23:36:57Z

@caitp I simplified the code to just Do the Right Thing™ without a configuration array. I also updated the docs.

caitp · 2015-02-06T23:41:50Z

okay, i think a blacklist would be better than a whitelist, but this is fine for now... @petebacondarwin / @Narretz / @lgalfaso could you merge this before monday?

petebacondarwin · 2015-02-07T07:34:29Z

Will do

Closes #10388 Closes #10766

marcysutton · 2015-02-07T22:39:14Z

Thanks @petebacondarwin!

AshutoshAm · 2016-11-11T10:55:05Z

try this .. its 100% works for me ...

.config(function( $mdGestureProvider ) {
$mdGestureProvider.skipClickHijack();
})

source..
https://rubberchickin.com/issue-with-ng-click-on-ios-firing-twice/

marcysutton · 2016-11-11T17:03:43Z

@Ashutosh7376 that is unrelated to this change, and only applicable to Angular Material.

googlebot added the cla: yes label Jan 15, 2015

marcysutton mentioned this pull request Jan 15, 2015

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

Closed

marcysutton force-pushed the ngaria-ngclick-bugfix branch from 3f251ba to ec66e16 Compare January 15, 2015 20:29

caitp added the type: bug label Jan 16, 2015

gkalpak reviewed Jan 16, 2015
View reviewed changes

marcysutton force-pushed the ngaria-ngclick-bugfix branch from ec66e16 to 35504fa Compare January 19, 2015 20:44

caitp reviewed Jan 21, 2015
View reviewed changes

marcysutton force-pushed the ngaria-ngclick-bugfix branch from 35504fa to 93dd331 Compare January 21, 2015 00:36

caitp reviewed Jan 23, 2015
View reviewed changes

caitp added the component: ngAria label Jan 23, 2015

caitp added this to the 1.4.x milestone Jan 23, 2015

caitp self-assigned this Jan 23, 2015

marcysutton mentioned this pull request Jan 28, 2015

md-button with ng-click - Behavior when invoked from keyboard angular/material#1326

Closed

marcysutton force-pushed the ngaria-ngclick-bugfix branch 2 times, most recently from 0e4f397 to eea9805 Compare February 6, 2015 23:32

bug(ngAria): native controls fire a single click

5dcb824

Closes angular#10388

marcysutton force-pushed the ngaria-ngclick-bugfix branch from eea9805 to 5dcb824 Compare February 6, 2015 23:34

petebacondarwin assigned petebacondarwin and unassigned caitp Feb 7, 2015

petebacondarwin closed this Feb 7, 2015

petebacondarwin pushed a commit that referenced this pull request Feb 7, 2015

fix(ngAria): ensure native controls fire a single click

9d53e5a

Closes #10388 Closes #10766

petebacondarwin pushed a commit that referenced this pull request Feb 7, 2015

fix(ngAria): ensure native controls fire a single click

69ee593

Closes #10388 Closes #10766

a14m mentioned this pull request Mar 27, 2015

ngClick using ngTouch fires twice #6251

Closed

marcysutton deleted the ngaria-ngclick-bugfix branch April 11, 2015 21:15

marcysutton mentioned this pull request Jul 2, 2015

fix(ngAria): clean up tabindex usage #12262

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug(ngAria): native controls fire a single click event #10766

bug(ngAria): native controls fire a single click event #10766

marcysutton commented Jan 15, 2015

gkalpak Jan 16, 2015

marcysutton Jan 19, 2015

marcysutton commented Jan 20, 2015

caitp Jan 21, 2015

marcysutton Jan 21, 2015

caitp commented Jan 21, 2015

caitp Jan 23, 2015

marcysutton Jan 23, 2015

caitp Jan 23, 2015

caitp Jan 23, 2015

marcysutton Jan 24, 2015

marcysutton Feb 3, 2015

caitp Feb 3, 2015

marcysutton Feb 6, 2015

marcysutton commented Feb 6, 2015

caitp commented Feb 6, 2015

petebacondarwin commented Feb 7, 2015

marcysutton commented Feb 7, 2015

AshutoshAm commented Nov 11, 2016

marcysutton commented Nov 11, 2016

bug(ngAria): native controls fire a single click event #10766

bug(ngAria): native controls fire a single click event #10766

Conversation

marcysutton commented Jan 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton commented Jan 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caitp commented Jan 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton commented Feb 6, 2015

caitp commented Feb 6, 2015

petebacondarwin commented Feb 7, 2015

marcysutton commented Feb 7, 2015

AshutoshAm commented Nov 11, 2016

marcysutton commented Nov 11, 2016