Skip to main content

Bug Tracker

Side navigation

#7138 closed bug (patchwelcome)

Opened October 09, 2010 10:12PM UTC

Closed April 15, 2011 03:31AM UTC

Last modified March 09, 2012 09:06PM UTC

.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.

Attachments (0)
Change History (10)

Changed October 09, 2010 10:19PM UTC by timmolendijk comment:1

criterions=criteria

Changed October 14, 2010 05:42AM UTC by snover comment:2

owner: → 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.

Changed October 16, 2010 07:11PM UTC by timmolendijk comment:3

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.

Changed October 18, 2010 10:26PM UTC by snover comment:4

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.

Changed November 11, 2010 01:45AM UTC by timmolendijk comment:5

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.

Changed November 11, 2010 02:43PM UTC by SlexAxton comment:6

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?

Changed November 11, 2010 03:36PM UTC by timmolendijk comment:7

_comment0: 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 cases 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.1289489877429132
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.

Changed November 21, 2010 04:31AM UTC by snover comment:8

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. :\\

Changed November 21, 2010 09:43PM UTC by timmolendijk comment:9

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)?

Changed April 15, 2011 03:31AM UTC by timmywil comment:10

component: attributesmanipulation
resolution: → patchwelcome
status: openclosed