Bug Tracker

Opened 5 years ago

Closed 5 years ago

#14290 closed bug (fixed)

regression $('<table></table>').append("text content") fails in 1.10

Reported by: karger@… Owned by: karger@…
Priority: undecided Milestone: None
Component: unfiled Version: 1.10.2
Keywords: Cc:
Blocked by: Blocking:

Description

$('<table></table>').append("text content") generates an exception in jquery 1.10. It works fine in jquery 1.9 . The resulting html is invalid, but it still seems wrong for jquery to throw an exception.

jsfiddle: http://jsfiddle.net/EPJS6/

The culprit is line 6300 of jquery-1.10.2.js , jQuery.nodeName( content.nodeType === 1 ? content : content.firstChild, "tr" ) which for some reason takes the firstChild of any content that is not an elementnode, so in particular when content is a textnode it takes the textNode's firstChild which is null and passes it as an argument to nodeName. Are the two cases of the conditional swapped from what they should be?

Change History (7)

comment:1 Changed 5 years ago by dmethvin

I'm okay with it throwing an error for this case. Is there some benefit to developers to letting it quietly create invalid markup?

comment:2 Changed 5 years ago by anonymous

Well, to be clearer, while it always crashes it only *sometimes* creates invalid markup. I ran into the bug because I am using a templating library that recursively copies child elements into (a copy of) the parent element. The bug was triggered when the templating engine was copying a template containing <table> <tbody><tr>stuff...</table>. The first child the templating engine tried to copy into a new table node was the textnode consisting of just whitespace before the tbody. Creating (temporarily) <table>..whitespace..</table> is, I think, a plausible step in building a table. If jq continues to throw this error, then it becomes the responsibility of the template engine to check for and skip inserting whitespace, which just doesn't seem like a job it should have to do given that it is building valid html.

comment:3 Changed 5 years ago by jordifebrer

Ideally do not throw an exception and add the text content like jQuery 1.9 and older versions.

Possible fix is:

function manipulationTarget( elem, content ) {
   return jQuery.nodeName( elem, "table" ) &&
-    jQuery.nodeName( content.nodeType === 1 ? content : content.firstChild, "tr" ) ?
+    jQuery.nodeName( ( content.nodeType === 1 || content.firstChild === null ) ? content : content.firstChild, "tr" )

Last edited 5 years ago by jordifebrer (previous) (diff)

comment:4 Changed 5 years ago by dmethvin

Owner: set to karger@…
Status: newpending

Sorry, what I was asking is, other than not throwing an error, what should the outcome be? You're defining a behavior with your suggested patch but is that really the desired behavior?

comment:5 in reply to:  4 Changed 5 years ago by karger

Replying to dmethvin:

Sorry, what I was asking is, other than not throwing an error, what should the outcome be? You're defining a behavior with your suggested patch but is that really the desired behavior?

So at the risk of overgeneralizing, I think we should recognize that it is possible (and natural) for invalid html to "grow" to become valid. This implies that jquery should be robust in handling (temporarily) invalid html, in order to protect the user from having to maintain validity at every single step.

Another, related example of this would be manipulating a table without a tbody. jquery appears to helpfully create a tbody in this case. But what happens if the table is only temporarily without a tbody, and the user adds one later? Suddenly we have two tbodys, which is a problem.

comment:6 Changed 5 years ago by dmethvin

So at the risk of overgeneralizing, I think we should recognize that it is possible (and natural) for invalid html to "grow" to become valid. This implies that jquery should be robust in handling (temporarily) invalid html, in order to protect the user from having to maintain validity at every single step.

It can be risky to define our behavior as a superset of the standard. If the standard changes against jQuery's behavior, we're forced to choose between standards and backwards compatibility.

The pull request, only ensures that there is one child element in the table and (implicitly) that no exception was thrown. I think that is good, actually, because it doesn't put too many constraints on how a browser could handle invalid markup. But what if the browser discarded the text? I haven't looked at the requirements for how HTML5 would parse this but I wouldn't want to go beyond (and certainly not against) that.

Another, related example of this would be manipulating a table without a tbody. jquery appears to helpfully create a tbody in this case.

That is required because IE auto-inserts a tbody if we don't. It is easier to make all browsers behave consistently that way. I don't think this case is analogous.

But what happens if the table is only temporarily without a tbody, and the user adds one later? Suddenly we have two tbodys, which is a problem.

A table can have multiple tbody elements. http://dev.w3.org/html5/html-author/#the-table-element

comment:7 Changed 5 years ago by Dave Methvin

Resolution: fixed
Status: pendingclosed

Fix #14290. Don't throw if text node is appended to table. Close gh-1371.

Changeset: ec3ac9a247ec3132dea3c30ca36cefc217e25441

Note: See TracTickets for help on using tickets.