Traversing: $.fn.contents() supports HTMLTemplateElement #3462

TechQuery · 2016-12-20T12:55:56Z

Summary

Add support to $.fn.contents() for HTMLTemplateElement.

Related with #3436

Checklist

All authors have signed the CLA at https://contribute.jquery.com/CLA/
New tests have been added to show the fix or feature works
Grunt build and unit tests pass locally with these changes
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com

Resolved #3436

jsf-clabot · 2016-12-20T12:55:56Z

All committers have signed the CLA.

mention-bot · 2016-12-20T12:55:57Z

@TechQuery, thanks for your PR! By analyzing the history of the files in this pull request, we identified @markelog, @timmywil and @jeresig to be potential reviewers.

timmywil · 2016-12-20T17:09:43Z

@TechQuery Thanks! This looks great. However, the form element has unique behavior in that form elements are attached as properties of the form. I could see content being an input name, so if elem were a form, elem.content.childNodes would not return what you expect. Make sense? In other words, I think a jQuery.nodeName check will guard us here.

Ref #3462 (comment)

markelog · 2016-12-23T08:39:26Z

src/traversing.js

 	},
 	contents: function( elem ) {
-		return elem.contentDocument || jQuery.merge( [], elem.childNodes );
+        switch ( elem.nodeName.toLowerCase() ) {


Just if..else, no need in swtich

markelog · 2016-12-23T08:39:52Z

test/index.html

 		<area shape="rect" coords="0,0,200,50">
 	</map>
-
+    <template id="template">


Please create that html in unit test and then append it

markelog · 2016-12-23T08:40:09Z

test/unit/traversing.js

 } );

 QUnit.test( "contents()", function( assert ) {
-	assert.expect( 12 );


Please create separate test for this

@markelog

@markelog Merry X'mas ! :-)

@jeresig

Thanks @jeresig for http://ejohn.org/blog/nodename-case-sensitivity/

markelog · 2016-12-27T10:42:12Z

test/unit/traversing.js

+QUnit.test( "contents() for <template />", function( assert ) {
+	assert.expect( 6 );
+
+    jQuery( "body" ).append(


Not on body but on #qunit-fixture otherwise it will not be cleared

If I append the HTML to #qunit-fixture, should I remove them after my testing?

@TechQuery no, the point of #qunit-fixture is that it gets cleaned up automatically between test runs.

markelog · 2016-12-27T10:42:58Z

In which browsers did you test this?

TechQuery · 2016-12-27T11:51:02Z

@markelog

Chrome

48.0.2564.23
54.0.2840.59 m (64-bit)

Firefox

50.1.0

Internet Explorer

11.0.9600.18449
Document Mode 10 (IE 11 Debugger)
Document Mode 9 (IE 11 Debugger)

markelog · 2016-12-27T12:30:51Z

src/traversing.js

+        }
+
+        if ( jQuery.nodeName( elem, "template" ) ) {
+            elem = elem.content || elem;


That's because of the IE right?

markelog · 2016-12-27T12:32:02Z

test/unit/traversing.js

+        "</form>"
+    );
+
+    var c = jQuery( "#template" ).contents();


Probably better to use more verbose variable name

markelog · 2016-12-27T12:32:46Z

test/unit/traversing.js

+    iform.children().eq( 0 ).remove();
+    assert.equal( iform.contents().length, 3, "Check no conflict with Form shortcuts" );
+
+    iform.add( iform.prev() ).remove();


markelog · 2016-12-27T12:33:18Z

test/unit/traversing.js

+        jQuery( jQuery.map( c, function( node ) {
+            return document.importNode( node, true );
+        } ) )
+    ).appendTo( document.body );


$.fn.clone() doesn't support document.importNode() which child nodes of template needs (as MDN document said)...

The same MDN page says importNode is not supported in IE 9 and we do support IE 9, soooo

https://developer.mozilla.org/en-US/docs/Web/API/Document/importNode

This page says that document.importNode() is an "Initial definition" in DOM Level 2 (which IE 9 had implemented), only the second optional argument is in DOM Level 4.

Ooh, I get it, ok

You probably responding to https://github.com/jquery/jquery/pull/3462/files/#r93926975 comment though?

markelog · 2016-12-27T12:34:34Z

test/unit/traversing.js

+QUnit.test( "contents() for <template />", function( assert ) {
+	assert.expect( 6 );
+
+    jQuery( "#qunit-fixture" ).append(


Sounds easier to create node, assign, append and use through the tests?

Like so –

var template = jQuery("<template id='template'>...")

Note the single quotes inside the string

markelog · 2016-12-27T12:35:39Z

test/unit/traversing.js

+
+    assert.equal( c.find( "span" ).text(), "Hello, Web Component!", "Find span in Template and check its text" );
+
+    jQuery( "<div id=\"templateTest\" />" ).append(


What we doing here?

Test handling of nodes imported from the template.

markelog · 2016-12-27T12:36:33Z

test/unit/traversing.js

+    var c = jQuery( "#template" ).contents();
+    assert.equal( c.length, 6, "Check template element contents" );
+
+    assert.equal( c.find( "span" ).text(), "Hello, Web Component!", "Find span in Template and check its text" );


Sometimes you use uppercase and sometimes lowercase, I suggest to Template -> template, same for form

mgol · 2016-12-27T12:39:08Z

src/traversing.js

+        }
+
+        if ( jQuery.nodeName( elem, "template" ) ) {
+            elem = elem.content || elem;


@TechQuery All places where a specific logic has to be applied because of deficiencies of a browser a support comment needs to be added like this one.

In this case, I'd expect something like that:

// Support: IE 9 - 11 only, iOS 7 only, Android Browser <=4.3 only // Treat the template element as a regular one in browsers that // don't support it.

It's only necessary to mention browsers we still support; see https://jquery.com/browser-support/.

@mgol I was planning to get to support comment realization, but there is also couple considerations with this code, so right now, I think we need to discuss this a bit further

markelog · 2016-12-28T08:54:03Z

test/unit/traversing.js

+    assert.equal( contents.length, 6, "Check cloned nodes of template element contents" );
+
+    assert.equal( contents.filter( "div" ).length, 3, "Count cloned elements from template" );
+    jQuery( "#templateTest" ).remove();


I suppose we no longer need that line?

markelog · 2016-12-28T08:54:53Z

test/unit/traversing.js

+        "</template>"
+    );
+
+    var contents = jQuery( "#template" ).contents();


That's a bit different what I had in mind, but I guess we can work with that

markelog · 2016-12-28T08:55:55Z

src/traversing.js

+        }
+
+        // Support: IE 9 - 11 only, iOS 7 only, Android Browser <=4.3 only
+        // Treat the template element as a regular one in browsers that


Total nitpick, but anyhow - only Treat -> only treat and no dot at the end

Uh... My opinion of #3462 (comment) is that the first line is a title, and the rest lines are description...

(This is my first PR to jQuery such an international OS project, I'm still in the process of learning OS regulation & specification. I'm really sorry to bother you checking these details...)

Uh... My opinion of #3462 (comment) is that the first line is a title, and the rest lines are description...

More a concrete information about affected browsers in a consistent format than a title. But yeah, the rest is just a description of what's going on there.

(This is my first PR to jQuery such an international OS project, I'm still in the process of learning OS regulation & specification. I'm really sorry to bother you checking these details...)

It's absolutely not a problem, ask away! :)

markelog · 2016-12-28T08:57:00Z

src/traversing.js

+        // Treat the template element as a regular one in browsers that
+        // don't support it.
+        if ( jQuery.nodeName( elem, "template" ) ) {
+            elem = elem.content || elem;


I was wondering that if someone define content property on the node, then we would we be screwed, but I we probably don't need to support such case

Before my PR, the code of $.fn.contents() didn't check the constructor of iframe.contentDocument property too...

Yeah, I don't think it's necessary to worry about it at this point. If someone defines the content property on a template element in a non-standard way, they're asking for a problem.

markelog · 2016-12-28T08:57:50Z

Small fixes still needed, after that I will recheck browser tests and if no one else has any comments we can land this

mgol · 2016-12-29T01:02:43Z

test/unit/traversing.js

 	assert.equal( c[ 0 ].nodeValue, "hi", "Check node,textnode,comment contents is just the one from span" );
 } );

+QUnit.test( "contents() for <template />", function( assert ) {


Pasting from #3436 (comment):

Note that we have to be careful to not make the contents stop being inert once running .contents() on a template element wrapped in jQuery so we'll need a test that ensures this does not happen. For example, you could create a test with a <template> tag containing a script and <img> with an onload handler and make sure none of them fires.

I'd prefer a separate test for inertness, btw; something like:

QUnit.test( "contents() for <template /> remains inert", function( assert ) {

<div id="outerBox"> <template> <img src="path/to/image.jpg" onload="alert('OK')" /> </template> </div>

In my opinion, event handler executes or not, it's depended on browser support of <template /> during parsing HTML code.

IE treats <template /> as an HTMLUnknownElement, so <img /> will be in the same document (fragment) of this template, then onload will be executed.

Otherwise, <img /> will be in a DocumentFragment, and onload won't be executed until $('#outerBox template').contents().appendTo('#outerBox').

Yes, I know in older browsers the code will get executed; you should skip the inertness test in those browsers; see this test for an example how to do it.

By treating the contents of the template element not carefully you can lose the inertness; for example if you append template.content to the DOM. jQuery applies some manipulation steps in various APIs so we need tests that ensure that a simple $('template').contents() doesn't break the inertness by itself.

In the tests in the onload for <img> and in the script tag just assign something to globals and test that the assignments didn't happen. You need to register the globals you set via Globals.register to get them cleaned up automatically at the end of the test, here is an example test that uses this pattern.

As for the last test, it uses assert.ok( true, "Something is not supported in this browser" ) pattern that we used before QUnit.skip was available. Now I'd rather expect something like:

QUnit[ "content" in document.createElement( "template" ) ? "test" : "skip" ]( "contents() for <template /> remains inert", function( assert ) { Globals.register( "testScript" ); Globals.register( "testImgOnload" ); /* the rest of the test */ } )();

Let me know if something is not clear.

I confused about it is necessary to assert.ok() when I use QUnit.skip()? Or it's just a placeholder like this example?

assert.ok( true, "String" ) was just a poor man's QUnit.skip before it existed; it's not necessary here. That's what I was trying to communicate in my last comment. :)

mgol · 2016-12-29T12:06:45Z

test/unit/traversing.js

+
+QUnit[ "content" in document.createElement( "template" ) ? "test" : "skip" ](
+	"contents() for <template /> remains inert",
+    2,


Please use the assert.expect syntax here and in the previous test. The one you used here is deprecated and removed in QUnit 2: https://qunitjs.com/upgrade-guide-2.x/#replace-expected-argument-in-qunit-test

markelog · 2017-01-29T22:36:04Z

@TechQuery awesome work! Your patience and hard work are much appreciated.

@mgol it seems all your concerns were addressed, but just in case, could you give another look?

mgol · 2017-02-01T13:22:21Z

@markelog Everything looks good here 👍

Fixes jquerygh-3436 Closes jquerygh-3462

Traversing: $.fn.contents() supports HTMLTemplateElement

6b357e1

Resolved #3436

Traversing: Improve .contents() to avoid conflicts with Form shortcuts

9360b90

Ref #3462 (comment)

markelog requested changes Dec 23, 2016

View reviewed changes

TechQuery added 2 commits December 23, 2016 19:50

Traversing: Optimize code of .contents() as the reviewer required

274224f

@markelog Merry X'mas ! :-)

Traversing: Make .contents() safer for inserting nodes from HTML 5 / XML

4ad96ca

Thanks @jeresig for http://ejohn.org/blog/nodename-case-sensitivity/

markelog approved these changes Dec 27, 2016

View reviewed changes

Traversing: Fix for test of $.fn.contents()

bdf7d35

markelog reviewed Dec 27, 2016

View reviewed changes

src/traversing.js

}

if ( jQuery.nodeName( elem, "template" ) ) {

elem = elem.content || elem;

Copy link

Member

markelog Dec 27, 2016

Choose a reason for hiding this comment

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

That's because of the IE right?

markelog requested changes Dec 27, 2016

View reviewed changes

mgol requested changes Dec 27, 2016

View reviewed changes

Traversing: Review fixes for the code comment & test of $.fn.contents()

3455f11

markelog requested changes Dec 28, 2016

View reviewed changes

mgol requested changes Dec 29, 2016

View reviewed changes

Traversing: Add a test --- contents() for <template /> remains inert

5c181e7

mgol requested changes Dec 29, 2016

View reviewed changes

Traversing: Rewrite the expect argument of .contents() test to QUnit 2

91ea012

timmywil added this to the 3.2.0 milestone Jan 9, 2017

markelog merged commit 3e3b09d into jquery:master Jan 29, 2017

timmywil pushed a commit to timmywil/jquery that referenced this pull request Feb 6, 2017

Traversing: $.fn.contents() supports HTMLTemplateElement

0a7a117

Fixes jquerygh-3436 Closes jquerygh-3462

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019


		assert.equal( c.find( "span" ).text(), "Hello, Web Component!", "Find span in Template and check its text" );

		jQuery( "<div id=\"templateTest\" />" ).append(

Traversing: $.fn.contents() supports HTMLTemplateElement #3462

Traversing: $.fn.contents() supports HTMLTemplateElement #3462

Uh oh!

Conversation

TechQuery commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

jsf-clabot commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Dec 20, 2016

Uh oh!

timmywil commented Dec 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelog Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelog commented Dec 27, 2016

Uh oh!

TechQuery commented Dec 27, 2016

Chrome

Firefox

Internet Explorer

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelog Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelog Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

TechQuery commented Dec 20, 2016 •

edited

Loading

jsf-clabot commented Dec 20, 2016 •

edited

Loading

markelog Dec 23, 2016 •

edited

Loading

markelog Dec 27, 2016 •

edited

Loading

markelog Dec 27, 2016 •

edited

Loading

markelog commented Jan 29, 2017 •

edited

Loading