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

fix(htmlAnchorDirective): remove event listener if target is not element #10849

Closed
wants to merge 1 commit into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Jan 23, 2015

Previously, when an a tag element used a directive with a replacing template, and did not include an href or name attribute before linkage, the anchor directive would always prevent default.

Now, the anchor directive will cancel its event listener if the linked element is not the same as the target element of the event.

Closes #4262

Previously, when an `a` tag element used a directive with a replacing template, and did not include an `href` or `name` attribute
before linkage, the anchor directive would always prevent default.

Now, the anchor directive will cancel its event listener if the linked element is not the same as the target element of the event.

Closes angular#4262
@caitp
Copy link
Contributor Author

caitp commented Jan 23, 2015

self-reviewed, minimal perf impact, doesn't seem to introduce any new issues

caitp added a commit that referenced this pull request Jan 23, 2015
…e event if target is different element

Previously, when an `a` tag element used a directive with a replacing template, and did not include an `href` or `name` attribute
before linkage, the anchor directive would always prevent default.

Now, the anchor directive will not register an event listener at all if the original directive is replaced with a non-anchor, and
will ignore events which do not target the linked element.

Closes #4262
Closes #10849
@caitp caitp closed this in b146af1 Jan 23, 2015
@b1rdex
Copy link

b1rdex commented Jan 26, 2015

This causes regression: previously <a href="" ng-click="doSomething()">test</a> just called doSomething() and click was prevented.
Now if I click such link, doSomething() is called, but click event is not prevented for <a>.

@caitp
Copy link
Contributor Author

caitp commented Jan 26, 2015

please produce a reproduction

@b1rdex
Copy link

b1rdex commented Jan 26, 2015

Can't include all my project code, but here is link example.

<div class="actions">
        <a href="" class="remover" ng-click="removeAttachment(attachment)"><span class="glyphicon glyphicon-remove"></span></a>
      </div>
$scope.removeAttachment = (attachment) ->
      log 'remove called'
      index = $scope.attachments.indexOf(attachment)
      link = $scope.attachments[index]
      $scope.attachments.splice index, 1
      link.canceler.resolve "aborted"

      $http.get(Data.storage.delete + link.id);

      link = null
      log 'remove completed'

      return false

@caitp
Copy link
Contributor Author

caitp commented Jan 26, 2015

so your issue is that the target element of the click is not the link itself, it's the span/icon. i'll try to produce a fix for that.

// SVGAElement does not use the href attribute, but rather the 'xlinkHref' attribute.
var href = toString.call(element.prop('href')) === '[object SVGAnimatedString]' ?
'xlink:href' : 'href';
element.on('click', function(event) {
// If a different element was clicked, ignore it.
if (element[0] !== event.target) return;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line caused regression. preventDefault() is below and not called anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code hasn't been released yet, but thanks for testing out the cutting edge, i'll push a fix tonight

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but just to make sure, you aren't using "cutting edge" in production right? that would be a bad idea.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I'm not using edge. Just tested my PR
#7995 to be working after rebase.

2015-01-26 14:37 GMT+10:00 ⭐caitp⭐ [email protected]:

In src/ng/directive/a.js
#10849 (comment):

     // SVGAElement does not use the href attribute, but rather the 'xlinkHref' attribute.
     var href = toString.call(element.prop('href')) === '[object SVGAnimatedString]' ?
                'xlink:href' : 'href';
     element.on('click', function(event) {
  •      // If a different element was clicked, ignore it.
    
  •      if (element[0] !== event.target) return;
    

but just to make sure, you aren't using "cutting edge" in production
right? that would be a bad idea.


Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/10849/files#r23512554.

Пашин Анатолий,
эникейщик.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken anchor link when directive replaces its markup
3 participants