Directive fails to insert/replace html when template root element is table row (tr) #1459

Closed
phaas opened this Issue Oct 14, 2012 · 69 comments

Projects

None yet
@phaas
phaas commented Oct 14, 2012

In an attempt to automate some common table layouts I created a directive that emits the <tr> ...</tr>, including the table cells.

When this directive is applied, angular throws an exception indicating that the template does not have a single root element (it does).

When replace is set to false, this issue does not occur -- but the resulting markup will be invalid.

I suspect that the problem is partially due to the browser "cleaning up" the table HTML and moving unrelated or unknown tags outside of the table -- while preventing table tags from being inserted/replaced at the desired location.

Example

Url: http://jsfiddle.net/phaas/sL9gM/

<div ng-app="tables">
    <table>
        <row key="x" value="y"></row>
        <row key="1" value="2"></row>
        <row key="longer" value="very long!"></row>
    </table>
</div>
angular.module('tables',[])
    .directive('row', function() {
        return {
            restrict: 'E',
            replace: true,
            scope: { key: '@', value: '@' },
            template: '<tr><td>{{key}}</td><td>{{value}}</td></tr>'            
        }
    });

Stack Trace

Error: Template must have exactly one root element. was: <tr><td>{{key}}</td><td>{{value}}</td></tr>
    at applyDirectivesToNode (http://ajax.googleapis.com/ajax/libs/angularjs/1.0.2/angular.js:4010:21)
    at compileNodes (http://ajax.googleapis.com/ajax/libs/angularjs/1.0.2/angular.js:3790:14)
    at compileNodes (http://ajax.googleapis.com/ajax/libs/angularjs/1.0.2/angular.js:3795:14)
    at compile (http://ajax.googleapis.com/ajax/libs/angularjs/1.0.2/angular.js:3735:29)
    at http://ajax.googleapis.com/ajax/libs/angularjs/1.0.2/angular.js:932:9
    at Object.Scope.$eval (http://ajax.googleapis.com/ajax/libs/angularjs/1.0.2/angular.js:7808:28)
    at Object.Scope.$apply (http://ajax.googleapis.com/ajax/libs/angularjs/1.0.2/angular.js:7888:23)
    at http://ajax.googleapis.com/ajax/libs/angularjs/1.0.2/angular.js:930:13
    at Object.invoke (http://ajax.googleapis.com/ajax/libs/angularjs/1.0.2/angular.js:2788:25)
    at bootstrap (http://ajax.googleapis.com/ajax/libs/angularjs/1.0.2/angular.js:928:12) 
@phaas
phaas commented Oct 14, 2012

Workaround: Instead of replacing a non-standard element with a tr defined in the template, the directive can be applied as an attribute on an existing tr.
This avoids the exception but isn't quite as nice to read.

<div ng-app="tables">
    <table>
        <tr row key="x" value="y"></tr>
        <tr row key="1" value="2"></tr>
        <tr row key="longer" value="very long!"></tr>
    </table>
</div>​
angular.module('tables',[])
    .directive('row', function() {
        return {
            restrict: 'A',
            replace: false,
            scope: { key: '@', value: '@' },
            template: '<td>{{key}}</td><td>{{value}}</td>'            
        }
    });​

Edit: Further testing shows that this still breaks ng-transclude:

<div ng-app="tables">
    <table>
        <tr row key="Label 1"><input type="text"></input></tr>
        <tr row key="Label 2">Text</tr>
    </table>
</div>​
angular.module('tables',[])
    .directive('row', function() {
        return {
            transclude: true,
            scope: { key: '@' },
            template: '<td>{{key}}</td><td> <span ng-transclude></span>{{value}}</td>'            
        }
    });​

It appears that the browser is again moving the element content outside of the element before angular has a chance to apply the template. I'm not sure that it's even possible for angular to make this work.

Avoiding table/tr/td entirely and instead using a css-based approach may be the only feasible option here.

@phaas
phaas commented Oct 15, 2012

I'll write this one off to the browser modifying the markup (moving nodes out of the table) before it ever gets to angular.

I rewrote the page & directive using divs with display: table.. styling applied and it works fine.

@phaas phaas closed this Oct 15, 2012
@marco-m-alves

I have the same problem trying to transclude td into a tr.

Please see http://jsbin.com/otogih/9.

@marco-m-alves

And how about using this solution to ensure transclusion of content which otherwise would be removed by the browser --- if that's the case:

< directive-that-transcludes-content >
< script id="transclude-1" type="text/ng-transclude">
< !-- insert content here -- >
< /script >
< /directive-that-transcludes-content >

Angular would have to try to detect if it is transcluding a script element and then extract the html template from the script and insert it in each element.

Note: I think the id attribute in the script tag is not required

@marco-m-alves

Working solution here: http://jsbin.com/otogih/18/edit

It uses a scriptTransclude directive.

@vojtajina
Contributor

Re-opening, as I think this is still an issue.

Here's very simple example of the problem http://jsfiddle.net/A2FPR/2/

@vojtajina vojtajina reopened this Feb 14, 2013
@marco-m-alves

Here is a simpler example comparing the scriptTransclude directive and the regular ng-transclude.

It is not directly applicable to the example Vojta has shared but has similarities.

http://jsbin.com/uhocep/2/edit

@rwlogel
rwlogel commented Feb 27, 2013

I have also run into this problem trying to replace a element with in a directive with the following template:

    ...
    replace:true,
    transclude:true,
    template: '<td ng-transclude></td>

I have a potential fix for the problem. In the file "src/ng/compile.js" line 660 it does this:

    if (directive.replace) {
            $template = jqLite('<div>' +
                                 trim(directiveValue) +
                               '</div>').contents();
            compileNode = $template[0];
            if ($template.length != 1 || compileNode.nodeType !== 1) {
              throw new Error(MULTI_ROOT_TEMPLATE_ERROR + directiveValue);
            }

That is the exception that is thrown, the jqLite calls is failing because it is wrapping the td/tr with in a div the jqLite node creator thinks it's bad HTML. This is the fix I made to make work:

    if (directive.replace) {
            var trimDirective = trim(directiveValue);
            if (trimDirective.substring(0, 3) == '<tr') {
                trimDirective = '<tbody>' + trimDirective + '</tbody>';
            } else if (trimDirective.substring(0, 3) == '<td') {
                trimDirective = '<tr>' + trimDirective + '</tr>';
            } else {
                trimDirective = '<div>' + trimDirective + '</div>';
            }
            $template = jqLite(trimDirective).contents();
            compileNode = $template[0];
@stevedomin

Is this planned to be fixed at some point ?
I'm happy to look into it if no one's working on it.

@arturgspb

vote +1

@davidells

+1

The work around I've gone with for this problem is naive, but workable for my situation: A linking function to swap out the tag name...

directive("wsReplaceTag", function() {
  return function(scope, element, attrs) {
      var newTag = attrs.wsReplaceTag;
      var nodeAttributes = {};

      $.each(element[0].attributes, function(idx, attr) {
          nodeAttributes[attr.nodeName] = attr.nodeValue;
      });

      element.replaceWith(function () {
          return $("<" + newTag + "/>", nodeAttributes).append(element.contents());
      });
  };
});

And then some monkey business in the template that looks like this (ws-lister is creating a table that ends up with a <tr ng-transclude> element)...

<div ws-lister>

  <div ws-replace-tag="td">
    Name: {{item.name}}
  </div>

  <div ws-replace-tag="td">
    Email: {{item.email}}
  </div>

</div>
@rweng
rweng commented Aug 5, 2013

+1

@esvit esvit referenced this issue in esvit/ng-table Aug 8, 2013
Closed

Remove unnecessary dependence on jQuery #56

@pauljoey

I have encountered the same issue in 1.15.

As suggested, I had to replace the "replace: true" approach by using an attribute tag instead. This worked.

<tr my-row></tr>
@KurtRMueller

Yep, running into this problem right now. I've tried both writing the directive as an element as well as an attribute.

@pelme
Contributor
pelme commented Aug 17, 2013

I just managed to fix this for <tr> tags, special cases will be needed for at least thead, tbody, tfoot, th, td too, since none of those can be wrapped in a div.

I will continue work on this tomorrow and hopefully have a working pull request ready that covers all those tags.

@pelme pelme added a commit to pelme/angular.js that referenced this issue Aug 18, 2013
@pelme pelme fix($compile): properly handle directive replace for table elements
To work around limitations in jqLite, the template snippet is wrapped in
a <div> element. This does not work for table elements, since they
cannot be constructed inside a div. This patch takes special to properly
wrap caption, thead, tbody, tfoot, tr, th and td in tags that allow them
to be created.

Closes #1459
984dc4b
@bcronje
bcronje commented Oct 15, 2013

Any idea if/when pelme's PR will be merged?

@marcorinck

Stumbled about this today too, d'oh! Took me a while to understand that it wasn't my fault but instead a limitation of angular.

@Holokai
Holokai commented Nov 8, 2013

Looking forward to an update on this...

@ooflorent

+1

@KurtRMueller

I got around this by doing <div><tr>...</tr></div>. Ugly ugly ugly but it works.

@prosellen

+1

@calebegg
Member

Just a note that this also seems to affect templates whose root element is <body>

@eckenbach

So to put it plainly, at present it is impossible to write directives using HTML tables without resorting to "hacks"?

@stevedomin

Yes, that's correct

@simbeto
simbeto commented Nov 29, 2013

Also <br/> in the template generates this error.

@dalcib
dalcib commented Dec 14, 2013

+1

@jyonker
jyonker commented Dec 15, 2013

+1

@enriquecastl

+1

@arroz
arroz commented Dec 17, 2013

+1

@albv
albv commented Dec 17, 2013

+1

@VasimHayat

Solution of this bug:-

angular.module('tables',[])
.directive('row', function() {
return {
restrict: 'E',

        scope: { key: '@', value: '@' },
        template: ' <td>{{key}}</td><td>{{value}}</td> '            
    }
});

http://jsfiddle.net/sL9gM/13/

@VasimHayat

Solution of this issue:-

angular.module('tables',[])
.directive('row', function() {
return {
restrict: 'E',

        scope: { key: '@', value: '@' },
        template: ' <td>{{key}}</td><td>{{value}}</td> '            
    }
});

http://jsfiddle.net/sL9gM/13/

@caitp
Contributor
caitp commented Dec 17, 2013

@VasimHayat go ahead and add replace: true to that directive ;) also, this isn't actually technically table content at all, you'll notice that the <row>s are not within the <table> in the DOM

Anyways, we have (at least) two patches with proper solutions for this. Naturally I prefer my own, which can be seen at #5235 complete with tests and demos

@mpodriezov

This issue still occurs with td, tr, thead, tbody...:( Can someone from angular core team review and merge @caitp pull?

@josebalius

+1 on this

@michaelahlers

Still having problems with this.

@tynman
tynman commented Jan 14, 2014

+1. Just ran into this today. Thanks for all the hard work on the issue so far, and I look forward to seeing it resolved. In the meantime, thanks to everyone who's posted hacks!

@jdazacon

I have this problem as well when replacing tr

@sebfie
sebfie commented Jan 21, 2014

Hello,

I have this issue with the "option" root tag.

@onethread

Hmm, don't know if this helps, but I noticed that removing jQuery and letting angular to default to its own jqLite seems to make it "work" as seen here:

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

Though technically, it really shouldn't work, because replace should be set to "true" for the observed behavior. Setting replace to "true" still breaks everything.

Loading in a jQuery library will result in the double "tr" element (which is the correct expected angular behavior).

@MarcMouallem

Soo, we going to have a fix any time soon?

@caitp
Contributor
caitp commented Jan 25, 2014

There are pull requests which provide fixes for this, so there's probably no reason we couldn't get them into the next release, if not 1.3. I'll see if I can get people to look at it.

@caitp caitp was assigned Jan 25, 2014
@Dawil Dawil added a commit to notionparallax/ShadowWolf that referenced this issue Jan 30, 2014
@Dawil Dawil bypass angular bug by not inline replacing <editable>s
see angular/angular.js#1459 for more info.
bbf4f1b
@edmilsonlani

I change the file angular.js:

//$template = jqLite('<div>' + trim(content) + '</div>').contents(); $template = jqLite(trim(content));

This work.

@tbosch tbosch modified the milestone: 1.2.12, 1.2.11 Feb 3, 2014
@tbosch tbosch modified the milestone: 1.2.13, 1.2.12 Feb 7, 2014
@caitp caitp added a commit that closed this issue Feb 14, 2014
@caitp Caitlin Potter + caitp fix($compile) support templates with table content root nodes
If the first element in a template is a <tr>, <th>, <td>, or <tbody> tag,
the HTML compiler will ensure that the template is wrapped in a <table>
element so that the table content is not discarded.

Closes #2848
Closes #1459
Closes #3647
Closes #3241
31c450b
@caitp caitp closed this in 31c450b Feb 14, 2014
@davidjnelson
Contributor

So happy to see this in the romantic-transclusion release!! Great job angular team!! :-D

@michaelahlers

This is some great news. Thanks, @caitp!

@khepin khepin pushed a commit to khepin/angular.js that referenced this issue Feb 19, 2014
Caitlin Potter + Sebastien Armand - sa250111 fix($compile) support templates with table content root nodes
If the first element in a template is a <tr>, <th>, <td>, or <tbody> tag,
the HTML compiler will ensure that the template is wrapped in a <table>
element so that the table content is not discarded.

Closes #2848
Closes #1459
Closes #3647
Closes #3241
8100491
@anagrius

+1

@mansu mansu referenced this issue in lorenzofox3/Smart-Table Mar 29, 2014
Closed

Unable to use angular-ui.bootstrap inside a cell template #96

@revolunet revolunet added a commit to revolunet/angular.js that referenced this issue Jun 12, 2014
@revolunet Caitlin Potter + revolunet fix($compile) support templates with table content root nodes
If the first element in a template is a <tr>, <th>, <td>, or <tbody> tag,
the HTML compiler will ensure that the template is wrapped in a <table>
element so that the table content is not discarded.

Closes #2848
Closes #1459
Closes #3647
Closes #3241
9d3a3c9
@timjacobi

I'm still seeing this issue using 1.3. Am I doing anything wrong?
http://plnkr.co/edit/7OF9zJgaTXTTlvPw1lPr?p=preview

@caitp
Contributor
caitp commented Oct 24, 2014

@timjacobi -- the issue is that the browser's HTML parser will take non-table-content nodes out of your table before angular ever sees them, so we aren't able to put them back for you. To work around this, you can use attribute directives.

@timjacobi

Cheers. I'm trying to only use tag names like in Polymer. Surrounding the <table> with a <div> seems to do the trick though. Not optimal but a good workaround.
template: '<div><table><post-item ng-repeat="post in posts" post="post"></post-item></table></div>'

@caitp
Contributor
caitp commented Oct 24, 2014

I don't think polymer will do any better in this case, the HTML parser just pukes on non-table content within tables unless is="tr" (for example) is used --- but only super-modern browsers support those at all

@pavlovt
pavlovt commented Oct 30, 2014

Here is my solution to this problem:
http://jsfiddle.net/pavlovt/jbwgjkgx/

@caitp
Contributor
caitp commented Oct 30, 2014

so... you're adding transcludes and never using them? :P anyways, it works in that case because the tbl isn't actually a table element when the html is originally parsed, so it doesn't get re-arranged.

@caitp
Contributor
caitp commented Oct 30, 2014

oh hang on, I missed the ng-transclude, so I guess you are using them --- let me tell you, this is going to create zillions of scopes that you probably don't want, so be careful when doing that

@caitp
Contributor
caitp commented Oct 30, 2014

not to mention all of the known unfixable bugs with replace directives

@pavlovt
pavlovt commented Oct 31, 2014

Thanks, I will look for better solution.

@caitp
Contributor
caitp commented Oct 31, 2014

I mean, it works fine, you just have to be careful with it =)

@chazzlabs

@caitp

I've been struggling with this issue today, so I found and modified an existing plunkr (http://plnkr.co/edit/n2aUcuz2yCoPe2RUE5Gx?p=preview) to give me a simple example of using attribute directives to get around this, but I'm still seeing the table row contents being replaced by a span.

Did I misunderstand or incorrectly implement the workaround you suggested?

@caitp
Contributor
caitp commented Dec 16, 2014

You're misunderstanding.

So, here's the deal. The HTML parser (in the browser) has this really stupid "adoption agency algorithm", which will reparent elements during parsing when it decides they don't belong. So what happens is, if you have a <table><tr><custom-td></custom-td></tr></table>, the <custom-td> element gets moved outside of the table during parsing --- this is HTML stupidity, but unfortunately it's not going anywhere.

So, to work around this, you have two options.

  1. Don't use custom-td elements, but just use <td custom-component> --- this will survive HTML parsing, and can be replaced with whatever you want.
  2. Don't make a <table> at all --- If you insert the table dynamically and transclude your child nodes into it, you don't need to worry about the adoption agency algorithm (provided that the element names you use don't result in the reparenting behaviour). This is what you're doing in the plunker you showed --- now in this case, it doesn't really make a difference if the "custom table" directive is an element or attribute directive, it will work in both cases because child nodes won't get reparented
@gitnik
gitnik commented Feb 5, 2015

Hey, I am also running into this problem (I think?). I am using the ng-multi-transclude plugin, which basically let's me have multiple transcludes instead of just one. When using the plugin, the content inside the table is not transcluded at all, unless I change that <td> to a <div>, which is not what I want. Omitting the plugin and using just one ngTransclude (the one inside the table), also didn't work for me, because angular will always replace the TD with a SPAN.
http://plnkr.co/edit/bN3FYKrQi4YMhyt8ocft?p=preview

@konrad-garus

It seems to still be broken with 1.4.7. Check out this plunkr: http://plnkr.co/edit/vgDJadfnwZo7OgCSEVuj?p=preview

This code:

  <div>
  <table>
      <tr><my-cell /><my-cell /></tr>
      <tr><my-cell /><my-cell /></tr>
  </table>
  </div>
  <script>
    angular
      .module('oops', [])
      .directive('myCell', function() {
        return {
          template: '<td>Hello</td>'
        }
      });
  </script>

... leads to the following DOM:

<div>
    <my-cell>Hello</my-cell><my-cell>Hello</my-cell>
    <table>
        <tbody><tr></tr>
        <tr></tr>
    </tbody></table>
</div>

Note how the cells land outside table, and there's only one of them for each table row (template had 4, DOM has 2).

@caitp
Contributor
caitp commented Oct 23, 2015

As has been pointed out in this thread several times now, HTML does not work the way you're wanting it to here, and there is no way for Angular to make this work for you.

When the browser parses <table><tr><my-cell></tr></table>, <my-cell> is evicted via the adoption agency algorithm (my-cell gets reparented), and this happens before Angular ever gets to process the template. This happens because <table>, <tr> and friends are only allowed to have a limited number of tags as child nodes (a tag named my-cell is not in the list of allowed tags)

For technical details on this, see https://html.spec.whatwg.org/#parsing-main-intable

Sorry, but this is not feasible :( You can work around this by defining your own custom <table> directive (eg <my-table>, (and friends), or by using other ways of invoking the directive (such as attributes rather than tag names). Or basically anything you can come up with that will prevent the browser from reparenting the node during parsing

@hgoebl
hgoebl commented Jan 19, 2016

@caitp many thanks for this information - saved me a lot of time finding a workaround.

@otterslide

For anyone still looking, I managed to replace the TR itself with no problem.

Template:
    <script type="text/ng-template" id="comment.html" class="collapse">
            <td>{{commentData.content}}</td><td>test</td>
        </script>

HTML:
<tr comment-item  ng-repeat="comment in comments"
data-index="{{$index}}" data-comment-data="comment">
</tr> 


Directive:
app.directive('commentItem', function($timeout, $http, $document) {
    return {
        restrict: 'A',
        scope: {
            index: '@'
        }, templateUrl: 'comment.html',
        link: function postLink(scope) {
@abacaj
abacaj commented Sep 14, 2016 edited

Why shouldn't this work the way "we expect HTML to work"?

This is why Angular sucks. Long live React.

@caitp caitp was unassigned by abacaj Sep 14, 2016
@caitp
Contributor
caitp commented Sep 14, 2016

That is pretty inappropriate, @abacaj. Please refer to code-of-conduct

@abacaj
abacaj commented Sep 14, 2016 edited

Don't think it's inappropriate. What's inappropriate is having to build out a workaround because of Angular not because of the browser.

Clearly - this is something Angular failed to resolve.

@sebfie
sebfie commented Sep 14, 2016
@otterslide
otterslide commented Sep 14, 2016 edited

@abacaj I have provided the solution and investigated why it happens. There's nothing angular can do to make it work, it's the browser that expects TR within TABLE opening and closing elements that is the issue. If it finds anything like myRow, which is a custom Angular element, it will just discard it. So, this is not why Angular sucks, sorry. Read @caitp 's post, it explains exactly what is going on.

Clearly - this is something Angular failed to resolve.

There is no way to make this work, due to way the the browser behaves. It's not possible to resolve this.

@abacaj
abacaj commented Sep 14, 2016

@otterslide I found it cumbersome that we couldn't use the 'E' restriction. But I suppose it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment