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

Add Imperative Slot API #860

Closed
wants to merge 17 commits into from
39 changes: 35 additions & 4 deletions dom.bs
Original file line number Diff line number Diff line change
Expand Up @@ -2195,6 +2195,10 @@ steps:</p>
<li><p>If the <i>open flag</i> is set and <var>shadow</var>'s <a for=ShadowRoot>mode</a> is
<em>not</em> "<code>open</code>", then return null.</p></li>

<li><p>If <var>shadow</var>'s <a for=ShadowRoot>slot assignment</a> is set to "<code>manual</code>",
return the <a>slot</a> in <var>shadow</var>'s <a for=tree>descendants</a> whose <a>manually assigned nodes</a>
yuzhe-han marked this conversation as resolved.
Show resolved Hide resolved
includes <var>slottable</var>, if any, and null otherwise.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

you want <a for=set>contains</a> instead of includes here I think. It's a set right?

And I guess we're not bothering with tree order and such since there can only be one such slot.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. Yes, contains is correct. I see how the verbiage is being used.
Right, there's can only be one slot because the corresponding HTML spec for HTMLSlotElement::assign() ensures that.


<li><p>Return the first <a>slot</a> in <a>tree order</a> in <var>shadow</var>'s
<a for=tree>descendants</a> whose <a for=slot>name</a> is <var>slottable</var>'s
<a for=slottable>name</a>, if any, and null otherwise.</p></li>
Expand Down Expand Up @@ -2450,7 +2454,8 @@ before a <var>child</var>, with an optional <i>suppress observers flag</i>, run
<li><p>Otherwise, <a for=set>insert</a> <var>node</var> into <var>parent</var>'s
<a for=tree>children</a> before <var>child</var>'s <a for=tree>index</a>.

<li><p>If <var>parent</var> is a <a for=Element>shadow host</a> and <var>node</var> is a
<li><p>If <var>parent</var> is a <a for=Element>shadow host</a> with its <a for=tree>root</a>'s
yuzhe-han marked this conversation as resolved.
Show resolved Hide resolved
<a for=ShadowRoot>slot assignment</a> set to "<code>auto</code>" and <var>node</var> is a
<a>slottable</a>, then <a>assign a slot</a> for <var>node</var>.

<li><p>If <var>parent</var>'s <a for=tree>root</a> is a <a for=/>shadow root</a>, and
Copy link
Member

Choose a reason for hiding this comment

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

Why should this step not be skipped?

Copy link
Author

Choose a reason for hiding this comment

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

This step,
** If parent’s root is a shadow root, and parent is a slot whose assigned nodes is the empty list, then run signal a slot change for parent. **
is to signal slot change event because the slot's fallback content has changed.

For slot assignment "manual", the slot still supports fallback content.

Expand Down Expand Up @@ -2667,8 +2672,16 @@ indicated in the <a for=/>remove</a> algorithm below.

<li><p><a for=set>Remove</a> <var>node</var> from its <var>parent</var>'s <a for=tree>children</a>.

<li><p>If <var>node</var> is <a for=slottable>assigned</a>, then run <a>assign slottables</a> for
<var>node</var>'s <a>assigned slot</a>.
<li>
<p>If <var>node</var> is <a for=slottable>assigned</a>, then:

<ol>
<li><p><p>If <var>parent</var>'s <a for=tree>root</a> is a <a for=/>shadow root</a> with its
yuzhe-han marked this conversation as resolved.
Show resolved Hide resolved
<a for=tree>root</a>'s <a for=ShadowRoot>slot assignment</a> set to "<code>auto</code>",
then <a for=set>remove</a> <var>node</var> from its <a>assigned slot</a>'s <a>manually assigned nodes</a>.

<li><p>Run <a>assign slottables</a> for <var>node</var>'s <a>assigned slot</a>.
Copy link
Member

Choose a reason for hiding this comment

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

Here too I have the feeling we're not skipping enough for the manual case. Same below.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure how much optimization can be done here. Once the assigned node has been removed from slot's manually assigned node, we need to recalculate the slot's assigned nodes and signal slot change event. Both occurs inside assign slottables.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that's fair.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How can this be right? assign slottables would set the list of assigned nodes to slottable matching the slot in tree order. It would mean that we're always re-order slottable in the tree order regardless of in what order they're inserted.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for catching that. I've updated the find-slottables algorithm to preserve the order of manually assigned nodes.

</ol>

<li><p>If <var>parent</var>'s <a for=tree>root</a> is a <a for=/>shadow root</a>, and
<var>parent</var> is a <a>slot</a> whose <a for=slot>assigned nodes</a> is the empty list,
Expand All @@ -2678,6 +2691,11 @@ indicated in the <a for=/>remove</a> algorithm below.
<p>If <var>node</var> has an <a>inclusive descendant</a> that is a <a>slot</a>, then:

<ol>
<li><p>for each <a>slot</a> <var>slot</var> in <var>node</var>'s <a for=tree>inclusive descendants</a>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<li><p>for each <a>slot</a> <var>slot</var> in <var>node</var>'s <a for=tree>inclusive descendants</a>,
<li><p>For each <a>slot</a> <var>slot</var> in <var>node</var>'s <a for=tree>inclusive descendants</a>,

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

if <var>slot</var>'s <a for=tree>root</a> is a <a for=/>shadow root</a> with its <a for=tree>root</a>'s
<a for=ShadowRoot>slot assignment</a> set to "<code>manual</code>", then set <var>slot</var>'s
yuzhe-han marked this conversation as resolved.
Show resolved Hide resolved
<a>manually assigned nodes</a> to an empty list.
yuzhe-han marked this conversation as resolved.
Show resolved Hide resolved

<li><p>Run <a>assign slottables for a tree</a> with <var>parent</var>'s <a for=tree>root</a>.

<li><p>Run <a>assign slottables for a tree</a> with <var>node</var>.
Expand Down Expand Up @@ -5711,11 +5729,13 @@ invoked, must return a new {{DocumentFragment}} <a>node</a> whose <a for=Node>no
[Exposed=Window]
interface ShadowRoot : DocumentFragment {
readonly attribute ShadowRootMode mode;
readonly attribute SlotAssignmentMode slotAssignment;
readonly attribute Element host;
attribute EventHandler onslotchange;
};

enum ShadowRootMode { "open", "closed" };
enum SlotAssignmentMode { "manual", "auto" };
annevk marked this conversation as resolved.
Show resolved Hide resolved
</pre>

<p>{{ShadowRoot}} <a for=/>nodes</a> are simply known as
Expand All @@ -5731,6 +5751,9 @@ It is initially set to false.</p>
<!-- If we ever change this, e.g., add a ShadowRoot object constructor, that would have serious
consequences for innerHTML. -->

<p><a for=/>Shadow roots</a> have an associated <dfn for=ShadowRoot>slot assignment</dfn>
("<code>manual</code>" or "<code>auto</code>").</p>

<p>A <a for=/>shadow root</a>'s <a>get the parent</a> algorithm, given an <var>event</var>, returns
null if <var>event</var>'s <a>composed flag</a> is unset and <a for=/>shadow root</a> is the
<a for=tree>root</a> of <var>event</var>'s <a for=Event>path</a>'s first struct's
Expand All @@ -5748,6 +5771,9 @@ null if <var>event</var>'s <a>composed flag</a> is unset and <a for=/>shadow roo
<dfn for=ShadowRoot export><code>onslotchange</code></dfn> <a>event handler</a>, whose
<a>event handler event type</a> is {{HTMLSlotElement/slotchange}}.

<p>The <dfn attribute for=ShadowRoot><code>slotAssignment</code></dfn> attribute's getter must
return <a>this</a>'s <a for=ShadowRoot>slot assignment</a>.</p>

<hr>

<p>In <dfn export id=concept-shadow-including-tree-order>shadow-including tree order</dfn> is
Expand Down Expand Up @@ -5871,6 +5897,7 @@ interface Element : Node {
dictionary ShadowRootInit {
required ShadowRootMode mode;
boolean delegatesFocus = false;
SlotAssignmentMode slotAssignment = "auto";
};
</pre>

Expand Down Expand Up @@ -6744,6 +6771,9 @@ invoked, must run these steps:
<li><p>Set <var>shadow</var>'s <a for=ShadowRoot>delegates focus</a> to <var>init</var>'s
{{ShadowRootInit/delegatesFocus}}.

<li><p>Set <var>shadow</var>'s <a for=ShadowRoot>slot assignment</a> to <var>init</var>'s
{{ShadowRootInit/slotAssignment}}.

<li><p>Set <a>this</a>'s <a for=Element>shadow root</a> to <var>shadow</var>.

<li><p>Return <var>shadow</var>.
Expand Down Expand Up @@ -10187,7 +10217,8 @@ Xidorn Quan,
Yehuda Katz,
Yoav Weiss,
Yoichi Osato,
Yoshinori Sano, and
Yoshinori Sano,
Yu Han, <!-- yuzhe-han on GitHub --> and
Zack Weinberg
for being awesome!

Expand Down