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.

Sign up for GitHub

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 #966

Merged
merged 8 commits into from
Apr 15, 2021
Merged

Add Imperative Slot API #966

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
055360f
Add updated imperative slot assignment API
mfreed7 Apr 2, 2021
d45e54e
Fix array typo
mfreed7 Apr 2, 2021
eb1e6fc
Add step to remove a node
mfreed7 Apr 6, 2021
7915e2b
Add manual slot assignment, and remove removal step
mfreed7 Apr 9, 2021
a0c69d9
Move weak ref stuff to a note
mfreed7 Apr 9, 2021
cf6dd5a
name -> named
mfreed7 Apr 12, 2021
39f9ba9
a bunch of editorial nits
annevk Apr 14, 2021
dfc9b44
nits
annevk Apr 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 43 additions & 5 deletions dom.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2175,6 +2175,11 @@ Unless stated otherwise it is null. A <a>slottable</a> is
<dfn export for=slottable id=slotable-assigned>assigned</dfn> if its <a>assigned slot</a> is
non-null.</p>

<p>A <a>slottable</a> has an associated
<dfn export for=slottable id=slottable-manual-slot-assignment>manual slot assignment</dfn> (null
or a <a>slot</a>). Unless stated otherwise, it is null. The <a>manual slot assignment</a> is
a weak reference to the slot, such that it can be garbage collected if not referenced elsewhere.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here also, comments appreciated on the "weak pointer" stuff.


<h5 id=finding-slots-and-slotables>Finding slots and slottables</h5>

<p>To <dfn export lt="find a slot|finding a slot">find a slot</dfn> for a given <a>slottable</a>
Expand All @@ -2192,6 +2197,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 "<code>manual</code>",
then return the <a>slot</a> in <var>shadow</var>'s <a for=tree>descendants</a> whose <a>manually assigned nodes</a>
mfreed7 marked this conversation as resolved.
Show resolved Hide resolved
mfreed7 marked this conversation as resolved.
Show resolved Hide resolved
<a for=set>contains</a> <var>slottable</var>, if any, and null otherwise.</p></li>

<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 All @@ -2203,14 +2212,28 @@ for a given <a>slot</a> <var>slot</var>, run these steps:</p>
<ol>
<li><p>Let <var>result</var> be an empty list.</p></li>

<li><p>If <var>slot</var>'s <a for=tree>root</a> is not a <a for=/>shadow root</a>, then return
<li><p>Let <var>root</var> be <var>slot</var>'s <a for=tree>root</a>.</p></li>

<li><p>If <var>root</var> is not a <a for=/>shadow root</a>, then return
<var>result</var>.</p></li>

<li><p>Let <var>host</var> be <var>slot</var>'s <a for=tree>root</a>'s
<a for=DocumentFragment>host</a>.</p></li>
<li><p>Let <var>host</var> be <var>root</var>'s <a for=DocumentFragment>host</a>.</p></li>

<li>
<p>For each <a>slottable</a> <a for=tree>child</a> of <var>host</var>, <var>slottable</var>, in
<p>If <var>root</var>'s <a for=ShadowRoot>slot assignment</a> is "<code>manual</code>",
then:</p>

<ol>
<li><p>Let <var>result</var> be an empty list.</p></li>
mfreed7 marked this conversation as resolved.
Show resolved Hide resolved

<li><p>For each <a>slottable</a> in <var>slot</var>'s <a>manually assigned nodes</a>,
if <a>slottable</a>'s <a for=tree>parent</a> is <var>host</var>, add <a>slottable</a>
to <var>result</var>.</p></li>
</ol>
</li>

<li>
<p>Otherwise, for each <a>slottable</a> <a for=tree>child</a> of <var>host</var>, <var>slottable</var>, in
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess there is a question about what we're gonna do about slots which has non-empty manually assigned nodes inserted inside a shadow tree whose assignment mode is "name". Maybe clearing the manual assignment is the simplest thing to do in that case? It would be very mysterious if manually assigned nodes are completely ignored and it was treated as the default slot and/or it was silently ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I thought we were leaving these as-is. I.e. while that slot is located in a "named" mode shadow root, then "named" mode takes precedence. But nothing, other than re-assigning a node to another slot, removes anything from manually assigned nodes. I.e. .assign() is the only thing with the power to add/remove from manually assigned nodes. And no amount of node movement or slot movement can cause assignments to get cleared. I kind of feel like that's the easiest mental model for developers to have, rather than having to remember never to put a manually assigned slot into the wrong shadow root. It seems like a common pattern might be:

shadow.appendChild(slot);
slot.assign([a,b,c]);
shadow.slotAssignment = 'named';

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the last line of your example. slotAssignment is readonly. I do tend to agree that assign() being the only way to clear seems more straightforward than having insertion in certain trees have side effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sorry, good point. That's not a great example. So you'd prefer this?

const shadow = host.attachShadow({mode: 'open'}); // "named" slot assignment
shadow.appendChild(slot);
slot.assign([a,b,c]); // Has no effect

I suppose I could live with this. It just feels weird that the entire point of all of this conversation was to make the node-to-slot assignments "sticky" across all kinds of tree mutations. But then this would be the one exception - you can move a node or a slot anywhere in any tree and the references will be maintained, except if any stop is ever within a "named"-assignment shadow root.

Given the fact that the node references are unobservable, there will definitely be cases that are a bit hard to understand for developers. E.g. assigning a node to a slot, and then making the node a grandchild (instead of direct child) of the host. We'll need to develop developer tooling to make this situation better. But I don't think adding even more magic is the answer. Again, I'm ok either way, just trying to get this right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, re-reading your comment @annevk, it does sound like you might agree that .assign() should be the only way to clear/change node assignments.

I think this is the only remaining material issue to resolve.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So assign(~) has no side effect and its results get simply ignored? That's a bit strange but all other options are equally strange so maybe it's okay. I guess this might be something we want to solicit some developer feedback.

Copy link
Member

Choose a reason for hiding this comment

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

Oh no, it got confusing. That's not what Mason or I mean. That particular comment was based on a misunderstanding.

assign() always succeeds (and doesn't no-op or throw), as per the current HTML PR. And moving a slot around doesn't affect its manually assigned nodes, as per this PR.

And the answer to your question in this subthread OP is that manually assigned nodes would be ignored in that case as it would be quite esoteric if moving a slot element around is a no-op for manually assigned nodes except if you move it to a shadow tree with named slot assignment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I mean that assign(~) will succeed but it won't have any observable side effect until the slot is moved to another shadow root with manual slot assignment along with the assigned nodes, right?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, indeed. 😊

If we can get developer feedback in a timely fashion it seems reasonable to block on that, but otherwise I'd be inclined to land these PRs and count on the fact that making a change for that particular scenario is without risk and can be made once developers have some more experience with the API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, sounds good to me.

annevk marked this conversation as resolved.
Show resolved Hide resolved
<a>tree order</a>:</p>

<ol>
Expand Down Expand Up @@ -2447,7 +2470,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> whose <a for=/>shadow root</a>'s
<a for=ShadowRoot>slot assignment</a> is "<code>name</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
Expand Down Expand Up @@ -5706,11 +5730,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", "name" };
</pre>

<p>{{ShadowRoot}} <a for=/>nodes</a> are simply known as
Expand All @@ -5729,6 +5755,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>name</code>").</p>
mfreed7 marked this conversation as resolved.
Show resolved Hide resolved

<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 @@ -5746,6 +5775,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
annevk marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -5869,6 +5901,7 @@ interface Element : Node {
dictionary ShadowRootInit {
required ShadowRootMode mode;
boolean delegatesFocus = false;
SlotAssignmentMode slotAssignment = "name";
};
</pre>

Expand Down Expand Up @@ -6753,6 +6786,9 @@ invoked, must run these steps:
"<code>custom</code>", then set <var>shadow</var>'s
<a for=ShadowRoot>available to element internals</a> to true.

<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 @@ -10087,6 +10123,7 @@ Manish Tripathi,
Marcos Caceres,
Mark Miller,
Martijn van der Ven,
Mason Freed,
Mats Palmgren,
Mounir Lamouri,
Michael Stramel,
Expand Down Expand Up @@ -10150,6 +10187,7 @@ Yehuda Katz,
Yoav Weiss,
Yoichi Osato,
Yoshinori Sano,
Yu Han,
Yusuke Abe, and
Zack Weinberg
for being awesome!
Expand Down