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

regression: ngHref - #6273

Closed
petebacondarwin opened this issue Feb 15, 2014 · 4 comments
Closed

regression: ngHref - #6273

petebacondarwin opened this issue Feb 15, 2014 · 4 comments

Comments

@petebacondarwin
Copy link
Contributor

This commit (f3de5b6) caused an old e2e test to fail (See angular/dgeni#44).

See https://github.com/angular/angular.js/blob/master/src/ng/directive/booleanAttrs.js#L72

<a id="link-4" href="" name="xx" ng-click="value = 4">anchor</a> (link, don't reload)<br />
        it('should execute ng-click but not reload when href empty string and name specified', function() {
          element(by.id('link-4')).click();
          expect(element(by.model('value')).getAttribute('value')).toEqual('4');
          expect(element(by.id('link-4')).getAttribute('href')).toBe('');
        });

Previously, if you had a name but no href it would still prevent the click from propagating. The performance refactor commit, referenced above, changed the a directive so that if there is neither href or name then the click will propagate and so clicking the link would cause the page to reload.

We need to decide if this previous functionality is important and if so modify the a directive accordingly and potentially re-activate the protractor test that was broken. If not, then we should just remove the failing protractor test altogether.

FAO: @IgorMinar and @kseamon

@ashleygwilliams ashleygwilliams added this to the Backlog milestone Feb 22, 2014
@IgorMinar IgorMinar self-assigned this Mar 17, 2014
@IgorMinar IgorMinar modified the milestones: 1.3.0-beta.3, Backlog Mar 17, 2014
@btford btford modified the milestones: 1.3.0-beta.4, 1.3.0-beta.3 Mar 24, 2014
@jeffbcross jeffbcross modified the milestones: 1.3.0-beta.6, 1.3.0-beta.5 Apr 3, 2014
@IgorMinar IgorMinar modified the milestones: 1.3.0-beta.7, 1.3.0-beta.6 Apr 22, 2014
@caitp caitp modified the milestones: 1.3.0-beta.9, 1.3.0-beta.8 May 9, 2014
@IgorMinar IgorMinar modified the milestones: 1.3.0, 1.3.0-beta.9 May 12, 2014
@btford btford removed the gh: issue label Aug 20, 2014
@btford btford modified the milestones: 1.3.0-rc.5, 1.3.0 Sep 30, 2014
@IgorMinar
Copy link
Contributor

@petebacondarwin would checking !(href in attrs) instead of !attrs.href solve the issue?

@petebacondarwin
Copy link
Contributor Author

@IgorMinar - I don't believe this would solve the problem. As soon as you have either non-empty href or name attributes on the element then the a directive does not create a click event handler to prevent the default action when clicked. In this case, it is the name attribute that is doing it not the href.

So I think the real issue is that we don't want to reload is if there is an ngClick event on the anchor, right? If there is just a name or href and no ngClick then it should act as a link and reload.

If so then the obvious solution is to call preventDefault() in the ngClick handler. You can see this works here: http://plnkr.co/edit/GVCeQpwUCia38gyzaBMe?p=preview

Now this is a simple workaround (by creating an additional ngClick directive) that people could apply, and so should be documented. But is it good enough that it should be applied to the ngClick directive by default?

Also are there other scenarios that this doesn't fix?

href name ngClick reload?
no no no no
yes no no yes
no yes no no
yes yes no yes
no no yes no
yes no yes no
no yes yes no
yes yes yes no

@IgorMinar IgorMinar modified the milestones: 1.3.0-rc.6, 1.3.0 Oct 13, 2014
@tbosch tbosch modified the milestones: 1.3.x, 1.3.1, Backlog Oct 15, 2014
@petebacondarwin petebacondarwin modified the milestones: 1.3.x, Backlog Nov 10, 2014
@petebacondarwin petebacondarwin modified the milestones: 1.3.9, 1.3.x Dec 17, 2014
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Jan 27, 2015
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Jan 27, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.