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

fix($compile): properly handle directive replace for table elements #3647

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@pelme
Contributor

pelme commented Aug 18, 2013

To work around limitations in jqLite, the template snippet is wrapped in
a

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

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
@pburgmer

This comment has been minimized.

Show comment
Hide comment
@pburgmer

pburgmer commented Aug 23, 2013

+1

@mabuzer

This comment has been minimized.

Show comment
Hide comment
@mabuzer

mabuzer commented Aug 23, 2013

+1

@ricardo37

This comment has been minimized.

Show comment
Hide comment
@ricardo37

ricardo37 commented Aug 29, 2013

+1

@bashmish

This comment has been minimized.

Show comment
Hide comment
@bashmish

bashmish commented Sep 5, 2013

+1

@mhallin79

This comment has been minimized.

Show comment
Hide comment
@mhallin79

mhallin79 commented Sep 11, 2013

+1

@migajek

This comment has been minimized.

Show comment
Hide comment
@migajek

migajek Sep 29, 2013

I've recklessly started learning directives from writing the one for table manipularing, and had no idea what was doing wrong until found out things work properly when I replace tr/ths with divs or anything else.

migajek commented Sep 29, 2013

I've recklessly started learning directives from writing the one for table manipularing, and had no idea what was doing wrong until found out things work properly when I replace tr/ths with divs or anything else.

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Oct 1, 2013

Contributor

If this works in jQuery, perhaps it would be better to improve jqLite. @IgorMinar what do you think of this?

Contributor

btford commented Oct 1, 2013

If this works in jQuery, perhaps it would be better to improve jqLite. @IgorMinar what do you think of this?

@ghost ghost assigned btford Oct 1, 2013

@pelme

This comment has been minimized.

Show comment
Hide comment
@pelme

pelme Oct 1, 2013

Contributor

This is more of a browser thing, jQuery('<div><tr></tr></div>') does not work either.

It might be possible to do away with the div-wrapping altogether by improving jqLite, though.

Contributor

pelme commented Oct 1, 2013

This is more of a browser thing, jQuery('<div><tr></tr></div>') does not work either.

It might be possible to do away with the div-wrapping altogether by improving jqLite, though.

@migajek

This comment has been minimized.

Show comment
Hide comment
@migajek

migajek Oct 1, 2013

@pelme why would one need to wrap table-only element with div? jQuery('<div><tr></tr></div>') doesn't work, but $('<tr></tr>') does - yet Angular doesn't let me replace <tr my-directive> with another <tr> from template ... so how is it related?

migajek commented Oct 1, 2013

@pelme why would one need to wrap table-only element with div? jQuery('<div><tr></tr></div>') doesn't work, but $('<tr></tr>') does - yet Angular doesn't let me replace <tr my-directive> with another <tr> from template ... so how is it related?

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Oct 2, 2013

Contributor

Punting this to 1.2.1 for now. Will follow with details (spoiler: it's complicated).

Contributor

btford commented Oct 2, 2013

Punting this to 1.2.1 for now. Will follow with details (spoiler: it's complicated).

@migajek

This comment has been minimized.

Show comment
Hide comment
@migajek

migajek Oct 2, 2013

Is there any workaround/dirty fix for that? Or should we just avoid table-related directives until 1.2.1 ?

migajek commented Oct 2, 2013

Is there any workaround/dirty fix for that? Or should we just avoid table-related directives until 1.2.1 ?

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Oct 14, 2013

Member

the problem here is that angular needs to create fragments of dom and the easiest way to do so is via innerHTML. with innerHTML howerver, you need a container to innerHTML the string template into and we use <div> element for that. But because of certain html rules you can't innerHTML certain elements to certain other elements. This is the case with <div> and <tr>. jQuery uses quite a bit of code to overcome this via heuristics, I'm not keen on doing something along those lines in jqLite.

Even if we did, there is still issue with multi-root templates <tr>1</tr><tr>2</tr> which we need to create within some container and as I wrote before, determining what this container should be is tricky.

Member

IgorMinar commented Oct 14, 2013

the problem here is that angular needs to create fragments of dom and the easiest way to do so is via innerHTML. with innerHTML howerver, you need a container to innerHTML the string template into and we use <div> element for that. But because of certain html rules you can't innerHTML certain elements to certain other elements. This is the case with <div> and <tr>. jQuery uses quite a bit of code to overcome this via heuristics, I'm not keen on doing something along those lines in jqLite.

Even if we did, there is still issue with multi-root templates <tr>1</tr><tr>2</tr> which we need to create within some container and as I wrote before, determining what this container should be is tricky.

@migajek

This comment has been minimized.

Show comment
Hide comment
@migajek

migajek Oct 22, 2013

Summing up, will this one be kind of wontfix answer? I have seen quite a few table related directives and all of them uses some tricks for overcoming this problem.

migajek commented Oct 22, 2013

Summing up, will this one be kind of wontfix answer? I have seen quite a few table related directives and all of them uses some tricks for overcoming this problem.

@migajek

This comment has been minimized.

Show comment
Hide comment
@migajek

migajek Oct 22, 2013

Btw, @IgorMinar if auto-detecting what wrapping tag should be used, how about letting the developer decide? Just add the "templateWrapper" property which defaults to '

', yet the developer could provide other (like table or thead)?

migajek commented Oct 22, 2013

Btw, @IgorMinar if auto-detecting what wrapping tag should be used, how about letting the developer decide? Just add the "templateWrapper" property which defaults to '

', yet the developer could provide other (like table or thead)?

@migajek

This comment has been minimized.

Show comment
Hide comment
@migajek

migajek Oct 22, 2013

  • defaults do div tag...

migajek commented Oct 22, 2013

  • defaults do div tag...
@KendallPark

This comment has been minimized.

Show comment
Hide comment

KendallPark commented Oct 31, 2013

+1

@marcorinck

This comment has been minimized.

Show comment
Hide comment
@marcorinck

marcorinck commented Nov 3, 2013

+1

@ooflorent

This comment has been minimized.

Show comment
Hide comment
@ooflorent

ooflorent Nov 14, 2013

@btford:
If this works in jQuery, perhaps it would be better to improve jqLite.

I included jQuery 2.0 before angular (so jqLite is replaced by jQuery) and the problem is still there.
This seems to be specific to angular...

ooflorent commented Nov 14, 2013

@btford:
If this works in jQuery, perhaps it would be better to improve jqLite.

I included jQuery 2.0 before angular (so jqLite is replaced by jQuery) and the problem is still there.
This seems to be specific to angular...

@migajek

This comment has been minimized.

Show comment
Hide comment
@migajek

migajek Nov 15, 2013

any chances to get this one fixed?

migajek commented Nov 15, 2013

any chances to get this one fixed?

Merge branch 'master' into issue1459
Conflicts:
	test/ng/compileSpec.js
@dalcib

This comment has been minimized.

Show comment
Hide comment
@dalcib

dalcib commented Dec 14, 2013

+1

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 14, 2013

Contributor

I didn't realize someone else had a patch for this. I think this is okay, but it's possible to move this into a closure so that in the future it might be extended to support other content types (like SVG content, for example). Also, the 'indexOf' strategy could potentially be problematic, as it is very strict --- I think a regexp is probably a nicer way to do this, in terms of being forgiving. My implementation is at #5235

Contributor

caitp commented Dec 14, 2013

I didn't realize someone else had a patch for this. I think this is okay, but it's possible to move this into a closure so that in the future it might be extended to support other content types (like SVG content, for example). Also, the 'indexOf' strategy could potentially be problematic, as it is very strict --- I think a regexp is probably a nicer way to do this, in terms of being forgiving. My implementation is at #5235

@caitp caitp closed this in 31c450b Feb 14, 2014

khepin pushed a commit to khepin/angular.js that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment