Add assignedElements() method to <slot> #2269

cyrilletuzi · 2017-01-16T10:58:11Z

Add assignedElements() method to <slot>, similar to assignedNodes(), but returning Element nodes only.

/acknowledgements.html ( diff )
/scripting.html ( diff )

annevk · 2017-01-16T12:45:25Z

Thank you. I think from the issue it's clear that there's interest from implementers. What we also need is tests. It would be best to add those to https://github.com/w3c/web-platform-tests/tree/master/shadow-dom I think.

annevk

Looks great, mostly nits.

annevk · 2017-01-16T12:46:32Z

source

@@ -58667,6 +58668,9 @@ dictionary <dfn>AssignedNodesOptions</dfn> {
   <dd>Returns <var>slot</var>'s <span>assigned nodes</span>, if any, and <var>slot</var>'s children
   otherwise, and does the same for any <code>slot</code> elements encountered therein, recursively,
   until there are no <code>slot</code> elements left.</dd>
+
+  <dt><var>slot</var> . <code subdfn data-x="dom-slot-assignedElements">assignedElements</code>()</dt>


Needs one more space of indentation.

annevk · 2017-01-16T12:56:19Z

source

@@ -58667,6 +58668,9 @@ dictionary <dfn>AssignedNodesOptions</dfn> {
   <dd>Returns <var>slot</var>'s <span>assigned nodes</span>, if any, and <var>slot</var>'s children
   otherwise, and does the same for any <code>slot</code> elements encountered therein, recursively,
   until there are no <code>slot</code> elements left.</dd>
+
+  <dt><var>slot</var> . <code subdfn data-x="dom-slot-assignedElements">assignedElements</code>()</dt>
+   <dd>Returns <var>slot</var>'s <span>Element</span> <span>assigned nodes</span> only.</dd>


Maybe "Return slot's assigned nodes, limited to elements"? No need to reference the Element class here I think.

Also, you should describe what happens when the parameter is passed as well.

(The reason to not use Element here is because this is non-normative and meant to be a more friendly description of what's going on.)

I first added a description with the parameter too, but as I thought the "Note" section is non normative and just here to help, I chose to keep it light, as it's quite obvious it will do the same as assignedNodes() but just for elements. But tell me if it's better to add this.

Even though it's obvious, it's better to be consistent, since we'll definitely get folks asking why it's missing if it is.

annevk · 2017-01-16T12:58:11Z

source

+
+  <ol>
+   <li><p>If the value of <var>options</var>'s <code data-x="">flatten</code> member is false, then
+   return this element's <span>assigned nodes</span>, filtered to return only <span>Element</span> nodes.</p></li>


<code>Element</code> here and below. Also, we have a line length limit of 100 columns. Also seems to apply below.

cyrilletuzi · 2017-01-16T13:28:40Z

Done.

Should I write the tests too ? If so, should I group them with assignedNodes() tests as it's quite the same method, or should I do specific test groups ?

annevk · 2017-01-16T13:35:01Z

If you could write them that'd be great. Modifying the existing test resource for assignedNodes() seems good, but please make sure you use a distinct test() wrapper so they get their own reported failures.

cyrilletuzi · 2017-01-16T14:55:04Z

Tests done in web-platform-tests/wpt#4541

annevk · 2017-01-16T15:03:01Z

source

+   <dd>Returns <var>slot</var>'s <span>assigned nodes</span> limited to elements, if any, and
+   <var>slot</var>'s children limited to elements otherwise, and does the same for any
+   <code>slot</code> elements encountered therein, recursively, until there are no
+   <code>slot</code> elements left.</dd>


Maybe instead we should say here that it returns the same as slot.assignedNodes({flatten:true}), limited to elements? The current sentence does not make much sense and this might be easier to write.

annevk · 2017-01-16T15:03:57Z

Tests look great, if you could ping this thread once they are reviewed and landed that would help.

TimvdLippe · 2017-12-12T10:57:29Z

Tests have been accepted in web-platform-tests/wpt#8636 🎉

annevk · 2017-12-12T13:20:35Z

Great, unfortunately this PR has conflicts. Would you also be willing to work on a new specification PR @TimvdLippe? If not, I can probably take care of the conflicts sometime this week or next.

@hayatoito for next time, it'd be great to not merge the web-platform-tests PR until we can also update the standard around the same time. Otherwise we might end up confusing people. (I don't think it's a particularly big deal now, since it's a minor addition, but worth keeping in mind.)

TimvdLippe · 2017-12-12T13:22:35Z

@annevk As I am not familiar with this repository nor the original author of this PR, I think you resolving conflicts is the fastest solution 😄 Thanks in advance!

hayatoito · 2017-12-14T02:57:29Z

@annevk I got it. Let me keep it in mind. I thought that the lack of a test was a blocking issue for merging this PR . Let me confirm that a PR is really ready to be merged before merging a test from the next time.

Thank you for taking care of it!

Addressed my own nits

annevk · 2017-12-16T17:35:19Z

Commit:

Add <slot>.assignedElements()

Equivalent to assignedNodes(), limited to returning element nodes.

Tests: https://github.com/w3c/web-platform-tests/pull/8636.

Fixes https://github.com/w3c/webcomponents/issues/602.

@domenic final review?

annevk · 2017-12-16T17:53:03Z

Bugs:

TimvdLippe · 2017-12-17T20:42:20Z

@annevk Awesome, thanks for your help!

domenic

Minor nits

domenic · 2017-12-19T00:20:34Z

source

+   <dd>Returns <var>slot</var>'s <span>assigned nodes</span>, limited to elements.</dd>
+
+   <dt><var>slot</var> . <code data-x="dom-slot-assignedElements">assignedElements</code>({ flatten: true })</dt>
+   <dd>Returns the same as <code data-x="dom-slot-assignedNodes">assignedElements({ flatten: true


s/assignedElements/assignedNodes

domenic · 2017-12-19T00:21:02Z

source

+
+  <ol>
+   <li><p>If the value of <var>options</var>'s <code data-x="">flatten</code> member is false, then
+   return this element's <span>assigned nodes</span>, filtered to return only <code>Element</code>


s/return only/contain only

domenic · 2017-12-19T00:21:21Z

source

+   nodes.</p></li>
+
+   <li><p>Return the result of <span>finding flattened slotables</span> with this element, filtered
+   to return only <code>Element</code> nodes.</p></li>


s/return only/contain only

annevk added the needs tests Moving the issue forward requires someone to write tests label Jan 16, 2017

annevk requested changes Jan 16, 2017

View reviewed changes

cyrilletuzi mentioned this pull request Jan 16, 2017

Add tests for slot.assignedElements() method web-platform-tests/wpt#4541

Closed

annevk previously requested changes Jan 16, 2017

View reviewed changes

annevk added addition/proposal New features or enhancements do not merge yet Pull request must not be merged per rationale in comment topic: shadow Relates to shadow trees (as defined in DOM) and removed needs tests Moving the issue forward requires someone to write tests labels Jan 16, 2017

TimvdLippe mentioned this pull request Dec 8, 2017

Add tests for slot.assignedElements method web-platform-tests/wpt#8636

Merged

cyrilletuzi and others added 5 commits December 16, 2017 11:26

Add assignedElements() method to <slot>

8c5e1e2

Add assignedElements to <slot>

7c2e253

Add assignedElements() method to <slot>

a08cf1d

ack Tim van der Lippe

e2a2cb5

final nit

9faf034

annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Dec 17, 2017

domenic requested changes Dec 19, 2017

View reviewed changes

final nits

b977c09

oops

f2d87e8

domenic approved these changes Dec 19, 2017

View reviewed changes

annevk merged commit b626a63 into whatwg:master Dec 19, 2017

cyrilletuzi deleted the assigned-elements branch December 19, 2017 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add assignedElements() method to <slot> #2269

Add assignedElements() method to <slot> #2269

cyrilletuzi commented Jan 16, 2017 •

edited by pr-preview bot

annevk commented Jan 16, 2017

annevk left a comment

annevk Jan 16, 2017

annevk Jan 16, 2017

annevk Jan 16, 2017

cyrilletuzi Jan 16, 2017

annevk Jan 16, 2017

annevk Jan 16, 2017

cyrilletuzi commented Jan 16, 2017

annevk commented Jan 16, 2017

cyrilletuzi commented Jan 16, 2017

annevk Jan 16, 2017

annevk commented Jan 16, 2017

TimvdLippe commented Dec 12, 2017

annevk commented Dec 12, 2017

TimvdLippe commented Dec 12, 2017

hayatoito commented Dec 14, 2017 •

edited

annevk commented Dec 16, 2017

annevk commented Dec 16, 2017

TimvdLippe commented Dec 17, 2017

domenic left a comment

domenic Dec 19, 2017

domenic Dec 19, 2017

domenic Dec 19, 2017

Add assignedElements() method to <slot> #2269

Add assignedElements() method to <slot> #2269

Conversation

cyrilletuzi commented Jan 16, 2017 • edited by pr-preview bot

annevk commented Jan 16, 2017

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrilletuzi commented Jan 16, 2017

annevk commented Jan 16, 2017

cyrilletuzi commented Jan 16, 2017

Choose a reason for hiding this comment

annevk commented Jan 16, 2017

TimvdLippe commented Dec 12, 2017

annevk commented Dec 12, 2017

TimvdLippe commented Dec 12, 2017

hayatoito commented Dec 14, 2017 • edited

annevk commented Dec 16, 2017

annevk commented Dec 16, 2017

TimvdLippe commented Dec 17, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrilletuzi commented Jan 16, 2017 •

edited by pr-preview bot

hayatoito commented Dec 14, 2017 •

edited