Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

bug(ngAria): ng-click evals attrs.ngClick without passing locals #10442

Closed
kentcdodds opened this issue Dec 12, 2014 · 4 comments
Closed

bug(ngAria): ng-click evals attrs.ngClick without passing locals #10442

kentcdodds opened this issue Dec 12, 2014 · 4 comments

Comments

@kentcdodds
Copy link
Member

Here's an example of the bug. What's happening is ngAria has this line:

scope.$eval(attr.ngClick);

It should probably be:

scope.$eval(attr.ngClick, {$event: event});

Can't think of anything else that is an expected expression local that isn't on the scope, but if there are, we'd need to make sure those made it in there as well...

pinging @marcysutton on this one.
@caitp
Copy link
Contributor

caitp commented Dec 12, 2014

There are a few improvements to make for this:

  1. Yes, pass $event as a local
  2. Parse the expression before-hand, use the same security features as core ngClick (https://github.com/angular/angular.js/blob/master/src/ng/directive/ngEventDirs.js#L60)
  3. Evaluate the expression for ngDblClick too
  4. Restrict ngDblClick to attributes only

@caitp
Copy link
Contributor

caitp commented Dec 12, 2014

There are other problems:

Changes to bindings which occur during the event handler installed by aria's ngClick will not update the DOM --- because they don't cause a digest.

caitp added a commit to caitp/angular.js that referenced this issue Dec 12, 2014
@caitp
fix(ngAria): trigger digest on ng-click via keypress, pass $event to …
6c48bdd 
…expression

Minor improvement to ng-click directive from ngAria. Now, if bindings are updated
during the click handler, the DOM will be updated as well. Additionally, the $event
object is passed in to the expression via locals, as is done for core event directives.

Closes angular#10442
@caitp caitp mentioned this issue Dec 12, 2014
Closed
@gkalpak
Copy link
Member

gkalpak commented Dec 12, 2014

@caitp: I wonder if pressing enter or spacebar should execute the dblclick handler (from an aria point of view). In any case, I don't think it's a good idea to execute both.

👍 for restricting ngDblClick to attributes only.

caitp added a commit to caitp/angular.js that referenced this issue Dec 16, 2014
@caitp
fix(ngAria): trigger digest on ng-click via keypress, pass $event to …
657879d 
…expression

Minor improvement to ng-click directive from ngAria. Now, if bindings are updated
during the click handler, the DOM will be updated as well. Additionally, the $event
object is passed in to the expression via locals, as is done for core event directives.

Closes angular#10442
Closes angular#10443
Closes angular#10447
@caitp caitp closed this as completed in 924e68c Dec 16, 2014
@marcysutton
Copy link
Contributor

High-five for fixing this @caitp, the original PR was intended to be a starting point.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(ngAria): trigger digest on ng-click via keypress, pass $event to expression caitp/angular.js
4 participants
@marcysutton @kentcdodds @caitp @gkalpak