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

[Feature Request] $.fn.contents() supports <template /> #3436

Closed
TechQuery opened this issue Dec 7, 2016 · 9 comments
Closed

[Feature Request] $.fn.contents() supports <template /> #3436

TechQuery opened this issue Dec 7, 2016 · 9 comments
Milestone

Comments

@TechQuery
Copy link
Contributor

@TechQuery TechQuery commented Dec 7, 2016

TODO

$.fn.contents() can return the reference of HTMLTemplateElement.prototype.content, not childNodes.

Such as how <iframe /> is supported --- https://github.com/jquery/jquery/blob/1.12-stable/src/traversing.js#L144

Reference

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template#Attributes

@dmethvin
Copy link
Member

@dmethvin dmethvin commented Dec 8, 2016

I know this "return something different depending on the collection" approach is a trademark of jQuery's early design, but I'm not a fan of continuing that tradition. Especially in this case where you can get the information via $("template").prop("content") which works all the way back to jQuery 1.6. If there was a strong argument for a getter/setter API it might make sense, but I don't think that applies for this case.

@timmywil
Copy link
Member

@timmywil timmywil commented Dec 12, 2016

@dmethvin's comment is very sensible and hard to argue with, but I kinda want to argue with it anyway. This might be worth it. I'd like to see examples of return values.

@mgol
Copy link
Member

@mgol mgol commented Dec 12, 2016

@timmywil one example - with HTML:

<template>
	<div id="template-div0">
		<div id="template-div00"></div>
	</div>
	<div id="template-div1"></div>
	<div id="template-div2"></div>
</template>

if templateNode is a what its name suggests then running:

templateNode.content.childNodes

produces a NodeList with 7 nodes:

[text, div#template-div0, text, div#template-div1, text, div#template-div2, text]
@timmywil
Copy link
Member

@timmywil timmywil commented Dec 19, 2016

Put together http://jsbin.com/mahudek/edit?html,js,output.

templateNode.childNodes
// => NodeList[0]

templateNode.content.childNodes
// => NodeList[7], with text elements

templateNode.content.children
// => HTMLCollection[3]
@timmywil
Copy link
Member

@timmywil timmywil commented Dec 19, 2016

We discussed this one and given that the solution is short and we currently return an empty list, we can fix this. Something like,

...
jQuery.nodeName( elem, "template" ) && elem.content ?
    elem.content.childNodes
...
@timmywil timmywil removed the Needs review label Dec 19, 2016
@timmywil timmywil added this to the 3.2.0 milestone Dec 19, 2016
@timmywil
Copy link
Member

@timmywil timmywil commented Dec 19, 2016

@TechQuery would you like to do the PR?

@mgol
Copy link
Member

@mgol mgol commented Dec 19, 2016

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

@TechQuery
Copy link
Contributor Author

@TechQuery TechQuery commented Dec 20, 2016

@timmywil I'd like to make a new PR for this issue~

But one question: Which branch should I commit to ? 1.12-stable, 2.2-stable or master ?

@mgol
Copy link
Member

@mgol mgol commented Dec 20, 2016

@TechQuery master. We don't maintain versions 1.x & 2.x anymore.

@markelog markelog closed this in 3e3b09d Jan 29, 2017
timmywil added a commit to timmywil/jquery that referenced this issue Feb 6, 2017
TechQuery added a commit to TechQuery/iQuery.js that referenced this issue Jun 7, 2017
【重构】$.makeArray() 充分利用 JS 运行时引擎代码(基于 @ecalf 的研究)
【重构】$.fn.contents() 支持 template 元素(合并特性 jquery/jquery#3436)
【重构】$.dataHash() 核心基于 Crypto 标准 polyfill 重写
【优化】多处实现细节
@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.