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

Commit

Permalink
perf(a): do not link when href or name exists in template
Browse files Browse the repository at this point in the history
Change the a directive to link and hookup a click event only when
there is no href or name in the template element.
In a large Google app, this results in about 800 fewer registrations,
saving a small but measurable amount of time and memory.

Closes #5362
  • Loading branch information
kseamon authored and IgorMinar committed Dec 13, 2013
1 parent fcd2a81 commit f3de5b6
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 8 deletions.
18 changes: 10 additions & 8 deletions src/ng/directive/a.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,15 @@ var htmlAnchorDirective = valueFn({
element.append(document.createComment('IE fix'));
}

return function(scope, element) {
element.on('click', function(event){
// if we have no href url, then don't navigate anywhere.
if (!element.attr('href')) {
event.preventDefault();
}
});
};
if (!attr.href && !attr.name) {
return function(scope, element) {
element.on('click', function(event){
// if we have no href url, then don't navigate anywhere.
if (!element.attr('href')) {
event.preventDefault();
}
});
};
}
}
});
26 changes: 26 additions & 0 deletions test/ng/directive/aSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,30 @@ describe('a', function() {

expect(element.text()).toBe('hello@you');
});


it('should not link and hookup an event if href is present at compile', function() {
var jq = jQuery || jqLite;
element = jq('<a href="//a.com">hello@you</a>');
var linker = $compile(element);

spyOn(jq.prototype, 'on');

linker($rootScope);

expect(jq.prototype.on).not.toHaveBeenCalled();
});


it('should not link and hookup an event if name is present at compile', function() {
var jq = jQuery || jqLite;
element = jq('<a name="bobby">hello@you</a>');
var linker = $compile(element);

spyOn(jq.prototype, 'on');

linker($rootScope);

expect(jq.prototype.on).not.toHaveBeenCalled();
});
});

4 comments on commit f3de5b6

@petebacondarwin
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this change actually breaks an e2e test that we have in ngHref: https://github.com/angular/angular.js/blob/master/src/ng/directive/booleanAttrs.js#L72-L76

But it doesn't get caught by the protractor run because the old docs embeds this example and blocks clicks from propagating:
https://github.com/angular/angular.js/blob/master/docs/components/angular-bootstrap/bootstrap-prettify.js#L235

In the new docs, we are running these examples in iframes and so they do not have their click events hijacked. This makes the test above fail.

Either we modify this performance refactoring to allow this test to pass or we remove this aspect from the example. @IgorMinar and @kseamon what shall it be?

@petebacondarwin
Copy link
Member

Choose a reason for hiding this comment

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

Here is a runnable example of the problem: http://ci.angularjs.org/job/angular.js-pete/421/artifact/build/docs/examples/example-example4/index.html
Clicking on "anchor (link, don't reload)" does do a reload.

@rajeshsegu
Copy link

Choose a reason for hiding this comment

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

We at MobileIron are also seeing the same issue. As per W3Schools http://www.w3schools.com/tags/att_a_name.asp "name" is not supported by anchor tag, why consider it ?

<a href="" name="xyz" ng-click="value=4"></a> reloads the page. We got to fix it.

@petebacondarwin
Copy link
Member

Choose a reason for hiding this comment

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

See #6554 and #9214

Please sign in to comment.