Bug Tracker

Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#7138 closed bug (patchwelcome)

.html()'s criterions for using innerHTML are insufficient

Reported by: timmolendijk Owned by: timmolendijk
Priority: low Milestone:
Component: manipulation Version: 1.4.4
Keywords: html innerHTML whitespace newlines IE8 Cc:
Blocked by: Blocking:

Description

.html() internally uses innerHTML if it thinks it is safe to do so. One of the requirements is that jQuery.support.leadingWhitespace is true or the supplied value does not have any leading whitespace (line 4192).

This behavior is problematic as leading whitespace is not a sufficient definition of the risk with innerHTML. It is whitespace in general. To illustrate my point, the following statements are hold all true in Internet Explorer 8 (in which jQuery.leadingWhitespace is false):

var newline = 'line1\nline2'; $('<div />').html(newline).text() !== newline;

var newlineLeadingWhitespace = ' line1\nline2'; $('<div />').html(newlineLeadingWhitespace).text() === newlineLeadingWhitespace;

The source of this behavior is that in the first example innerHTML is used, in the second not.

Change History (10)

comment:1 Changed 9 years ago by timmolendijk

criterions=criteria

comment:2 Changed 9 years ago by snover

Owner: set to timmolendijk
Status: newpending

What exactly breaks here? Just because the whitespace is normalized doesn’t mean that it is unsafe to use innerHTML. Once markup touches the DOM, you should not expect to get back exactly the same thing as what you put in when it is reserialized by the browser later.

comment:3 Changed 9 years ago by timmolendijk

Status: pendingnew

http://jsfiddle.net/P2HEL/

Wouldn't you agree that this code should always alert with "true" ?

In IE8 it alerts with "false". This is directly caused by jQuery's implementation of .html() in which a different code path is taken based on whether the supplied string has prepending whitespace or not.

comment:4 Changed 9 years ago by snover

Status: newpending

Other than direct string to string comparison of serialized html, which you should not do because you are not guaranteed that what you put in is the same as what comes out, what breaks? If there is no actual use-case for this issue then I am not sure why we would slow down IE to prevent it.

comment:5 Changed 9 years ago by timmolendijk

Status: pendingnew

I ran into this because I was working on client-side templating, in which this can be a problem. jquery-tmpl for sure suffers from it (https://github.com/nje/jquery-tmpl/issues#issue/27).

Check-out http://jsfiddle.net/P2HEL/5/ first in any browser but IE and then in IE (I use version 8). The fact that it messes up the newline is an actual problem here.

Yes, you could use .text() instead, but the thing is that in abstractions such as templating libraries the code does not know if it is dealing with stuff that is gonna end up inside a pre tag (or any element with white-space:pre in that respect) or not. So I'd say it's something that .html() should be dealing with.

And also, from the .html() source I can tell that a deliberate effort has been made to fix problems with whitespace in IE. My test case demonstrates that I think this effort should be improved because it fixes the problem in situations in which the content starts with whitespace, but it fails to fix it if this is not the case.

comment:6 Changed 9 years ago by SlexAxton

Milestone: 1.4.31.5
Priority: undecidedlow
Status: newpending

In the case of jquery-tmpl text() would suffice since there are no tags. That is a fundamentally different thing, getting text out of a dom element and getting the representation of dom elements as text are very different problems.

Are you saying that if IE8 underwent the same treatment that IE6 or 7 goes through that it would just work?

Could you provide a patch that shows this?

comment:7 Changed 9 years ago by timmolendijk

Status: pendingnew

In case of .html("line1\nline2") the code at https://github.com/jquery/jquery/blob/master/src/manipulation.js#L234 decides that it can take the shortcut and use innerHTML.

What I'm saying is that this is incorrect. The jsFiddle test case demonstrates why.

In case of .html(" line1\nline2") it is (rightfully) decided that no shortcut can be taken (in IE). The same code path should be taken for my first example.

Last edited 9 years ago by timmolendijk (previous) (diff)

comment:8 Changed 9 years ago by snover

Status: newopen
Version: 1.4.21.4.4

So I guess we need to add a new test for HTML normalization to see if \n gets converted, and if so, take the slow path for strings that contain \n. I am just afraid the testing here is going to be really really slow, but I guess there is nothing to be done. :\

comment:9 Changed 9 years ago by timmolendijk

What exactly is going to be slow? Isn't it just one test like $('<div />').html("line1\nline2").text().charCodeAt(5) === 10 (but then using innerHTML instead of html() of course)?

comment:10 Changed 9 years ago by timmywil

Component: attributesmanipulation
Resolution: patchwelcome
Status: openclosed
Note: See TracTickets for help on using tickets.