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

Imperative shadow DOM distribution API #3534

Closed
domenic opened this issue Mar 6, 2018 · 55 comments
Closed

Imperative shadow DOM distribution API #3534

domenic opened this issue Mar 6, 2018 · 55 comments

Comments

@domenic
Copy link
Member

@domenic domenic commented Mar 6, 2018

Writing down the stuff we talked about with @rniwa, @hayatoito, et al yesterday:

We should add a slotElement.reassign(...nodes) which reassigns the nodes.

How does this interact with the existing declarative distribution API? Maybe it just throws if the name="" attribute is specified at all; that seems nice and simple.

This API is not "perfect" because authors don't have enough hooks to call it as often as you might want. E.g. if you are trying to emulate details/summary, you will use a MutationObserver to watch for child node changes and do slotEl.reassign(theDetailsIFound). This happens at microtask timing, which is later than is done for the browser's native details/summary (people tell me that happens at style recalc timing).

But, it's pretty darn good!! I'm really excited about not putting slot="" attributes everywhere.

Any further thoughts?

@hayatoito
Copy link
Member

@hayatoito hayatoito commented Mar 6, 2018

Thanks for filing a new issue. Yup. I think that is feasible. Let me explore API design and its semantics deeper.

I am also wondering whether there is a use case which imperative API can't address, or not. Please let us know if there is. I hope this kind of imperative API can address the rest of the world.

@annevk
Copy link
Member

@annevk annevk commented Mar 7, 2018

Maybe it just throws if the name="" attribute is specified at all; that seems nice and simple.

That wouldn't address the elements potentially getting slotted elsewhere.

This will also require some careful study of the mutation algorithms (a fair number of which simply reset the assigned nodes).

@hayatoito
Copy link
Member

@hayatoito hayatoito commented Mar 7, 2018

Yup, my concern is that we might have to update a lot of places in DOM Standard, depending on when we should reset imperatively specified "assigned nodes". I am sure we have to reset it somewhere.

It requires careful study.

@domenic
Copy link
Member Author

@domenic domenic commented Mar 7, 2018

Thanks. I am afraid I am not enough of a shadow DOM/DOM spec expert to do that careful study. But, I will try to coordinate folks into answering the question of

I am also wondering whether there is a use case which imperative API can't address, or not. Please let us know if there is. I hope this kind of imperative API can address the rest of the world.

Hopefully if we can answer "yes", then this would be high-priority enough for you experts to help us write the spec?

@hayatoito
Copy link
Member

@hayatoito hayatoito commented Mar 7, 2018

One thing we should decide is how declarative APIs and imperative APIs interact each other. Mixing them is troublesome and would be the cause of a confusion.

One idea to make the situation much simpler is to get "opt-in" from web developers to allow them to use imperative APIs. The scope of 'opt-in' should be a shadow tree.

e.g.
attachShadow({ mode: 'open', slotting: 'manual' /* tentative parameter name */ });

In other words, declarative APIs and imperative APIs should be mutually exclusive in one shadow tree.
We don't mix them in the same shadow tree. That would make the situation much simpler, I think.

@annevk
Copy link
Member

@annevk annevk commented Mar 7, 2018

I don't think that necessarily helps, since if you have another shadow root that's not manual you'll still run into many of the same questions. Either way we'll have to deal with what happens when a node is already assigned.

cc @whatwg/components

@hayatoito
Copy link
Member

@hayatoito hayatoito commented Mar 7, 2018

I'll post a straw-man proposal, based on my idea.

hayatoito added a commit to WICG/webcomponents that referenced this issue Mar 8, 2018
@hayatoito
Copy link
Member

@hayatoito hayatoito commented Mar 8, 2018

I've posted my straw-man proposal here.

https://github.com/w3c/webcomponents/blob/gh-pages/proposals/Imperative-Shadow-DOM-Distribution-API.md

I hope this can capture most use cases, with minimum changes to DOM Standard, HTML Standard, and browser's engines.

@annevk
Copy link
Member

@annevk annevk commented Mar 8, 2018

So what happens if you have two (parallel) host elements and I manually assign children (its slotables) from the first host element to the shadow tree slots of the second? How does that not run into the issues I alluded to earlier?

@hayatoito
Copy link
Member

@hayatoito hayatoito commented Mar 8, 2018

They are never used in other shadow trees. Let me show an example.

host1
├──/shadowroot1 (slotting=manual)
│   └── slot1
└── A

host2
├──/shadowroot2 (slotting=manual)
│   └── slot2
└── B
slot2.assign([A]);
assert(slot2.assignedNodes() == []);

slot1.assign([A]);
assert(slot1.assignedNodes() == [A]);

shadowroot2.append(slot1);
assert(slot1.assignedNodes() == []);

shadowroot1.append(slot1);
assert(slot1.assignedNodes() == [A]);
@annevk
Copy link
Member

@annevk annevk commented Mar 8, 2018

But wasn't the point of the imperative API that you were not restricted on where the elements came from? At least, it sounds like you want them to be restricted to children of the host element? How does this follow from your document?

@hayatoito
Copy link
Member

@hayatoito hayatoito commented Mar 8, 2018

No. The bottom line is that assigned nodes should be the host's children. That shouldn't change. We don't relax this restriction.

@annevk
Copy link
Member

@annevk annevk commented Mar 8, 2018

And manually-assigned-nodes too? And everyone agreed on that? Still unclear why the proposal does not state that though or why assign() succeeds despite failing.

@hayatoito
Copy link
Member

@hayatoito hayatoito commented Mar 8, 2018

I didn't introduce any restriction to manually-assigned-nodes. Any programmer's mistake can be okay there by design. Invalid nodes in manually-assigned-nodes are never selected as assigned nodes. assigned nodes are only observable.

I think this design choice would make the standard and the implementation much simpler.

Anyway, let me state that clearly.

Any alternative ideas are welcome, of course. I would like to hear feedback.

@hayatoito
Copy link
Member

@hayatoito hayatoito commented Mar 8, 2018

I've added some clarification and an example. Thanks!

@rniwa
Copy link
Collaborator

@rniwa rniwa commented Mar 8, 2018

That won't quite work if we allowed slotElement.reassign to select a non-direct-child descendent of a host element in nested shadow trees, or if we allowed some arbitrary node elsewhere in the trees and had two shadow trees.

Consider when the case when (x) is manually assigned of (d). In that case, (d) would belong to both (x) and (y), which shouldn't be allowed.

a (outer host) ---- SR (manual)
  + b                  + slot (x)
  + c (inner host) --- SR (auto)
      + d                 + default slot (y)
      + e 

I think a lot simpler model is to keep track of the slot to which a given node is manually assigned, and forbid declarative slotting from picking that node.

Namely, each node will have an internal slot manually slotted flag, which is initially set to false. Each time slotElement.reassign is invoked (and when the slot is removed from a shadow tree, etc...), we update this flag's value. The existing slotting algorithm would simply ignore any node with this flag set to true.

@rniwa
Copy link
Collaborator

@rniwa rniwa commented Mar 8, 2018

I'll add that we probably don't want to have a mode being set at a shadow root level. details element, for example, wants to use the default slot to get everything but the first summary.

@hayatoito
Copy link
Member

@hayatoito hayatoito commented Mar 8, 2018

Consider when the case when (x) is manually assigned of (d). In that case, (d) would belong to both (x) and (y), which shouldn't be allowed.

I think there is misunderstanding. Even if slotX.assign(d) is called, slotX.assignedNodes() doesn't contain d, according to my proposal. d is never used as assigned nodes of slotX because d is not the host a's child.

Even if the same node is added to manually-assigned-node in more than one different slots, the node should appear at most one slot's assigned nodes in my proposal.

See Example 3, where A is manually assigned to slot1 and slot2, however, A does not show in slot2's assigned nodes.

@caridy
Copy link

@caridy caridy commented Mar 9, 2018

@hayatoito, the proposal is looking good from the perspective of doing the manual allocation, but it is not providing enough information, or end-to-end examples.

My main concern is that with manual, no auto allocation/slotting will be done, which probably mean that you cannot use slotchange event to observe changes in your slots. The question is, how will the developer know when to do the manual allocation? And the answer cannot be: "just use mutation observer".

I think from our side, we will like to have a reliable signal for changes on the light tree so the allocation can happen accordingly. The most dummy example could be:

<fancy-menu>
     <menu-item>one</menu-item>
     <menu-item>two</menu-item>
</fancy-menu>

When adding a new menu item (e.g.: myFancyMenuElement.appendChild(menuItemThreeElement)), we should be able to detect that operation, and take care of the manual slotting on the spot.

/cc @diervo

@domenic
Copy link
Member Author

@domenic domenic commented Mar 9, 2018

Please explain why the answer cannot be mutation observers? They seem perfect for this case.

@JanMiksovsky
Copy link

@JanMiksovsky JanMiksovsky commented Mar 9, 2018

I can envision scenarios in which a developer would like to mix slotting by name and/or default slotting with manual slotting.

Suppose an app has a menu that shows menu items, some of which are conditionally available. Perhaps it supports markup like:

<menu-element>
  <div slot="caption">Menu</div>
  <menu-item show-when="signedout">Create account</menu-item>
  <menu-item show-when="signedin">Account settings</menu-item>
  <menu-item>Help</menu-item>
</menu-element>

And suppose menu-element has a template like:

<template>
  <slot name="caption"></slot>
  <slot name="availableItems"></slot>
</template>

This menu element wants to leverage preexisting support for slotting by name — in this situation, slotting a caption into the caption slot is easily managed by adding name="caption". At the same time, the menu wants to manually assign the appropriate menu items to the availableItems slot based on a condition evaluated at runtime.

Could the proposal be extended to allow, when slotting is manual, nodes to be assigned to named slots as usual? The idea is that named slotting happens first, then the dev can do what they want.

Along the same lines, I could imagine wanting to have a default slot still serve as the default destination even when manual slotting is being used for specific slots. It would be useful for a component to manually pluck the nodes it wants to assign to specific slots, then let everything else fall into the default (unnamed) slot.

In short, rather than having manual mode entirely disable existing behavior, the new imperative API could extend it. As I read the proposal, the new proposed "Find a slot" step could drop the "If shadow's slotting is manual" condition:

  1. [New Step] Return the first slot in shadow’s tree whose manually-assigned-nodes includes slotable, if any, and null otherwise.
  2. Otherwise, return the first slot in shadow’s tree whose name is slotable’s name, if any, and null otherwise. (<= No change)

[Note: the existing text of step 6 above fails to mention the default slot, but probably should.]

If such an accommodation could be found, it might allow the introduction of an imperative slotting API without needing to introduce a slotting mode at all.

(Side note: If it'd be helpful to fork this topic into a separate issue, I can do that. Perhaps someone who can create labels on this repo could set up a label for imperative distribution?)

@hayatoito
Copy link
Member

@hayatoito hayatoito commented Mar 9, 2018

Thanks for the feedback. I don't have enough bandwidth to reply all today, so let me reply in the next week.
I have a plan to add "Alternatives Considered" to the proposal.

@caridy
Our assumption is that users can use MutationObservers.

[Update: I understood that the following explanation is not directly related to @caridy's concern, but let me keep the following explanation, as a side note, because the concept of manually-assigned-nodes is likely to cause a confusion to end-users.]

We still auto-calculate assigned nodes for each slot in a shadow tree, using the information of manually-assigned-nodes users gave for each slot. manually-assigned-nodes is just a hint for an engine. The engine can't trust manually-assigned-nodes as is because manually-assigned-nodes may include an invalid node which can't be used as a member of assigned nodes.
If assigned nodes are changed as a result in some slots, slotchange is fired at the end of microtask timing for the slots even when they are in manual.

@rniwa
Copy link
Collaborator

@rniwa rniwa commented Mar 9, 2018

I think there is misunderstanding. Even if slotX.assign(d) is called, slotX.assignedNodes() doesn't contain d, according to my proposal. d is never used as assigned nodes of slotX because d is not the host a's child.

Okay, then your proposal doesn't satisfy a major use case of the imperative API to select a non-child node of a shadow host. I don't think that's okay. An imperative API should allow slotting of an arbitrarily deep descendent node.

@domenic
Copy link
Member Author

@domenic domenic commented Mar 9, 2018

It would help to have some concrete examples of elements that want to slot a deep descendant. I'm unsure if select/optgroup/option would count here (i.e., would select slot both options and optgroups, or would it slot its children, and then optgroup slots its children).

Nevertheless, I agree it seems like something we should be aiming for, especially if we want to fulfill

I hope this kind of imperative API can address the rest of the world.

@rniwa
Copy link
Collaborator

@rniwa rniwa commented Mar 9, 2018

See WICG/webcomponents#574 for a concrete example.

Just to be clear, we'd be opposed to any imperative API proposal which doesn't support this use case since we see this as one of the primary motivations for having imperative API at all. In fact, the only reason we receded our proposal to support this in declarative syntax was one of Google representatives made an argument that we can support this in imperative API.

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Apr 8, 2020
Prior to this CL, the ordering of the manually assigned nodes is not
preserved. Nodes were assigned in tree-order. In addition, assignment
is not absolute. A slotable node can be assign to multiple slots, and
where it appears is when the assigned slot first appear in a tree-order
traversal, not the last slot the node is assigned to.

This CL does two things. One, the order of manually assigned node is
preserved. Two, assignment is absolute. The implementation uses a
HeapHashMap, candidate_assigned_slot_map_, to keep track of
assignment node -> slot. It uses this map during node assignment to
find if another assigned slot exists. When found, the node is
removed from the previously assigned slot.

Preserving the ordering of manually assigned nodes is done in
HTMLSlotElement::UpdateManuallyAssignedNodesOrdering(). This is called
at the end of SlotAssignment::RecalcAssignment(). RecalcAssignment()
walks the ShadowHost's children in tree-order, so the assigned node
could be out of order from how these nodes were assigned.  I thought
about making a separate function for manually assigned nodes, looping
though assigned nodes instead. But I decided against it at this point
due to the complexity of the recalc function.

For Reference: point 1 in this comment is addressed by this CL.
whatwg/html#3534 (comment)

Bug: 869308
Change-Id: Idc45cb593313b00f13cd5f29df8972bfe246ecce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2103403
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Yu Han <yuzhehan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757574}
pull bot pushed a commit to Yannic/chromium that referenced this issue Apr 9, 2020
Prior to this CL, the ordering of the manually assigned nodes is not
preserved. Nodes were assigned in tree-order. In addition, assignment
is not absolute. A slotable node can be assign to multiple slots, and
where it appears is when the assigned slot first appear in a tree-order
traversal, not the last slot the node is assigned to.

This CL does two things. One, the order of manually assigned node is
preserved. Two, assignment is absolute. The implementation uses a
HeapHashMap, candidate_assigned_slot_map_, to keep track of
assignment node -> slot. It uses this map during node assignment to
find if another assigned slot exists. When found, the node is
removed from the previously assigned slot.

Preserving the ordering of manually assigned nodes is done in
HTMLSlotElement::UpdateManuallyAssignedNodesOrdering(). This is called
at the end of SlotAssignment::RecalcAssignment(). RecalcAssignment()
walks the ShadowHost's children in tree-order, so the assigned node
could be out of order from how these nodes were assigned.  I thought
about making a separate function for manually assigned nodes, looping
though assigned nodes instead. But I decided against it at this point
due to the complexity of the recalc function.

For Reference: point 1 in this comment is addressed by this CL.
whatwg/html#3534 (comment)


Bug: 869308
Change-Id: Idc45cb593313b00f13cd5f29df8972bfe246ecce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2103403
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Yu Han <yuzhehan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757574}
@yuzhe-han
Copy link

@yuzhe-han yuzhe-han commented Apr 10, 2020

As I started implementing this API, the following scenario came up, and I want to make sure we are all in agreement.
Moving a slot within its shadow root, with imperative slot assignment, clears out its assigned nodes.

Ex:
host1
├──/sr (manual-slot-assignment)
│   └── slot1
│   └── slot2
└── A
└── B
slot1.assign([A]);
slot2.assign([B]);

sr.appendChild(slot1);
assert(slot1.assignedNodes() == []);   // assigned nodes are cleared.
assert(slot2.assignedNodes() == [B]);

slot1.assign([A]);
sr.insertBefore(slot1, slot2);
assert(slot1.assignedNodes() == []);   // assigned nodes are cleared.
assert(slot2.assignedNodes() == [B]);

slot2.remove();
assert(slot2.assignedNodes() == []);  // assigned nodes are cleared.

sr.append(slot2);
assert(slot2.assignedNodes() == []);

This behavior looks to be the correct, because both appendChild(node) and insertBefore(node, refNode), runs the adopting steps for the node whereby it is removed from its original parent, https://dom.spec.whatwg.org/#concept-node-adopt. During removal, it triggers the assign slottables algorithm and clears out all the slot's assigned nodes since this slot is no longer a descendant of SR.

This result, however, differs from that of a shadow root with declarative slot assignment. When moving slots around within its shadow root via appendChild() or insertBefore(), assign slotables algorithm is rerun after inserting the node, and the previously assigned nodes gets reassigned again.

I want to double-check that we are in agreement with this behavior and also make sure web developers are aware of this difference between imperative and declarative slot assignment.

@domenic, @annevk, @rniwa, @JanMiksovsky

Thanks.

xeonchen pushed a commit to xeonchen/gecko that referenced this issue Apr 13, 2020
…of its manually assigned nodes., a=testonly

Automatic update from web-platform-tests
Adds ability for slot to preserve order of its manually assigned nodes.

Prior to this CL, the ordering of the manually assigned nodes is not
preserved. Nodes were assigned in tree-order. In addition, assignment
is not absolute. A slotable node can be assign to multiple slots, and
where it appears is when the assigned slot first appear in a tree-order
traversal, not the last slot the node is assigned to.

This CL does two things. One, the order of manually assigned node is
preserved. Two, assignment is absolute. The implementation uses a
HeapHashMap, candidate_assigned_slot_map_, to keep track of
assignment node -> slot. It uses this map during node assignment to
find if another assigned slot exists. When found, the node is
removed from the previously assigned slot.

Preserving the ordering of manually assigned nodes is done in
HTMLSlotElement::UpdateManuallyAssignedNodesOrdering(). This is called
at the end of SlotAssignment::RecalcAssignment(). RecalcAssignment()
walks the ShadowHost's children in tree-order, so the assigned node
could be out of order from how these nodes were assigned.  I thought
about making a separate function for manually assigned nodes, looping
though assigned nodes instead. But I decided against it at this point
due to the complexity of the recalc function.

For Reference: point 1 in this comment is addressed by this CL.
whatwg/html#3534 (comment)

Bug: 869308
Change-Id: Idc45cb593313b00f13cd5f29df8972bfe246ecce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2103403
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Yu Han <yuzhehan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757574}

--

wpt-commits: 0f44bf63f0fd0b9dc883ac65297dd115f3ae9ac5
wpt-pr: 22255
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 13, 2020
…of its manually assigned nodes., a=testonly

Automatic update from web-platform-tests
Adds ability for slot to preserve order of its manually assigned nodes.

Prior to this CL, the ordering of the manually assigned nodes is not
preserved. Nodes were assigned in tree-order. In addition, assignment
is not absolute. A slotable node can be assign to multiple slots, and
where it appears is when the assigned slot first appear in a tree-order
traversal, not the last slot the node is assigned to.

This CL does two things. One, the order of manually assigned node is
preserved. Two, assignment is absolute. The implementation uses a
HeapHashMap, candidate_assigned_slot_map_, to keep track of
assignment node -> slot. It uses this map during node assignment to
find if another assigned slot exists. When found, the node is
removed from the previously assigned slot.

Preserving the ordering of manually assigned nodes is done in
HTMLSlotElement::UpdateManuallyAssignedNodesOrdering(). This is called
at the end of SlotAssignment::RecalcAssignment(). RecalcAssignment()
walks the ShadowHost's children in tree-order, so the assigned node
could be out of order from how these nodes were assigned.  I thought
about making a separate function for manually assigned nodes, looping
though assigned nodes instead. But I decided against it at this point
due to the complexity of the recalc function.

For Reference: point 1 in this comment is addressed by this CL.
whatwg/html#3534 (comment)

Bug: 869308
Change-Id: Idc45cb593313b00f13cd5f29df8972bfe246ecce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2103403
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Yu Han <yuzhehan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757574}

--

wpt-commits: 0f44bf63f0fd0b9dc883ac65297dd115f3ae9ac5
wpt-pr: 22255
domenic pushed a commit to WICG/webcomponents that referenced this issue Apr 14, 2020
Updated based on the TPAC F2F meeting in Sept, 2019. whatwg/html#3534 (comment)
@bathos
Copy link

@bathos bathos commented May 10, 2020

I’ve been playing with this in Chrome Canary with the flag to see if it meets my needs. I think it does, so wanted to share that feedback somewhere (apologies if this isn’t the best place).

I was initially worried that the microtaskiness of MutationObserver might be a problem because I have some API that exposes values which are derived from the current assignment state — and it would be weird if some sync code appends a child + checks one of these methods since it would get the stale state.

However MutationObserver.prototype.takeRecords actually seemed to address this exact problem very well. At the point where observation is attempted, I can ensure the assignments are “flushed” before calculating the result.

In my case, wiring this up required a lot of code! But after messing around further, I was able to abstract it away. ”Declarative imperative slotting” (...) is made possible; e.g. static slots = { menuitems: [ 'xx-menuitem', 'xx-menusep' ] };, where "menuitems" is a slot name and a LitElement-like class manages assignment of elements of those types to the slot with that name.

I suspect "I want elements of these types to be slotted in this slot" is going to be the most common usage by far, so if there is a path to make that simpler to express in the HTML API itself, it’d be awesome, but I appreciate the additional power afforded by the current API design; it’s nice to know I could make things weirder if needed :)

@yuzhe-han
Copy link

@yuzhe-han yuzhe-han commented May 13, 2020

@bathos,
Thanks for trying out this API. Your feedback is greatly appreciated. And I'm happy that it meets your needs.

You mentioned that initially it required a lot of code to wire it up. What was the complexity and how did you simplify it? Do you have some code to share?

I thought as an alternative, instead of providing a sync state check for your component, you could dispatch an async event when the component state changes. Could that made it easier?
Thanks.

@bathos
Copy link

@bathos bathos commented Jul 15, 2020

You mentioned that initially it required a lot of code to wire it up. What was the complexity and how did you simplify it? Do you have some code to share?

I can’t share the code, but I can describe what I’d ended up doing in more detail. There’s an “ElementDefinition” class in play — it’s a kinda meta builder thing, not itself a base class extending HTMLElement. I added support for a slots option to its API. If present, this was expected to be an object of the form { slotname: [ tagname, ... ] }. The generated element’s behavior was augmented in this case to init a mutation observer on its own childlist and its callback performed the imperative slotting behavior according to the “rules” defined by the slots option. The individual element’s own implementation code was already given access to an internal interface (i.e., one created/managed by ElementDefinition), so I extended this with an ensureSlotAssignments method. That method is what would takeRecords; the point being to provide a way for the element to always report truthfully if “asked” something which actually depended on the current state of what has been slotted where.

I thought as an alternative, instead of providing a sync state check for your component, you could dispatch an async event when the component state changes. Could that made it easier?

Well, the point there was that this is not what we wanted, but yes, if we got rid of the accessors that cared about what had been slotted and made them (and everything everywhere else that depended on them in some way) into async methods instead, we would not need to use takeRecords. We would also end up with an unidiomatic API that doesn’t resemble that of any regular element — thankfully things like node.parentNode or selectElement.options don’t evaluate to promises :). But since takeRecords does work, and since the related complexity of MutationObserver can be tucked away in helper code, it seems to have worked out for us in any case.

Another scenario I just recalled where the ability to ensure all assignments were “flushed” synchronously came up in (UI) event handling. In this case, the specific set of applicable keyboard and pointer events depended on whether an element was a leaf-node menuitem or a menuitem which hosted a child menu — which here meant that a <glyf-menu> had been slotted in a <glyf-menuitem>. This handling can’t avoid deciding whether to preventDefault synchronously. I realize it would be pretty difficult to actually notice the state disconnect in most scenarios, but it’d be apparent in imperative code trying to leverage ordinary element APIs. For example, menuitem.append(menu); menuitem.click() should perform the behavior which is applicable when the menuitem has a valid submenu (opens it); that code shouldn’t require a setTimeout or something after appending, IMO.

tl;dr I guess is that using MutationObserver for this by default moves the act of slotting into a queued microtask, which is an awkward difference from what happens for declarative slotting, but because of takeRecords, it is not an insurmountable difference. I care more about the platform providing the capability than about it having an ideal API, since the latter can be corrected on our end but the former cannot be, so given the capability exists, I consider this workable personally.

@trusktr
Copy link

@trusktr trusktr commented Mar 18, 2021

Moving a slot within its shadow root, with imperative slot assignment, clears out its assigned nodes.

I think that makes a lot of sense. If one uses the imperative API, they will have to write procedures manually to re-distribute nodes once a slot is added back into DOM.

If there were a way for DOM to track it, then this feature would need to also be exposed for the author to be able to make decisions based on the state as well. Having something .previousAssignedNodes (because .assignedNodes is empty after removal) would just be more API surface area that serves only as a shortcut for when a slot will be added back to DOM, but this extra API would be something the author could easily do themselves so it wouldn't be super beneficial except for being a small mere convenience.

@rniwa
Copy link
Collaborator

@rniwa rniwa commented Mar 18, 2021

As I started implementing this API, the following scenario came up, and I want to make sure we are all in agreement.
Moving a slot within its shadow root, with imperative slot assignment, clears out its assigned nodes.
...
This behavior looks to be the correct, because both appendChild(node) and insertBefore(node, refNode), runs the adopting steps for the node whereby it is removed from its original parent, https://dom.spec.whatwg.org/#concept-node-adopt. During removal, it triggers the assign slottables algorithm and clears out all the slot's assigned nodes since this slot is no longer a descendant of SR.

This result, however, differs from that of a shadow root with declarative slot assignment. When moving slots around within its shadow root via appendChild() or insertBefore(), assign slotables algorithm is rerun after inserting the node, and the previously assigned nodes gets reassigned again.

Right. I thought I pointed out this issue earlier somewhere but I can't find it. FWIW, this exact same issue came up during the discussion of AOM / element reflection two years ago, and we've come up with a design that works around this so that the relationship will be preserved in cases like this. See #3917

I want to double-check that we are in agreement with this behavior and also make sure web developers are aware of this difference between imperative and declarative slot assignment.

It would be good to re-use the same mechanism element reflection is using since we've just designed that thing, and it would be really regrettable to put this new API behave differently from that.

Another scenario I just recalled where the ability to ensure all assignments were “flushed” synchronously came up in (UI) event handling. In this case, the specific set of applicable keyboard and pointer events depended on whether an element was a leaf-node menuitem or a menuitem which hosted a child menu — which here meant that a <glyf-menu> had been slotted in a <glyf-menuitem>. This handling can’t avoid deciding whether to preventDefault synchronously. I realize it would be pretty difficult to actually notice the state disconnect in most scenarios, but it’d be apparent in imperative code trying to leverage ordinary element APIs. For example, menuitem.append(menu); menuitem.click() should perform the behavior which is applicable when the menuitem has a valid submenu (opens it); that code shouldn’t require a setTimeout or something after appending, IMO.

Hm... menuitem.append(menu); menuitem.click() should work (btw, did you mean menu.append(menuitem); menuitem.click()?) because micro-task checkpoint exists before any click event listener/handler is called.

@annevk
Copy link
Member

@annevk annevk commented Mar 18, 2021

@rniwa I don't think everyone here is up-to-speed on the AOM discussion so if you could spell out more precisely what changes would be needed to the DOM and HTML PRs that would help a lot I think in evaluating whether that's a reasonable change to make.

@rniwa
Copy link
Collaborator

@rniwa rniwa commented Mar 18, 2021

@rniwa I don't think everyone here is up-to-speed on the AOM discussion so if you could spell out more precisely what changes would be needed to the DOM and HTML PRs that would help a lot I think in evaluating whether that's a reasonable change to make.

Okay, so basically the idea is that if an element A points to an element B, then that reference is conceptually permanent. However, it's visible to the script (i.e. treated as if it's been removed) if B is not a shadow-including ancestor of A (e.g. B is in an outer tree or the same tree as A).

This means that even if A was temporarily removed and inserted back, it would continue to point to B as long as A is inserted into back to the same tree or another tree which can see B because A is in an inner tree.

Implementation-wise, this would mean that A will have a weak pointer (e.g. weak_ptr) to B. This works because the only way B is ever accessible from A by scripts is if B is moved back into the same document and/or a script has a direct access to B, in which case B is kept alive as well.

For the purpose of using this in imperative slot assignment, we probably want to add an additional requirement that the assigned relationship is only in effect if the assigned node is a child node of the shadow host of the slot. If not, then that relationship is invisible; i.e. it disappears from the DOM API like assignedNodes (e.g. when the slot is removed from the shadow tree) but re-appears if the condition becomes true again later (e.g. the slot is inserted back into the original shadow tree).

@annevk
Copy link
Member

@annevk annevk commented Mar 18, 2021

Thanks! @domenic @yuzhe-han @mfreed7 does #3534 (comment) seem like reasonable changes to make to the imperative shadow DOM distribution API?

@mfreed7
Copy link
Collaborator

@mfreed7 mfreed7 commented Mar 30, 2021

So I think this does seem like reasonable behavior. In particular because it's not completely clear when slot reassignment happens, and that opaque behavior currently changes depending on where the node is located at the time. This change should make it "just work" if the assigned node is in the correct spot eventually.

I agree with the comment that the layout tree, assignedNodes, etc., should not show/use nodes that aren't a direct child of the shadow host at layout or JS call time.

We currently issue a console warning if, at slot assignment time, the provided node isn't a child of the shadow host. That seemed helpful to developers, otherwise the provided node simply wouldn't get assigned, with no indication of why. If we change this behavior, the question arises - should we still issue this warning? Seems like yes, but thoughts appreciated. (This point obviously doesn't affect the spec.)

@smaug----
Copy link
Collaborator

@smaug---- smaug---- commented Apr 1, 2021

If not, then that relationship is invisible; i.e. it disappears from the DOM API like assignedNodes (e.g. when the slot is removed from the shadow tree) but re-appears if the condition becomes true again later

ok, so we're trying to invent some 'move' semantics. I guess it makes sense here.

@mfreed7 mfreed7 mentioned this issue Apr 2, 2021
3 tasks done
@mfreed7
Copy link
Collaborator

@mfreed7 mfreed7 commented Apr 2, 2021

Ok, I've updated the DOM PR, comments appreciated.

@bathos
Copy link

@bathos bathos commented Apr 3, 2021

Hm... menuitem.append(menu); menuitem.click() should work because micro-task checkpoint exists before any click event listener/handler is called.

Interesting. It seemed like click() dispatched synchronously, like dispatchEvent(), in Chromium, but I’m not sure what the correct behavior is. Does this look wrong to you?:

image

(btw, did you mean menu.append(menuitem); menuitem.click()?)

I did mean menuitem.append(menu) because ... ... that’s how the tree of submenus was modeled. The activation behavior of menuitem changes to “open menu” by virtue of having a descendent menu slotted. Use of the same names as the aria roles may be confusing because in terms of accessible nodes, we have to end up w/ (pseudo code) <role=menuitem/><role=menu/> — siblings rather than parent->child. That happens under the hood in this case, but I’ve gone back and forth on this API because while it makes it clearer in the markup (and is consistent w/ how <ol> gets nested in <li>), it’s weird that the exposed <glyf-menuitem> element doesn’t end up having the role “menuitem”.
@rniwa
Copy link
Collaborator

@rniwa rniwa commented Apr 3, 2021

Hm... menuitem.append(menu); menuitem.click() should work because micro-task checkpoint exists before any click event listener/handler is called.

Interesting. It seemed like click() dispatched synchronously, like dispatchEvent(), in Chromium, but I’m not sure what the correct behavior is. Does this look wrong to you?:

image

Huh, I guess I misremembered how it worked. I guess this is another use case for WICG/webcomponents#809.

@caridy
Copy link

@caridy caridy commented Apr 3, 2021

@rniwa the solution described in #3534 (comment) for this API and for AOM will work very well for us as well! thanks for the explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet