Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate tbody in ngFor #5967

Closed
ggmod opened this issue Dec 16, 2015 · 29 comments
Closed

Duplicate tbody in ngFor #5967

ggmod opened this issue Dec 16, 2015 · 29 comments

Comments

@ggmod
Copy link

ggmod commented Dec 16, 2015

If I write something like this:

<tbody *ngFor="#item of [1,2,3]" myTableRow></tbody>

that uses the following component:

@Component({
  selector: '[myTableRow]',
  template: '<tr><td>test</td></tr>'
})
export class MyTableRow {}

then it produces invalid HTML, there will be two nested tbody tags, like this:

<tbody mytablerow="">
  <tbody>
    <tr><td>test</td></tr>
   </tbody>
</tbody>

(In the real application I use this to group together multiple tr tags in a tbody, and the table has many tbody tags after each other)

I get all kinds of malformed tables because of this, and it also breaks the usual table > tbody > td CSS rules, and is actually invalid HTML.
This worked fine with alpha.46, but has been broken since alpha.47.
Demo here: http://plnkr.co/edit/myYJZf6ZZj1ofhIcNj7E?p=preview

@ggmod
Copy link
Author

ggmod commented Dec 27, 2015

Actually, the issue doesn't depend on ngFor at all, the same thing happens with a template like this:

<tbody myTableRow></tbody>

The problem is caused by the fact that the template parser's required parent logic applies to the template's root elements too: but the parent element of the template's root element is unkown at the time when the template is compiled, it will only be revealed when the component is inserted into the page.

If I change angular's html parser to skip the required parent logic for the root element(s), then everything works fine. In html_parser.ts

if (tagDef.requireExtraParent(isPresent(parentEl) ? parentEl.name : null)) {

should be:

if (isPresent(parentEl) && tagDef.requireExtraParent(parentEl.name)) {

@pkozlowski-opensource
Copy link
Member

@ggmod what is the version of Angular2 that you are using? Could you please provide a reproduce scenario with plunker (or similar on-line tool)?

@ggmod
Copy link
Author

ggmod commented Dec 29, 2015

Both the plunker and the version number are there in the original post, but here it is again:
http://plnkr.co/edit/myYJZf6ZZj1ofhIcNj7E?p=preview
using the beta version, but the mistake appeared between alpha.46 and alpha.47

@pkozlowski-opensource
Copy link
Member

Yes, I see, this is a bug. Thnx for reporting.

@PhilippeOberti
Copy link

hi guys! any idea when this would be fixed? I am using 2.0.0-beta.3 and the issue is still there :)

@sasidhar
Copy link
Contributor

+1 Need a fix for this bug

@denodaeus
Copy link

+1, also need a fix for this bug (using beta.7)

@0x-r4bbit
Copy link
Contributor

Maybe someone is willing to try to fix it?

@sasidhar
Copy link
Contributor

sasidhar commented Mar 4, 2016

@PascalPrecht, already @ggmod mentioned a fix above. But it's not a pull request. I am currently using his fix with angular beta.7

@cagataycivici
Copy link

I seriously need the fix for this bug. +1

I had to make a workaround with using a div with display: table-row to avoid a tbody getting generated.

@adrianromanko
Copy link

+1

@denodaeus
Copy link

Is this going to make it into a release soon?

@mohsenvafa
Copy link

+1 I need also a fix for this.

@yassirj1
Copy link

+1 this needs to be fixed

Also, when trying to create a component using
<thead></thead>
angular2 will nest a tbody into it.

@tsalzinger
Copy link

I just stumpled across the same issue (using beta.14)

The issue acutally seems to be that the template starts with a <tr> element:
http://plnkr.co/edit/7zou2L4rMC8t6LccJbMV

@lukaszdechovan
Copy link

+1 I would like this bug fixed, as workaround I replaced all table elements with divs and created css classes (such as .table, .thead, .tr, .td, .th) but thats really ugly non-semantic solution.

@zoechi
Copy link
Contributor

zoechi commented May 4, 2016

I guess that's a browser feature. Angular probably creates documentFragments of the HTML (<tr>) before inserting and the browser automatically wrapps it with <tbody> and that is than inserted (just a guess).

@tsalzinger
Copy link

@zoechi i don't think that's the issue

if you execute

var df = new DocumentFragment();
df.appendChild(document.createElement('tr'));
document.querySelector('body').appendChild(df);

you get exactly the dom you would expect:

<body>
  <tr></tr>
</body>

not very useful or even valid but still exactly what you defined

@denodaeus
Copy link

Any update on this one? It's a P2 with a fix attached to it -- but this bug still made it into the release candidate.

revov added a commit to revov/uci-gui that referenced this issue May 7, 2016
- Also still experiencing socket cleanup issues due to angular/angular#8458
- Tables were changed with cards in previous commit due to angular/angular#5967
@andyrue
Copy link

andyrue commented May 12, 2016

+1 Also just ran into this with rc.1.

@fringd
Copy link

fringd commented May 13, 2016

It seems like you're doing it wrong? here's the way I write this. I don't see any reference to your syntax in the docs at https://angular.io/docs/ts/latest/guide/template-syntax.html

<tbody>
  <myTableRow *ngFor="#item of [1,2,3]"></myTableRow>
</tbody>

EDIT:
looks like this probably won't work either, since you can't have <myTableRow> elements in a tbody...

this is probably what you really want:

<tbody>
  <template *ngFor="#item of [1,2,3]" myTableRow></item>
</tbody>

@fringd
Copy link

fringd commented May 13, 2016

your <tbody *ngFor... myTableRow>
gets translated into

<template [ngFor]...>
  <tbody myTableRow></tbody>
</template>

and your myTableRow component will be re-rendered, including it's host element, which is tbody the way you've constructed it. Maybe the syntax was different earlier, but i think this is how it's supposed to work?

@zoechi
Copy link
Contributor

zoechi commented May 13, 2016

@fringd this should lead to

<tbody mytablerow="">
    <tr><td>test</td></tr>
</tbody>
<tbody mytablerow="">
    <tr><td>test</td></tr>
</tbody>
<tbody mytablerow="">
    <tr><td>test</td></tr>
</tbody>

but the result is

<tbody mytablerow="">
  <tbody>
    <tr><td>test</td></tr>
   </tbody>
</tbody>
<tbody mytablerow="">
  <tbody>
    <tr><td>test</td></tr>
   </tbody>
</tbody>
<tbody mytablerow="">
  <tbody>
    <tr><td>test</td></tr>
   </tbody>
</tbody>

@fringd
Copy link

fringd commented May 13, 2016

oh weird. the example at the top is a confusing example then. why would anybody want multiple tbody elements?

@poke
Copy link

poke commented May 18, 2016

@pkozlowski-opensource Is there anything we can do to speed this up? There’s already a proposed fix above. Would that be an acceptable fix? Do you need a pull request including that change? Can we please get this fixed soon?

@chrille112
Copy link

+1 on this. FYI, in rc1 this is fixed by:
compiler/src/html_parser.js, line 257 should be:
if (lang_1.isPresent(parentEl) && tagDef.requireExtraParent(parentEl.name)) {

@eapenjohn
Copy link

eapenjohn commented Jun 15, 2016

I am still facing the same issue even after applying the below.Its generate 2 duplicate items
if (lang_1.isPresent(parentEl) && tagDef.requireExtraParent(parentEl.name)) { var newParent = new html_ast_1.HtmlElementAst(tagDef.parentToAdd, [], [el], el.sourceSpan, el.startSourceSpan, el.endSourceSpan); this._addToParent(newParent);

template is
template: <ul> <dataset-tile *ngFor='let dataset of datasets' [hidden]='dataset.hide==true' [dataset]='dataset'></dataset-tile> </ul>

angular version :2.0.0-beta.17

@vicb
Copy link
Contributor

vicb commented Jun 15, 2016

fix is coming...

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests