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

Regression from 1.2.0rc1: ng-include 'bugs' href/ng-href in an anchor element #3793

Closed
sdenier opened this issue Aug 29, 2013 · 7 comments
Closed

Comments

@sdenier
Copy link

sdenier commented Aug 29, 2013

When I put a ng-include directive directly inside an a element, I get the following error in the console:
TypeError: Object #<Comment> has no method 'setAttribute'

For example:

      <a ng-href="#/{{href}}" ng-include="'link.html'"></a>
      <a href="#/{{href}}" ng-include="'link.html'"></a>

In the first case the element is not displayed as a link (as href is not set).

See Plunker http://plnkr.co/edit/LWa2asKqAxUYBtZL23YE?p=preview

At first I thought I was dealing with ng-include issues #3584 or #3573 . However, after testing this issue does not appear in 1.2.0rc1 and previous releases.

@sdenier
Copy link
Author

sdenier commented Aug 29, 2013

I forget to tell. A basic workaround is to put ng-include on a child element, or directly as an element of the anchor:

<a ng-href="#/{{href}}">
  <span ng-include="'link.html'"></span>
</a>

@jankuca
Copy link
Contributor

jankuca commented Aug 30, 2013

This is caused by ngInclude replacing the original element with a placeholder comment node and cloning the original element after that. If the included content is later replaced, a new copy of the original is created and placed there while the previous copy is removed. This, I believe, makes it possible to define transitions (animations) between the old and the new content.

@IgorMinar
Copy link
Contributor

this should be easy to fix. since ngInclude is terminal, we should check if that could be used to prevent the attribute interpolation directive getting processed prematurely by tweaking priorities.

@IgorMinar
Copy link
Contributor

@mhevery @matsko do you have another suggestion?

@matsko
Copy link
Contributor

matsko commented Sep 4, 2013

I'm starting to think about reverting ngInclude back to what is was and possibly introducing a new feature where you can style the newly created container element for animation purposes. What do you think?

Regarding this, terminal should be fine. @sdenier maybe test this out using ngView or ngRepeat first (since they both use terminal already)...

@sdenier
Copy link
Author

sdenier commented Sep 5, 2013

Indeed, it works when I put a ng-repeat instead of the ng-include in the anchor element

http://plnkr.co/edit/nejaV3tZSqEpMkMM5YBr?p=preview

@btford
Copy link
Contributor

btford commented Sep 20, 2013

I submitted a PR, following @IgorMinar's suggestion.

btford added a commit to btford/angular.js that referenced this issue Sep 20, 2013
IgorMinar pushed a commit to IgorMinar/angular.js that referenced this issue Sep 20, 2013
@btford btford closed this as completed in 5eb1fb6 Sep 20, 2013
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Sep 25, 2013
BREAKING CHANGE: ngInclude's priority is now set to 1000

It's quite rare for anyone to depend on explicity directive priority,
but if a custom directive that needs to run before ngInclude exists,
it should have its priority checked and adjusted if needed.

Closes angular#3793
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
BREAKING CHANGE: ngInclude's priority is now set to 1000

It's quite rare for anyone to depend on explicity directive priority,
but if a custom directive that needs to run before ngInclude exists,
it should have its priority checked and adjusted if needed.

Closes angular#3793
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.