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 comment:1
Changed October 14, 2010 05:42AM UTC by comment:2
owner: | → timmolendijk |
---|---|
status: | new → pending |
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 comment:3
status: | pending → new |
---|
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 comment:4
status: | new → pending |
---|
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 comment:5
status: | pending → new |
---|
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 comment:6
milestone: | 1.4.3 → 1.5 |
---|---|
priority: | undecided → low |
status: | new → pending |
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 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: | pending → new |
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 comment:8
status: | new → open |
---|---|
version: | 1.4.2 → 1.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 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
innerHTMLinstead of
html()of course)?
Changed April 15, 2011 03:31AM UTC by comment:10
component: | attributes → manipulation |
---|---|
resolution: | → patchwelcome |
status: | open → closed |
criterions=criteria