Skip to content
Permalink
Browse files
Adjust jQuery('html') detection to only match when html starts with '…
…<' (counting space characters). Fixes #11290
  • Loading branch information
timmywil committed Jun 20, 2012
1 parent 286c4d9 commit 239fc86
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 6 deletions.
@@ -40,9 +40,8 @@ var
trimRight = /\s+$/,

// A simple way to check for HTML strings
// Prioritize #id over <tag> to avoid XSS via location.hash (#9521)
// Ignore html if within quotes "" '' or brackets/parens [] ()
rhtmlString = /^(?:[^#<\\]*(<[\w\W]+>)(?![^\[]*\])(?![^\(]*\))(?![^']*')(?![^"]*")[^>]*$)/,
// If starts-with '<'
rhtmlString = /^\s*(<[\w\W]+>)[^>]*$/,

// Match a standalone tag
rsingleTag = /^<(\w+)\s*\/?>(?:<\/\1>)?$/,
@@ -27,7 +27,7 @@ test("jQuery()", function() {
div = jQuery("<div/><hr/><code/><b/>"),
exec = false,
long = "",
expected = 26,
expected = 24,
attrObj = {
click: function() { ok( exec, "Click executed." ); },
text: "test",
@@ -132,14 +132,12 @@ test("jQuery()", function() {
elem.remove();

equal( jQuery(" <div/> ").length, 1, "Make sure whitespace is trimmed." );
equal( jQuery(" a<div/>b ").length, 1, "Make sure whitespace and other characters are trimmed." );

for ( i = 0; i < 128; i++ ) {
long += "12345678";
}

equal( jQuery(" <div>" + long + "</div> ").length, 1, "Make sure whitespace is trimmed on long strings." );
equal( jQuery(" a<div>" + long + "</div>b ").length, 1, "Make sure whitespace and other characters are trimmed on long strings." );
});

test("selector state", function() {

16 comments on commit 239fc86

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this effect round-tripping?

var div = $('<div>hi <div>hello</div></div>');
div.html(div.html());

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timmywil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is intentionally more strict and would be a change in behavior for that situation. However, the coming introduction of parseHTML will take care of explicitly generating html.

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it's a change in behavior. Right now on edge, it's causing a syntax error. Please see my fiddle.

@dmethvin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a syntax error because the string prefixed with "hi ..." is no longer considered to be HTML, which is what the patch is trying to do.

For that particular case you're not really round-tripping the HTML since the outer div is gone and $() has always lost leading/trailing text.

If this seems to abrupt, and I can agree that it might be, I suggest that we leave $(html) untouched (or at least less aggressively neutered, for example allow leading alphanumerics) and only introduce $.parseHTML in 1.8 with the strong warning that people MUST switch over by 1.9 if they want to process random HTML.

@timmywil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be alright with that. I guess the question is are there enough cases where this would break to warrant not adding now?

@dmethvin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well if there are, I'm foreseeing a situation like we had in 1.6 with attroperties. If we warn about it now and provide $.parseHTML simultaneously, nobody can say they were not told.

@rwaldron
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, my concern is that this is too drastic of a change - especially for those that have large systems, plugins etc. /2cents

@timmywil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwldrn: agreed, I'm working on parseHTML right now. Dave is a little low on time.

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to be clear, this change and the previous mod to this regex are being reverted?

@dmethvin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we're not changing $() behavior in 1.8; instead we'll evangelize and get feedback over the next 6 months and really lock it down in 1.9 if all goes well. The key is to have parseHTML ready so people have a way to explicitly say they are expecting HTML and whether they want it to run scripts.

Sound reasonable, everyone?

@rwaldron
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@jaubourg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@gibson042
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@Krinkle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record: Reverted in c20e031.

By the way, this didn't affect .html(), right? I mean setting $foo.html('hello <p></p>'); is completely valid imho. Passing it to $() seems a different case. Although I can imagine both being a potential problem.

@timmywil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't affect that.

Please sign in to comment.