1.7 HTML5 Support for innerHTML, clone & style. Fixes #6485 #490

Closed
wants to merge 4 commits into
from

Projects

None yet

9 participants

@rwaldron
Member
rwaldron commented Sep 7, 2011

This initial pull request should serve as a request for comments, reviews etc.

The approach is based entirely on work by Jonathan Neal, championed by Paul Irish. Please see the ticket for complete details.

This has been tested and is passing in:

FF3.6.20, 6, 7
Chrome 12, 13
Safari 5.0, 5.1
IE 6, 7, 8
Opera 11.01, 11.50
If I could get some backup testing on IE9, that would be greatly appreciated.

http://bugs.jquery.com/ticket/6485

@paulirish
Member

@jdbartlett, would appreciate your code review.

@rwaldron
Member
rwaldron commented Sep 7, 2011

@jdalton code review please

@mathiasbynens mathiasbynens and 3 others commented on an outdated diff Sep 7, 2011
src/manipulation.js
@@ -537,6 +537,23 @@ function findInputs( elem ) {
jQuery.grep( elem.getElementsByTagName("input"), fixDefaultChecked );
}
}
+// Create safe fragments
+function safeFragment( context ) {
+ var nodeNames = (
+ "abbr article aside audio canvas datalist details figcaption figure footer " +
+ "header hgroup mark meter nav output progress section subline summary time video"
+ ).split(/\s+/),
@mathiasbynens
mathiasbynens Sep 7, 2011 Contributor

Honest question: why split(/\s+/) instead of just split(' ')? Is it supposed to be faster or anything?

@rwaldron
rwaldron Sep 7, 2011 Member

Borrowed from rspace in src/attributes.js, not married to it

@rwaldron
rwaldron Sep 7, 2011 Member

http://jsperf.com/literal-space-vs-regex according to this, literal space is faster

@mathiasbynens
mathiasbynens Sep 7, 2011 Contributor

Cool, it’s shorter too :) It doesn’t seem to make any difference performance-wise in IE.

@staabm
staabm Sep 19, 2011 Contributor

Should attributes.js than also be changed to use space instead of regex?

@millermedeiros
millermedeiros Sep 28, 2011

/\s+/ will capture multiple spaces, it's safer since it can be hard to spot 2 spaces.

("abbr article  aside").split(' ');    // ["abbr", "article", "", "aside"]
("abbr article  aside").split(/\s+/);  // ["abbr", "article", "aside"]
@mathiasbynens
mathiasbynens Sep 28, 2011 Contributor

It’s not like this line of code will change very frequently.

@millermedeiros
millermedeiros Sep 28, 2011

the line will be only executed once, so if it runs 125K ops/sec instead of 200K it won't make any difference.. I would keep the RegExp.

@rwaldron
rwaldron Sep 28, 2011 Member

Pretty sure I did perf testing for that one, but thanks anyway.

@mathiasbynens
mathiasbynens Sep 28, 2011 Contributor

Uhm, yeah — you linked to a jsPerf test case a few lines up ;)

@millermedeiros
millermedeiros Sep 28, 2011

I took the 125K/200K ops/sec from the Chrome result (of the same jsPerf).

tried to find out if the bottleneck was the split() itself or code evaluation ( http://jsperf.com/literal-space-vs-regex/2 ) my version is "doing more stuff" and runs just slightly slower on Firefox/Chrome, so bottleneck is really the split(). Just to make sure results aren't skewed.. I wrote a post about it a couple weeks ago.

not a big deal, just trying to explain why somebody else may have used the RegExp inside attributes.js and why I consider it "safer".

@rwaldron
rwaldron Sep 28, 2011 Member

Your results seem to favor the literal space as well... I'm not sure what we're arguing here anymore

@millermedeiros
millermedeiros Sep 28, 2011

yes the literal is faster, but it doesn't really matter since it is doing hundreds of thousands of operations per second and code only runs once...

for me it doesn't matter if it's a space, RegExp or plain array as long as it works properly and nobody introduce bugs because of it. I was just trying to show that the RegExp work with multiple spaces, can avoid headaches, speed in this case is irrelevant and it's possibly the reason why someone used it on src/attributes.js..

the link to my jsPerf was just to confirm that result wasn't skewed and my blog post explain what I did to avoid JIT optimizations that invalidate a lot of the perf tests out there.

cheers.

@mathiasbynens mathiasbynens and 2 others commented on an outdated diff Sep 7, 2011
src/manipulation.js
@@ -537,6 +537,23 @@ function findInputs( elem ) {
jQuery.grep( elem.getElementsByTagName("input"), fixDefaultChecked );
}
}
+// Create safe fragments
+function safeFragment( context ) {
+ var nodeNames = (
+ "abbr article aside audio canvas datalist details figcaption figure footer " +
+ "header hgroup mark meter nav output progress section subline summary time video"
@mathiasbynens
mathiasbynens Sep 7, 2011 Contributor

What is subline?

@mathiasbynens
mathiasbynens Sep 7, 2011 Contributor

IMHO jQuery should not get ahead of the official (WHATWG/W3C) specs.

@rwaldron
rwaldron Sep 7, 2011 Member

Eh, I had to google for that. I literally copied this string from Jonathan and Paul's prior work (without double checking each nodeName )

@paulirish
paulirish Sep 7, 2011 Member

My mistake to let it last this far. Let's drop subline.

@mathiasbynens
Contributor

FWIW, I just opened up IE9 to test this; all unit tests pass. \o/

@rwaldron
Member
rwaldron commented Sep 7, 2011

@mathiasbynens oh awesome! thank you :)

@rwaldron

I'll give a try and report back here. Thanks

Sorry, I moved that comment to the diff just before you replied: https://github.com/jquery/jquery/pull/490/files#r116316

@redoPop redoPop commented on an outdated diff Sep 7, 2011
src/manipulation.js
@@ -624,7 +641,11 @@ jQuery.extend({
var tag = (rtagName.exec( elem ) || ["", ""])[1].toLowerCase(),
wrap = wrapMap[ tag ] || wrapMap._default,
depth = wrap[0],
- div = context.createElement("div");
+ div = context.createElement("div"),
+ safe = safeFragment( context );
@redoPop
redoPop Sep 7, 2011

Is it necessary to create a new shim'd fragment each time? Can we pluck the div once we're done and recycle safe in a closure so we don't have to rebuild it?

@aFarkas
Contributor
aFarkas commented Sep 8, 2011

It would be really nice, if the shim would not only create a safeFragment, but also a safeDocument. Something like the following:

var safeFragment = (function() {
    var nodeNames = (
        "abbr article aside audio canvas datalist details figcaption figure footer " +
        "header hgroup mark meter nav output progress section summary time video"
    ).split( " " ),
    safeFrag = document.createDocumentFragment(),
    nodeName;

    while ( nodeNames.length ) {
        nodeName = nodeNames.pop();
        document.createElement(nodeName);
        safeFrag.createElement(nodeName);
    }
    return safeFrag;
})();
@rwaldron
Member
rwaldron commented Sep 8, 2011

@aFarkas while I fully understand what the outcome of that addition is, I intentionally omitted this behaviour based on the goals of the ticket - which are not to provide a shim.

@paulirish
Member

If we did that then the recommendation would be:

If you're using HTML5 elements (in the markup) put jQuery in the head, otherwise leave it at the bottom.

I'm not sure we want to force a 35kish file to the head, regardless. But I don't really have perf numbers on this myself.

@rwaldron
Member
rwaldron commented Sep 8, 2011

@dmethvin and I discussed this at length and agree that adding a "must exist in head" rule to jQuery would be bad.

@aFarkas
Contributor
aFarkas commented Sep 8, 2011

I don't think, that this is the recommendation. It's simply an additional feature, which we get for free. Whether this feature is used directly or the developer has to use Modernizr/iepp additionally is left to the developer. In short: This change, won't break anything, if a developer loads jQuery from the bottom.

And to be here a little bit off topic:
I'm not a real fan of blindly following old performance rules, which where made for IE7. In fact loading 2 15kb js-files is a lot faster in modern browsers (including non-borwsers like IE8), than loading 1 30kb file.

In case of using a script loader, the advantage of loading a script dynamically from head is a lot better than loading it from the bottom. And sometimes loading a bunch of scripts from head is exactly what a developer wants.

@redoPop
redoPop commented Sep 8, 2011

@AFarkas unfortunately, the shim technique needs to be applied before the browser starts rendering the document content, so a non-blocking loading technique (e.g., a script loader) would kill it. :(

@dmethvin
Member
dmethvin commented Sep 8, 2011

This change, won't break anything, if a developer loads jQuery from the bottom.

But it also won't provide HTML5 support in that case. If everyone knew the details of what this is doing and knew how to use it, I think it would be perfectly fine. My worry is that the headline will be "OMG jQuery 1.7 supports HTML5!" and then we'll start getting bugs reported by people who don't load it in the head or who do some other HTML5ish work before jQuery can shim things.

@rwaldron
Member
rwaldron commented Sep 8, 2011

@aFarkas please be sure to review: http://bugs.jquery.com/ticket/6485 and note that @paulirish has updated the original ticket's description to clearly define the goals of the ticket:

EDIT by paulirish.. the scope of this ticket it only to fix #2 and #3. the markup in the document will still require the basic html5shiv/modernizr to adjust.

this fix will correct the assumption that "jquery is broken" because it cannot handle ajax'd in html5 content and the like.

Note also that my pull request is very clear in this: HTML5 Support for innerHTML, clone & style - care was taken to omit "shim/shiv"

@redoPop redoPop commented on an outdated diff Sep 8, 2011
src/manipulation.js
@@ -20,7 +20,21 @@ var rinlinejQuery = / jQuery\d+="(?:\d+|null)"/g,
col: [ 2, "<table><tbody></tbody><colgroup>", "</colgroup></table>" ],
area: [ 1, "<map>", "</map>" ],
_default: [ 0, "", "" ]
- };
+ },
+ safeFragment = (function() {
+ var nodeNames = (
+ "abbr article aside audio canvas datalist details figcaption figure footer " +
+ "header hgroup mark meter nav output progress section summary time video"
+ ).split( " " ),
+ safeFrag = document.createDocumentFragment();
+
+ while ( nodeNames.length ) {
@redoPop
redoPop Sep 8, 2011

while loop should be enclosed in if (safeFrag.createElement) { ... } since only IE supports this.

@paulirish
Member

it's cool. afarkas is just pointing out jquery can optionally make the html5shiv redundant which would be a nice benefit.

as he's the primary author of the html5shiv, he's in a fair place to propose it. :)

I agree with Dave's assertion that we should be clear on what this fixes. Maybe something like...

jQuery will now successfully inject HTML5 sectioning elements into the page. Use Modernizr or html5shiv in the <head> if you use the HTML5 sectioning elements in the original markup of the page

@redoPop redoPop commented on the diff Sep 8, 2011
src/manipulation.js
@@ -626,6 +640,9 @@ jQuery.extend({
depth = wrap[0],
div = context.createElement("div");
+ // Append wrapper element to unknown element safe doc fragment
+ safeFragment.appendChild( div );
@redoPop
redoPop Sep 8, 2011

nice! but now that it's recycling the safeFragment, i noticed the div doesn't actually get removed. i don't think the div itself can be recycled, but around line 680 or so something like safeFragment.removeChild(div); should be added so we don't end up with a heap of used divs loitering around safeFrag.

@redoPop
redoPop Sep 8, 2011

...actually, make that line 650 -- just after div's innerHTML is set: otherwise IE chokes on the depth fixing. :o

@rwaldron
rwaldron Sep 8, 2011 Member

Haha, yeah I was just coming back to leave a note that I had to move it higher. Patch en route

@rwaldron
rwaldron Sep 8, 2011 Member

Scratch that... I just ran this in IE6 and consistently crashed the browser 3 times in a row

@redoPop
redoPop commented Sep 8, 2011

The current script is definitely causing old-IE to recognize HTML5 elements, and apply styles based on class names, but for some reason the browser's still not picking up on CSS that uses tags as a selector. Haven't been able to work out why this is.

@timmywil
Member

Landed in commit 9ecdb24.

@timmywil timmywil closed this Sep 19, 2011
@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@rwaldron rwaldron + timmywil Landing pull request 490. 1.7 HTML5 Support for innerHTML, clone & st…
…yle. Fixes #6485.

More Details:
 - jquery#490
 - http://bugs.jquery.com/ticket/6485
bd01b86
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment