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

JQuery 3.1.1 vs 2.2.4 change in behavior appending rows to a table #3976

Closed
rmacfadyen opened this Issue Feb 12, 2018 · 14 comments

Comments

Projects
None yet
5 participants
@rmacfadyen

rmacfadyen commented Feb 12, 2018

The following code:

$('<TABLE />').append('<TR><TD>1</TD></TR>').appendTo('BODY');
results in slightly different DOM structures.

Under 2.2.4 you get <TABLE><TBODY><TR>...

Under 3.1.1 you get <TABLE><TR>...

(one has a TBODY and one does not)

This causes problems if you later have selectors like #id TBODY TR (to get all the body rows, and no header rows).

I've reviewed the jQuery Core 3.0 Upgrade Guide and can't see anything that might be related.

So my question is... is this expected behaviour or a bug in JQuery?

(from stackoverflow question: https://stackoverflow.com/questions/48658605/jquery-3-1-1-vs-2-2-4-change-in-behavior-appending-rows-to-a-table and forum question https://forum.jquery.com/topic/jquery-3-1-1-vs-2-2-4-change-in-behavior-appending-rows-to-a-table#14737000007604149)

@gibson042

This comment has been minimized.

Member

gibson042 commented Feb 12, 2018

The behavior is expected; we included a fix for #1835 in 3.0.0 but failed to mention it in the upgrade guide (probably because the fix was first included in 2.2.0 and then immediately reverted as an accidental breaking change for 2.2.1, disrupting our milestone tracking).

@JakeCigar

This comment has been minimized.

JakeCigar commented Feb 12, 2018

I see a problem that

        $("<table><tr><td>3.3.1foo</td></tr></table>").appendTo("body")

adds a tbody and

        $('<table>').append('<tr><td>3.3.1bar</td></tr>').appendTo('body')

does not.

@timmywil

This comment has been minimized.

Member

timmywil commented Feb 12, 2018

@JakeCigar That's the browser behavior. jQuery is not adding the tbody there.

@JakeCigar

This comment has been minimized.

JakeCigar commented Feb 12, 2018

I know the browser does. And I think jQuery should too. Lots of people use the tbody for CSS.

@JakeCigar

This comment has been minimized.

JakeCigar commented Feb 12, 2018

And…

        $('<table>').append("<tbody>").append('<tr><td>STRANGE</td></tr>').appendTo('body'); // broken

Might be expected to append 2 children. It does not.

Either tbody is just a node or it is special. It should not be both ways.

@rmacfadyen

This comment has been minimized.

rmacfadyen commented Feb 12, 2018

fix for #1835 in 3.0.0 but failed to mention it in the upgrade guide

Any thoughts on what else might be affected by this breaking change?

How/where can I refile this issue such that the upgrade guide gets updated with this breaking change?

@dmethvin

This comment has been minimized.

Member

dmethvin commented Feb 14, 2018

@rmacfadyen the upgrade guide is at https://github.com/jquery/jquery.com/blob/master/pages/upgrade-guide/3.0.md , you can file a ticket and PR at that repo. It would be great if you or @JakeCigar could propose a description for this, it would be a new item in Manipulation near https://jquery.com/upgrade-guide/3.0/#breaking-change-wrapall-function-only-calls-the-function-once . Generally you'd want to succinctly describe the change and propose an alternate solution.

The behavior of tbody in tables has always been strange in browsers. There can be multiple tbody elements in a table which makes it even stranger. If there is already a tbody you can select that one and insert rows there or create a new one. Until 3.0 jQuery had a bunch of fiddly logic about it which is why we wanted to remove it. The person inserting the markup knows best where it should go.

@JakeCigar

This comment has been minimized.

JakeCigar commented Feb 14, 2018

Do you want me to document how jQuery is broken, with regard to the tbody tag? Even in 3.3.1
Native code like this gives the table 2 children…

var table=document.createElement("table"),
    tbody=document.createElement("tbody"),
    tr=document.createElement("tr"),
    td=document.createElement("td");
td.textContent="OK"
tr.appendChild(td);
table.appendChild(tbody);
table.appendChild(tr);
document.body.appendChild(table);

but jQuery code like…

$('<table>').append("<tbody>").append('<tr><td>STRANGE</td></tr>').appendTo('body');

has one child under the table.

I think you should get rid of manipulationTarget. It doesn’t match up with the rest of the behavior of jQuery.

@JakeCigar

This comment has been minimized.

JakeCigar commented Feb 14, 2018

Maybe we should say. "Sometimes jQuery running in some browsers will give you a tbody, other times not. If you care about getting a tbody, make sure you code for it." Followed by some ways to make sure you get a tbody.

@rmacfadyen

This comment has been minimized.

rmacfadyen commented Feb 14, 2018

@dmethvin I've added issue jquery/jquery.com#171, thanks for pointing me to the right spot.

I haven't done a PR because I'm pretty uncertain that I understand the scope of the breaking change, and I'm really not sure what the document should say about it either.

@timmywil

This comment has been minimized.

Member

timmywil commented Feb 14, 2018

@JakeCigar That example is invalid. Specifically, it's not valid to insert tbody elements as siblings of tr elements per the spec. Fortunately, jQuery prevents creating the invalid table and adds the tr element where it should be. Be sure you're not just seeking out errors here. If you're counting on tbody elements in your CSS, adding them explicitly is a much better way to go. It's more readable and prevents confusion.

Sometimes jQuery running in some browsers will give you a tbody, other times not. If you care about getting a tbody, make sure you code for it.

jQuery is no longer involved in the auto-insertion, but I agree it's useful for users to know when browsers will automatically insert a tbody. That is something useful for the docs, not just upgrade guide.

@JakeCigar

This comment has been minimized.

JakeCigar commented Feb 14, 2018

@timmywil jQuery is involved. manipulationTarget does appends under a tbody when the user specifically asks to append under the table. If a tbody is present.

I could be happy with it completely one way or the other. I like the idea of being less kind.

@timmywil

This comment has been minimized.

Member

timmywil commented Feb 14, 2018

@JakeCigar That's not what I meant. We no longer insert a tbody at all. It's the browser doing it natively. manipulationTarget is there to help prevent the creation of invalid tables, which your example actually demonstrates and is an argument for it rather than against it.

@dmethvin

This comment has been minimized.

Member

dmethvin commented Feb 14, 2018

As far as I can tell, the reason why I haven't come across this difference in any code that I write is that I inject the HTML I expect to see. It seems like as long as you do this there are no problems. Appending a TR to a TABLE shouldn't be done. I completely agree that it's a behavior change and that it should be mentioned in the upgrade guide.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2018

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