Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from Jan 29, 2017
Merged

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

merged 8 commits into from Jan 29, 2017

Conversation

TechQuery
Copy link
Contributor

@TechQuery TechQuery commented Dec 20, 2016

Summary

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

Related with #3436

Checklist

@jsf-clabot
Copy link

jsf-clabot commented Dec 20, 2016

CLA assistant check
All committers have signed the CLA.

@mention-bot
Copy link

@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
Copy link
Member

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

@@ -143,7 +143,11 @@ jQuery.each( {
return siblings( elem.firstChild );
},
contents: function( elem ) {
return elem.contentDocument || jQuery.merge( [], elem.childNodes );
switch ( elem.nodeName.toLowerCase() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Just if..else, no need in swtich

@@ -291,6 +291,16 @@
<map name="imgmap" id="imgmap">
<area shape="rect" coords="0,0,200,50">
</map>

<template id="template">
Copy link
Member

@markelog markelog Dec 23, 2016

Choose a reason for hiding this comment

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

Please create that html in unit test and then append it

@@ -710,8 +710,8 @@ QUnit.test( "prevUntil([String])", function( assert ) {
} );

QUnit.test( "contents()", function( assert ) {
assert.expect( 12 );
Copy link
Member

Choose a reason for hiding this comment

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

Please create separate test for this

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

jQuery( "body" ).append(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@markelog
Copy link
Member

In which browsers did you test this?

@TechQuery
Copy link
Contributor Author

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

}

if ( jQuery.nodeName( elem, "template" ) ) {
elem = elem.content || elem;
Copy link
Member

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?

"</form>"
);

var c = jQuery( "#template" ).contents();
Copy link
Member

Choose a reason for hiding this comment

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

Probably better to use more verbose variable name

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

iform.add( iform.prev() ).remove();
Copy link
Member

Choose a reason for hiding this comment

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

For what?

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

Choose a reason for hiding this comment

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

body?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@markelog markelog Dec 27, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Ooh, I get it, ok

Copy link
Member

@markelog markelog Dec 27, 2016

Choose a reason for hiding this comment

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

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

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

jQuery( "#qunit-fixture" ).append(
Copy link
Member

Choose a reason for hiding this comment

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

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


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

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

Choose a reason for hiding this comment

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

What we doing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test handling of nodes imported from the template.

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" );
Copy link
Member

Choose a reason for hiding this comment

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

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

}

if ( jQuery.nodeName( elem, "template" ) ) {
elem = elem.content || elem;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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

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();
Copy link
Member

Choose a reason for hiding this comment

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

I suppose we no longer need that line?

"</template>"
);

var contents = jQuery( "#template" ).contents();
Copy link
Member

Choose a reason for hiding this comment

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

That's a bit different what I had in mind, but I guess we can work with 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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! :)

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@@ -743,6 +743,37 @@ QUnit.test( "contents()", function( assert ) {
assert.equal( c[ 0 ].nodeValue, "hi", "Check node,textnode,comment contents is just the one from span" );
} );

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

@timmywil timmywil added this to the 3.2.0 milestone Jan 9, 2017
@markelog markelog merged commit 3e3b09d into jquery:master Jan 29, 2017
@markelog
Copy link
Member

markelog commented Jan 29, 2017

@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
Copy link
Member

mgol commented Feb 1, 2017

@markelog Everything looks good here 👍

timmywil pushed a commit to timmywil/jquery that referenced this pull request Feb 6, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants