1409975 - Implement node distribution for shadow tree slots

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Jessica Jong [:jessica] (inactive)]

Relevant spec:
https://dom.spec.whatwg.org/#assigning-slotables-and-slots
https://dom.spec.whatwg.org/#mutation-algorithms

Comment 1

[Jessica Jong [:jessica] (inactive)]

I plan to add the patches for the implementation step by step, I think it'd be easier to implement and review.
For the first step, we'll support `assignedNodes` with `flatten: false` only, that is, we won't support multi-level of slotting or fallback content in the first version.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 1: assignedNodes implementation.

This is mostly based on wchen's patch.

As mentioned in comment 1, I have left out the part of multi-level slotting and fallback content, so this patch is just about .assignedNodes with flatten: false. And, of course, this does not have effect on layout. Just wanted to know if we're on the right track.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

oops, sorry, this wasn't in the review queue, so I forgot this.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8921378 [details] [diff] [review]
(WIP) Part 1: assignedNodes implementation.

First couple of nits, then I'll try to understand what the patch does.

>+ShadowRoot::AddSlot(HTMLSlotElement* aSlot)
>+{
>+  MOZ_ASSERT(aSlot);
>+
>+  nsString name;
nsAutoString


>+ShadowRoot::RemoveSlot(HTMLSlotElement* aSlot)
>+{
>+  MOZ_ASSERT(aSlot);
>+
>+  nsString name;
nsAutoString

>+ShadowRoot::RedistributeElement(Element* aElement, int32_t aNameSpaceID,
>+                                nsAtom* aAttribute,
>+                                const nsAttrValue* aOldValue)
> {
>+  nsIContent* parent = aElement->GetParent();
>+  if (parent && parent == GetHost() && aNameSpaceID == kNameSpaceID_None &&
>+      aAttribute == nsGkAtoms::slot) {
>+    auto* oldSlot = UnassignSlotFor(aElement,
>+      aOldValue ? aOldValue->GetStringValue() : EmptyString());
>+    auto* newSlot = AssignSlotFor(aElement);
Please no auto usage when it isn't clear from the surrounding code what the type is.

>   auto* oldInsertionPoint = RemoveDistributedNode(aElement);
>   auto* newInsertionPoint = DistributeSingleNode(aElement);
this old code has the same issue

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8921378 [details] [diff] [review]
(WIP) Part 1: assignedNodes implementation.



>+HTMLSlotElement::SetAttr(int32_t aNameSpaceID, nsAtom* aName,
>+                         nsAtom* aPrefix, const nsAString& aValue,
>+                         nsIPrincipal* aSubjectPrincipal, bool aNotify)
>+{
>+  bool slotNameChanged = false;
>+  ShadowRoot* containingShadow = GetContainingShadow();
>+  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::name) {
>+    if (containingShadow) {
Weird to call GetContainingShadow() even in cases we don't need it.
If we just called it here, we wouldn't need slotNameChanged at all.

>+      containingShadow->RemoveSlot(this);
>+      slotNameChanged = true;
>+    }
>+  }
>+
>+  nsresult rv = nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix,
>+                                              aValue, aSubjectPrincipal,
>+                                              aNotify);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (slotNameChanged) {
Since we could use containingShadow here.

>+HTMLSlotElement::UnsetAttr(int32_t aNameSpaceID, nsAtom* aAttribute,
>+                           bool aNotify)
>+{
>+  bool slotNameChanged = false;
>+  ShadowRoot* containingShadow = GetContainingShadow();
>+  if (aNameSpaceID == kNameSpaceID_None && aAttribute == nsGkAtoms::name) {
>+    if (containingShadow) {
>+      containingShadow->RemoveSlot(this);
>+      slotNameChanged = true;
>+    }
>+  }
>+
>+  nsresult rv = nsGenericHTMLElement::UnsetAttr(aNameSpaceID,
>+                                                aAttribute, aNotify);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (slotNameChanged) {
>+    // Re-add with empty name attribute.
>+    containingShadow->AddSlot(this);
>+  }
Same stuff here.

But we should not add more SetAttr and UnsetAttr implementations, but instead use Before/AfterSetAttr
(I'm hoping we could make SetAttr and UnsetAttr eventually non-virtual methods, and also, overriding SetAttr and UnsetAttr doesn't
play too well with mutation events)


But yeah, I think this looks reasonable.
Sorry about delay! Please ping me on IRC or send email if I'm being slow at replying to feedback?.

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #5)
> Sorry about delay! Please ping me on IRC or send email if I'm being slow at
> replying to feedback?.

No problem, will ping you if the feeback/review is blocking the work. Thanks :)

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: assignedNodes (flatten: false) implementation, v1.

Addressed review comment 4 and comment 5:
- Use nsAutoString for local variables
- Remove auto usage
- Use Before/AfterSetAttr instead of Set/UnsetAttr

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: let ExplicitChildIterator handle slots, v0.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: assignedNodes (flatten: true) implementation, v0.

With this patch we can pass all the test in wpt shadow-dom/HTMLSlotElement-interface.html and most of the tests in slots.html (tests that fail are related to shadow root open/close mode)

Note that this patch does not handle fallback content yet.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Quick question Jessica, if I read the patches correctly this means that <slot> is treated the same way as <content> or <xbl:children>, that is, not appearing on the flat tree, is that right?

If so, I think this is wrong per spec, slots are supposed to have display: contents but appear on the flat tree, and not making it like that would mean that we get wrong styling (descendants of slots should inherit from the slot, not from its parent).

Comment 11

[Jessica Jong [:jessica] (inactive)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Quick question Jessica, if I read the patches correctly this means that
> <slot> is treated the same way as <content> or <xbl:children>, that is, not
> appearing on the flat tree, is that right?

Yes, according to this algorithm [1] and this example [2], <slot> itself does not appear in the distributed nodes.

> 
> If so, I think this is wrong per spec, slots are supposed to have display:
> contents but appear on the flat tree, and not making it like that would mean
> that we get wrong styling (descendants of slots should inherit from the
> slot, not from its parent).

Does that mean we should not use distributed nodes to generate the flat tree used by layout?


[1] https://dom.spec.whatwg.org/#find-flattened-slotables
[2] https://hayato.io/2016/shadowdomv1/#v1_5

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: assignedNodes (flatten: true) implementation, v0.

Minor fixes.

Comment 13

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #11)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> > Quick question Jessica, if I read the patches correctly this means that
> > <slot> is treated the same way as <content> or <xbl:children>, that is, not
> > appearing on the flat tree, is that right?
> 
> Yes, according to this algorithm [1] and this example [2], <slot> itself
> does not appear in the distributed nodes.
> 
> > 
> > If so, I think this is wrong per spec, slots are supposed to have display:
> > contents but appear on the flat tree, and not making it like that would mean
> > that we get wrong styling (descendants of slots should inherit from the
> > slot, not from its parent).
> 
> Does that mean we should not use distributed nodes to generate the flat tree
> used by layout?
> 
> 
> [1] https://dom.spec.whatwg.org/#find-flattened-slotables
> [2] https://hayato.io/2016/shadowdomv1/#v1_5

Looks like Chromium is working on this too:
https://groups.google.com/a/chromium.org/forum/m/#!msg/blink-dev/5s1nbytTL68/SnnummScBAAJ

We may need to change how our flat tree iterator works, need to dig deeper. We might do it in a separate bug.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jessica Jong [:jessica] from comment #11)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> > Quick question Jessica, if I read the patches correctly this means that
> > <slot> is treated the same way as <content> or <xbl:children>, that is, not
> > appearing on the flat tree, is that right?
> 
> Yes, according to this algorithm [1] and this example [2], <slot> itself
> does not appear in the distributed nodes.
> 
> > 
> > If so, I think this is wrong per spec, slots are supposed to have display:
> > contents but appear on the flat tree, and not making it like that would mean
> > that we get wrong styling (descendants of slots should inherit from the
> > slot, not from its parent).
> 
> Does that mean we should not use distributed nodes to generate the flat tree
> used by layout?

No. As I understand it, we should use distributed nodes, but instead of:

 <parent>
   <distributed-node-1 />
   <distributed-node-2 />
 </parent>

The flattened tree should look like:

 <parent>
   <slot (display: contents)>
     <distributed-node-1 />
     <distributed-node-2 />
   </slot>
 </parent>

The result in terms of how nodes are distributed by layout is the same (because of display: contents), but not in terms of styling (that is, you could use <slot style="color: green"> and the distributed nodes should be green).

Comment 15

[Jessica Jong [:jessica] (inactive)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> No. As I understand it, we should use distributed nodes, but instead of:
> 
>  <parent>
>    <distributed-node-1 />
>    <distributed-node-2 />
>  </parent>
> 
> The flattened tree should look like:
> 
>  <parent>
>    <slot (display: contents)>
>      <distributed-node-1 />
>      <distributed-node-2 />
>    </slot>
>  </parent>
> 
> The result in terms of how nodes are distributed by layout is the same
> (because of display: contents), but not in terms of styling (that is, you
> could use <slot style="color: green"> and the distributed nodes should be
> green).

But would distributed nodes work when there is muliple level of distribution?
Take this [1] as an example. When we get to "child-slot", its distributed nodes is the <span> only, so "parent-slot" will be missing in the tree.


[1] https://www.polymer-project.org/2.0/docs/devguide/shadow-dom#multi-level-distribution

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jessica Jong [:jessica] from comment #15)
> But would distributed nodes work when there is muliple level of distribution?
> Take this [1] as an example. When we get to "child-slot", its distributed
> nodes is the <span> only, so "parent-slot" will be missing in the tree.
> 
> 
> [1]
> https://www.polymer-project.org/2.0/docs/devguide/shadow-dom#multi-level-
> distribution

As I understand it, the distributed nodes of <child-slot> is [<parent-slot>], and the distributed nodes of <parent-slot> is [<span>], so the flat tree would include them, right?

That is:

 <child-slot>
  <parent-slot>
   <span />
  </parent-slot>
 </child-slot>

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

right, that is my understanding too.

Comment 18

[Jessica Jong [:jessica] (inactive)]

Hmm, I think that is "assigned nodes". For "distributed nodes", when a slot is found, it will recursively find the distributed node until it is not a slot. See this example [1] and the spec [2] for finding flattened slotables. Or am I misunderstanding something here?


[1] https://hayato.io/2016/shadowdomv1/#v1_5
[2] https://dom.spec.whatwg.org/#find-flattened-slotables

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jessica Jong [:jessica] from comment #18)
> Hmm, I think that is "assigned nodes". For "distributed nodes", when a slot
> is found, it will recursively find the distributed node until it is not a
> slot. See this example [1] and the spec [2] for finding flattened slotables.
> Or am I misunderstanding something here?

That looks about right, sorry for the confusion. In any case, what should determine the flattened tree for layout are the assigned nodes, not the distributed nodes.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

One issue is that DOM spec doesn't talk about distribution, but just about flattened slottables.
And yes, slots aren't added to flattened slottables list, is my reading too.

Comment 21

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: let ExplicitChildIterator handle slots, v1.

I'm hitting some assertions when running try, so changing this to a simpler way.

Comment 22

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: assignedNodes (flatten: true) implementation with fallback content, v0.

Comment 23

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: tests expectations, v0.

Comment 24

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8926789 [details] [diff] [review]
Part 4: assignedNodes (flatten: true) implementation with fallback content, v0.

We should keep in mind two things when distributed nodes change: if parent is a shadow host and if parent is a slot (with no assigned nodes), the change should be reprojected.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8924875 [details] [diff] [review]
Part 1: assignedNodes (flatten: false) implementation, v1.

>+  if (oldSlot && oldSlot != currentSlot) {
>+    // Move assigned nodes from old slot to new slot.
>+    nsTArray<RefPtr<nsINode>>& assignedNodes = oldSlot->AssignedNodes();
>+    while (assignedNodes.Length() > 0) {
>+      nsINode* assignedNode = assignedNodes[0];
>+
>+      oldSlot->RemoveAssignedNode(assignedNode);
>+      currentSlot->AppendAssignedNode(assignedNode);
>+      // TODOv1: we may need to call distribute on parent shadows
Not sure I understand this comment, but perhaps some latter patches explain. 

>+    }
>+  } else {
>+    // Otherwise add appropriate nodes to this slot from the host.
>+    for (nsIContent* child = GetHost()->GetFirstChild();
>+         child;
>+         child = child->GetNextSibling()) {
>+      nsAutoString slotName;
>+      child->GetAttr(kNameSpaceID_None, nsGkAtoms::slot, slotName);
>+      if (child->IsSlotable() && slotName.Equals(name)) {
>+        currentSlot->AppendAssignedNode(child);
>+        // TODOv1: we may need to call distribute on parent shadows
Again, not sure why.

>+const HTMLSlotElement*
>+ShadowRoot::AssignSlotFor(nsIContent* aContent)
>+{
>+  // Find the slot to which the content belongs.
>+  HTMLSlotElement* slot = nullptr;
>+  nsAutoString slotName;
>+  aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::slot, slotName);
Shouldn't you check that there is slot attribute at all, and possibly return early if there is.
Something like
if (!aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::slot, slotName)) {
  return;
}

>+  nsTArray<HTMLSlotElement*>* slots = mSlotMap.Get(slotName);
...since hashtable operations are slow
>+  if (aChild->IsSlotable() && aContainer == GetHost()) {
>+    nsAutoString slotName;
>+    aChild->GetAttr(kNameSpaceID_None, nsGkAtoms::slot, slotName); 
>+    if (UnassignSlotFor(aChild, slotName)) {

Perhaps move GetAttr to that if check. if (aChild->GetAttr() && UnassignSlotFor())

>+nsresult
>+HTMLSlotElement::AfterSetAttr(int32_t aNameSpaceID, nsAtom* aName,
>+                              const nsAttrValue* aValue,
>+                              const nsAttrValue* aOldValue,
>+                              nsIPrincipal* aSubjectPrincipal,
>+                              bool aNotify)
>+{
>+
>+  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::name) {
>+    if (ShadowRoot* containingShadow = GetContainingShadow()) {
>+      containingShadow->AddSlot(this);
>+    }
>+  }
Check that aValue is non-null. No need to do work if there isn't attribute at all.



>+void
>+HTMLSlotElement::AppendAssignedNode(nsINode* aNode)
>+{
>+  if (mAssignedNodes.Length() == 0) {
>+    mAssignedNodes.Clear();
>+    // TOOD: clear distributed node list.
>+
>+    for (nsIContent* child = nsINode::GetFirstChild();
>+         child;
>+         child = child->GetNextSibling()) {
>+      nsTArray<nsIContent*>& destInsertionPoint = child->DestInsertionPoints();
>+      destInsertionPoint.Clear();
>+    }
>+  }
>+
>+  mAssignedNodes.AppendElement(aNode);
>+  aNode->AsContent()->SetAssignedSlot(this);
>+
>+  // TODO: insert the node into the distributed node list and handle the case of
>+  // multi-level slotting.
>+
>+  nsTArray<nsIContent*>& destInsertionPoints =
>+    aNode->AsContent()->DestInsertionPoints();
>+  destInsertionPoints.AppendElement(this);
Why we still need all this DestInsertionPoints stuff?
Isn't that for v0?

>+void
>+HTMLSlotElement::RemoveAssignedNode(nsINode* aNode)
>+{
>+  mAssignedNodes.RemoveElement(aNode);
>+  aNode->AsContent()->SetAssignedSlot(nullptr);
>+
>+  // TODO: distribute fallback content and handle the case of multi-level
>+  // slotting.
>+
>+  ShadowRoot::RemoveDestInsertionPoint(this,
>+    aNode->AsContent()->DestInsertionPoints());
(Why do we need destination insertion point stuff still)

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8926787 [details] [diff] [review]
Part 2: let ExplicitChildIterator handle slots, v1.

> class MatchedNodes {
> public:
>   explicit MatchedNodes(HTMLContentElement* aInsertionPoint)
>-    : mIsContentElement(true), mContentElement(aInsertionPoint) {}
>+    : mIsContentElement(true), mIsSlotElement(false),
>+      mContentElement(aInsertionPoint) {}
>+
>+  explicit MatchedNodes(HTMLSlotElement* aInsertionPoint)
>+    : mIsContentElement(false), mIsSlotElement(true),
>+      mSlotElement(aInsertionPoint) {}
> 
>   explicit MatchedNodes(XBLChildrenElement* aInsertionPoint)
>-    : mIsContentElement(false), mChildrenElement(aInsertionPoint) {}
>+    : mIsContentElement(false), mIsSlotElement(false),
>+      mChildrenElement(aInsertionPoint) {}
> 
>   uint32_t Length() const
>   {
>-    return mIsContentElement ? mContentElement->MatchedNodes().Length()
>-                             : mChildrenElement->InsertedChildrenLength();
>+    if (mIsContentElement) {
>+      return mContentElement->MatchedNodes().Length();
>+    } else if (mIsSlotElement) {
>+      return mSlotElement->DistributedNodes().Length();

So this makes slot element to behave similarly to content element. But they shouldn't, as emilio says. slot element stays there in the layout tree


I filed bug 1418002. Hopefully that will simplify the existing code a bit.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8924909 [details] [diff] [review]
Part 3: assignedNodes (flatten: true) implementation, v0.

So I think this needs quite some changes once slot stays there in the "flattened" tree the normal way, and doesn't get hidden.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8926789 [details] [diff] [review]
Part 4: assignedNodes (flatten: true) implementation with fallback content, v0.

Hmm, distribution looks weird here too. Shouldn't we just use the child nodes of a slot, and slots themselves are slotable, so the "redistribution" happens automatically

Comment 29

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #25)
> Comment on attachment 8924875 [details] [diff] [review]
> Part 1: assignedNodes (flatten: false) implementation, v1.
> 
> >+  if (oldSlot && oldSlot != currentSlot) {
> >+    // Move assigned nodes from old slot to new slot.
> >+    nsTArray<RefPtr<nsINode>>& assignedNodes = oldSlot->AssignedNodes();
> >+    while (assignedNodes.Length() > 0) {
> >+      nsINode* assignedNode = assignedNodes[0];
> >+
> >+      oldSlot->RemoveAssignedNode(assignedNode);
> >+      currentSlot->AppendAssignedNode(assignedNode);
> >+      // TODOv1: we may need to call distribute on parent shadows
> Not sure I understand this comment, but perhaps some latter patches explain. 
> 
> >+    }
> >+  } else {
> >+    // Otherwise add appropriate nodes to this slot from the host.
> >+    for (nsIContent* child = GetHost()->GetFirstChild();
> >+         child;
> >+         child = child->GetNextSibling()) {
> >+      nsAutoString slotName;
> >+      child->GetAttr(kNameSpaceID_None, nsGkAtoms::slot, slotName);
> >+      if (child->IsSlotable() && slotName.Equals(name)) {
> >+        currentSlot->AppendAssignedNode(child);
> >+        // TODOv1: we may need to call distribute on parent shadows
> Again, not sure why.

This and above are for multi-level slotting.

> 
> >+const HTMLSlotElement*
> >+ShadowRoot::AssignSlotFor(nsIContent* aContent)
> >+{
> >+  // Find the slot to which the content belongs.
> >+  HTMLSlotElement* slot = nullptr;
> >+  nsAutoString slotName;
> >+  aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::slot, slotName);
> Shouldn't you check that there is slot attribute at all, and possibly return
> early if there is.
> Something like
> if (!aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::slot, slotName)) {
>   return;
> }

If element does not have a slot attribute, it may distribute to the default slot, which is the first slot element without a name, if exists. So, we can reuse the same code. I can add some comment for this.

> 
> >+  nsTArray<HTMLSlotElement*>* slots = mSlotMap.Get(slotName);
> ...since hashtable operations are slow
> >+  if (aChild->IsSlotable() && aContainer == GetHost()) {
> >+    nsAutoString slotName;
> >+    aChild->GetAttr(kNameSpaceID_None, nsGkAtoms::slot, slotName); 
> >+    if (UnassignSlotFor(aChild, slotName)) {
> 
> Perhaps move GetAttr to that if check. if (aChild->GetAttr() &&
> UnassignSlotFor())

We may still need to unassign it from default slot.

> 
> >+nsresult
> >+HTMLSlotElement::AfterSetAttr(int32_t aNameSpaceID, nsAtom* aName,
> >+                              const nsAttrValue* aValue,
> >+                              const nsAttrValue* aOldValue,
> >+                              nsIPrincipal* aSubjectPrincipal,
> >+                              bool aNotify)
> >+{
> >+
> >+  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::name) {
> >+    if (ShadowRoot* containingShadow = GetContainingShadow()) {
> >+      containingShadow->AddSlot(this);
> >+    }
> >+  }
> Check that aValue is non-null. No need to do work if there isn't attribute
> at all.

Same as above.

> 
> 
> 
> >+void
> >+HTMLSlotElement::AppendAssignedNode(nsINode* aNode)
> >+{
> >+  if (mAssignedNodes.Length() == 0) {
> >+    mAssignedNodes.Clear();
> >+    // TOOD: clear distributed node list.
> >+
> >+    for (nsIContent* child = nsINode::GetFirstChild();
> >+         child;
> >+         child = child->GetNextSibling()) {
> >+      nsTArray<nsIContent*>& destInsertionPoint = child->DestInsertionPoints();
> >+      destInsertionPoint.Clear();
> >+    }
> >+  }
> >+
> >+  mAssignedNodes.AppendElement(aNode);
> >+  aNode->AsContent()->SetAssignedSlot(this);
> >+
> >+  // TODO: insert the node into the distributed node list and handle the case of
> >+  // multi-level slotting.
> >+
> >+  nsTArray<nsIContent*>& destInsertionPoints =
> >+    aNode->AsContent()->DestInsertionPoints();
> >+  destInsertionPoints.AppendElement(this);
> Why we still need all this DestInsertionPoints stuff?
> Isn't that for v0?

This is stil used in nsIContent::GetFlattenedTreeParentNodeInternal [1].

> 
> >+void
> >+HTMLSlotElement::RemoveAssignedNode(nsINode* aNode)
> >+{
> >+  mAssignedNodes.RemoveElement(aNode);
> >+  aNode->AsContent()->SetAssignedSlot(nullptr);
> >+
> >+  // TODO: distribute fallback content and handle the case of multi-level
> >+  // slotting.
> >+
> >+  ShadowRoot::RemoveDestInsertionPoint(this,
> >+    aNode->AsContent()->DestInsertionPoints());
> (Why do we need destination insertion point stuff still)

Same as above.

[1] https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/dom/base/FragmentOrElement.cpp#247

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

But shouldn't nsExtendedDOMSlots::mAssignedSlot tell you whether you've been assigned to a slot, and then you can iterate from there through slot elements' mAssignedSlot.
Well, the flattened tree parent is always the slot, and that slot may have been then slotted underneath some other slot. This is also about keeping slots in the flattened tree.

Comment 31

[Jessica Jong [:jessica] (inactive)]

Per discussion offline, we 'll try to maintain only assigned nodes, and not flattened assigned nodes (or distributed nodes). This is due to the following reason:
If we are going to include <slot> in the flat tree, then child iterator will make use of assigned nodes and not flattened assigned nodes. That means, there isn't really a place that needs flattened assigned nodes (except calling slot.assignedNodes({ flatten: true }) directly. So, we can calculate flattened assigned nodes only when this function is called. The best part is that the overall implementation will be much simpler.

There are still some places in the exisiting code that needs to be modified, they are parts that use DestInsertionPoints() [1][2]. Since we don't calculate flattened assigned nodes anymore, we can not set DestInsertionPoints(). However, we can find the final insertion point by using .assignedSlot (recursively, until slot has no .assignedSlot) instead.

[1] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/dom/base/FragmentOrElement.cpp#247
[2] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/dom/base/FragmentOrElement.cpp#1044

Comment 32

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: assignedNodes implementation, v0.

Since the implementation has become simpler, I've reorganized the patches. This patch adds the implementation for assignedNodes, with flatten true or false.

(I did not apply bug 1418002 yet)

Comment 33

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: Include slots in the flat tree.

(This is just a WIP patch)

This fixes ChildIterator/FlattenedChildIterator and GetFlattenedTreeParentNodeInternal, but not GetEventTargetParent yet.

I verified some of the basic cases and it seems to be working/rendering well.

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8931262 [details] [diff] [review]
(WIP) Part 2: Include slots in the flat tree.

Review of attachment 8931262 [details] [diff] [review]:
-----------------------------------------------------------------

Over-all it looks reasonable. Perhaps should be cleaned up a bit, but... thanks a lot Jessica!

You may want to add (in this or in a followup patch) this rule to the layout/style/res/ua.css:

slot { display: contents; }

Also, this has tests I assume? :)

::: dom/base/ChildIterator.cpp
@@ +81,5 @@
> +        mChild = assignedNodes[mIndexInInserted++]->AsContent();
> +        return mChild;
> +      }
> +
> +      mIndexInInserted = 0;

I'd avoid to set this back to zero, otherwise calling `GetNextChild()` can start returning non-null after it returns null once, which is unexpected, although I don't think it's used that much.

@@ +106,5 @@
>    } else if (mIsFirst) {  // at the beginning of the child list
> +
> +    if (mIsParentSlot) {
> +      HTMLSlotElement* parent =
> +        HTMLSlotElement::FromContent(const_cast<nsIContent*>(mParent));

Why do you need to const_cast it? AssignedNodes() should probably have a const version that returns a const reference to the array (perhaps it should even only have that version).

::: dom/base/ChildIterator.h
@@ +53,5 @@
>        mIsFirst(aOther.mIsFirst),
> +      mIndexInInserted(aOther.mIndexInInserted)
> +  {
> +    mIsParentSlot =
> +      mParent->IsHTMLElement(nsGkAtoms::slot) ? true : false;

There's no need for ? true : false. Just:

mIsParentSlot = mParent->IsHTMLElement(nsGkAtoms::slot);

Actually, maybe it's better to just check HTMLSlotElement::FromContent in the iterator.

::: dom/base/FragmentOrElement.cpp
@@ +185,5 @@
>  
> +nsIContent*
> +nsIContent::GetFlattenedTreeParentForSlotable() const
> +{
> +  if (!IsSlotable()) {

Given a non-slottable element won't have an assigned slot anyway, this check doesn't seem necessary? This leaves all comments and processing instruction out of the frame tree, and while it in practice may not matter, seems conceptually wrong.

@@ +279,5 @@
>      parent = destInsertionPoints->LastElement()->GetParent();
>      MOZ_ASSERT(parent);
> +    */
> +    // This works for Shadow DOM v1 only!
> +    parent = GetFlattenedTreeParentForSlotable();

Just:

```
return GetFlattenedTreeParentForSlotable();
```

It will never return a shadow root anyway.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #34)
> Given a non-slottable element won't have an assigned slot anyway, this check
> doesn't seem necessary? This leaves all comments and processing instruction
> out of the frame tree, and while it in practice may not matter, seems
> conceptually wrong.

Err, I meant flattened tree here. And it may not matter because we don't create frames for those anyway.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8931262 [details] [diff] [review]
(WIP) Part 2: Include slots in the flat tree.


>+  // When mIsParentSlot is set, mChild is always set to the current child, not
>+  // matter mChild is an assigned node or a fallback content.
nit. . after child. and then It does not matter whether mChild ...
Something like that.

>+  if (mIsParentSlot) {
>+    return mChild;
>+  }
>+
>+  // Flag set when parent is slot, in this case, the children will either be be
>+  // the assigned nodes or the direct children, if assigned nodes is empty.
>+  bool mIsParentSlot;
We could also just store HTMLSlotElement* mParentAsSlot here. 




>+nsIContent*
>+nsIContent::GetFlattenedTreeParentForSlotable() const
>+{
>+  if (!IsSlotable()) {
>+    return nullptr;
>+  }
If you change this method the way emilio suggests, the name shouldn't contain 'Slotable'


>+  HTMLSlotElement* slotEl = HTMLSlotElement::FromContent(aContent);
>+  if (slotEl && slotEl->GetContainingShadow()) {
>+    // Children of a slot insertion point are distributed to the
>+    // slot if it does not have any assigned nodes (fallback content).
>+    return slotEl->AssignedNodes().IsEmpty();
>+  }
I guess this matches what we do with v0, though I wouldn't call fallback content 'distributed'

Looks rather simple to me :) I like.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8931258 [details] [diff] [review]
Part 1: assignedNodes implementation, v0.

>-ShadowRoot::RedistributeElement(Element* aElement)
>+ShadowRoot::RedistributeElement(Element* aElement, int32_t aNameSpaceID,
>+                                nsAtom* aAttribute,
>+                                const nsAttrValue* aOldValue)
It would be nice to rename this method to something like... MaybeReassignElement or so, at least eventually

>+HTMLSlotElement::ClearAssignedNodes()
>+{
>+  for (uint32_t i = 0; i < mAssignedNodes.Length(); i++) {
>+    ShadowRoot::RemoveDestInsertionPoint(this,
>+      mAssignedNodes[i]->AsContent()->DestInsertionPoints());
Why we need to call RemoveDestInsertionPoint? Is this a leftover from previous version of the patch?

Comment 38

[Jessica Jong [:jessica] (inactive)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #34)
> Comment on attachment 8931262 [details] [diff] [review]
> (WIP) Part 2: Include slots in the flat tree.
> 
> Review of attachment 8931262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Over-all it looks reasonable. Perhaps should be cleaned up a bit, but...
> thanks a lot Jessica!

Yes, this is just a draft, will cleanup the patch before asking for review. And I was hoping this could be on top of bug 1418002.

> 
> You may want to add (in this or in a followup patch) this rule to the
> layout/style/res/ua.css:
> 
> slot { display: contents; }

Sure, will add it in this one.

> 
> Also, this has tests I assume? :)

About this.. can you point to me how to test that slots are actually in the flat tree? since it's not rendered, should I do something like add style to it and see if it affects its children?

> 
> ::: dom/base/ChildIterator.cpp
> @@ +81,5 @@
> > +        mChild = assignedNodes[mIndexInInserted++]->AsContent();
> > +        return mChild;
> > +      }
> > +
> > +      mIndexInInserted = 0;
> 
> I'd avoid to set this back to zero, otherwise calling `GetNextChild()` can
> start returning non-null after it returns null once, which is unexpected,
> although I don't think it's used that much.

Right, I will just set mChild to nullptr and return;

> 
> @@ +106,5 @@
> >    } else if (mIsFirst) {  // at the beginning of the child list
> > +
> > +    if (mIsParentSlot) {
> > +      HTMLSlotElement* parent =
> > +        HTMLSlotElement::FromContent(const_cast<nsIContent*>(mParent));
> 
> Why do you need to const_cast it? AssignedNodes() should probably have a
> const version that returns a const reference to the array (perhaps it should
> even only have that version).

That's just me being lazy :/
Will make AssignedNodes() const instead.

> 
> ::: dom/base/ChildIterator.h
> @@ +53,5 @@
> >        mIsFirst(aOther.mIsFirst),
> > +      mIndexInInserted(aOther.mIndexInInserted)
> > +  {
> > +    mIsParentSlot =
> > +      mParent->IsHTMLElement(nsGkAtoms::slot) ? true : false;
> 
> There's no need for ? true : false. Just:
> 
> mIsParentSlot = mParent->IsHTMLElement(nsGkAtoms::slot);
> 
> Actually, maybe it's better to just check HTMLSlotElement::FromContent in
> the iterator.

I think I'll just store HTMLSlotElement* mParentAsSlot, as Olli suggested above.

> 
> ::: dom/base/FragmentOrElement.cpp
> @@ +185,5 @@
> >  
> > +nsIContent*
> > +nsIContent::GetFlattenedTreeParentForSlotable() const
> > +{
> > +  if (!IsSlotable()) {
> 
> Given a non-slottable element won't have an assigned slot anyway, this check
> doesn't seem necessary? This leaves all comments and processing instruction
> out of the frame tree, and while it in practice may not matter, seems
> conceptually wrong.

Hmm, I don't have strong opinion about this, but need to think of a new function name :)

> 
> @@ +279,5 @@
> >      parent = destInsertionPoints->LastElement()->GetParent();
> >      MOZ_ASSERT(parent);
> > +    */
> > +    // This works for Shadow DOM v1 only!
> > +    parent = GetFlattenedTreeParentForSlotable();
> 
> Just:
> 
> ```
> return GetFlattenedTreeParentForSlotable();
> ```
> 
> It will never return a shadow root anyway.

Right, and it'll crash if the function returns nullptr, thanks for catching this.

Comment 39

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #36)
> Comment on attachment 8931262 [details] [diff] [review]
> (WIP) Part 2: Include slots in the flat tree.
> 
> 
> >+  // When mIsParentSlot is set, mChild is always set to the current child, not
> >+  // matter mChild is an assigned node or a fallback content.
> nit. . after child. and then It does not matter whether mChild ...
> Something like that.

Will do.

> 
> >+  if (mIsParentSlot) {
> >+    return mChild;
> >+  }
> >+
> >+  // Flag set when parent is slot, in this case, the children will either be be
> >+  // the assigned nodes or the direct children, if assigned nodes is empty.
> >+  bool mIsParentSlot;
> We could also just store HTMLSlotElement* mParentAsSlot here. 
> 

Sounds good, will do that.

> 
> >+nsIContent*
> >+nsIContent::GetFlattenedTreeParentForSlotable() const
> >+{
> >+  if (!IsSlotable()) {
> >+    return nullptr;
> >+  }
> If you change this method the way emilio suggests, the name shouldn't
> contain 'Slotable'
> 
> 
> >+  HTMLSlotElement* slotEl = HTMLSlotElement::FromContent(aContent);
> >+  if (slotEl && slotEl->GetContainingShadow()) {
> >+    // Children of a slot insertion point are distributed to the
> >+    // slot if it does not have any assigned nodes (fallback content).
> >+    return slotEl->AssignedNodes().IsEmpty();
> >+  }
> I guess this matches what we do with v0, though I wouldn't call fallback
> content 'distributed'

Ok, will refine the comment.

> 
> Looks rather simple to me :) I like.

:)

(In reply to Olli Pettay [:smaug] from comment #37)
> Comment on attachment 8931258 [details] [diff] [review]
> Part 1: assignedNodes implementation, v0.
> 
> >-ShadowRoot::RedistributeElement(Element* aElement)
> >+ShadowRoot::RedistributeElement(Element* aElement, int32_t aNameSpaceID,
> >+                                nsAtom* aAttribute,
> >+                                const nsAttrValue* aOldValue)
> It would be nice to rename this method to something like...
> MaybeReassignElement or so, at least eventually

I see, we should get rid of of the term "distribution".

> 
> >+HTMLSlotElement::ClearAssignedNodes()
> >+{
> >+  for (uint32_t i = 0; i < mAssignedNodes.Length(); i++) {
> >+    ShadowRoot::RemoveDestInsertionPoint(this,
> >+      mAssignedNodes[i]->AsContent()->DestInsertionPoints());
> Why we need to call RemoveDestInsertionPoint? Is this a leftover from
> previous version of the patch?

Yes, ha, will remove it in the next patch.

Comment 40

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jessica Jong [:jessica] from comment #38)
> > 
> > Also, this has tests I assume? :)
> 
> About this.. can you point to me how to test that slots are actually in the
> flat tree? since it's not rendered, should I do something like add style to
> it and see if it affects its children?

You don't even need to do that... They should be rendered if they're styled with something that isn't display: contents.

So you could test both, for example:

  <slot style="color: green">This text should be green</slot>

And:

  <p>There should be a green box below:</p>
  <slot style="display: block; width: 100px; height: 100px; background: green;"></slot>

Comment 41

[Jessica Jong [:jessica] (inactive)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #40)
> (In reply to Jessica Jong [:jessica] from comment #38)
> > > 
> > > Also, this has tests I assume? :)
> > 
> > About this.. can you point to me how to test that slots are actually in the
> > flat tree? since it's not rendered, should I do something like add style to
> > it and see if it affects its children?
> 
> You don't even need to do that... They should be rendered if they're styled
> with something that isn't display: contents.
> 
> So you could test both, for example:
> 
>   <slot style="color: green">This text should be green</slot>
> 
> And:
> 
>   <p>There should be a green box below:</p>
>   <slot style="display: block; width: 100px; height: 100px; background:
> green;"></slot>

Thanks, I will add them to the upcoming patch.

Comment 42

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 3: use assignedSlot in GetEventTargetParent for node.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8932721 [details] [diff] [review]
(WIP) Part 3: use assignedSlot in GetEventTargetParent for node.

>+  // A nodeâs get the parent algorithm, given an event, returns the nodeâs
>+  // assigned slot, if node is assigned, and nodeâs parent otherwise.
Some unusual characters in the the comment?

>+nsresult
>+ShadowRoot::GetEventTargetParent(EventChainPreVisitor& aVisitor)
>+{
>+  aVisitor.mCanHandle = true;
>+
>+  // TODO: implement get-the-parent for ShadowRoot according to the latest spec.
>+  // A shadow rootâs get the parent algorithm, given an event, returns null
>+  // if eventâs composed flag is unset and shadow root is the root of eventâs
>+  // pathâs first tupleâs item, and shadow rootâs host otherwise.
Also here.

>+  if (!aVisitor.mEvent->mFlags.mComposed) {
So here we could check event's originalTarget and compare whether its root this this ShadowRoot or so.


>+    // If we do stop propagation, we still want to propagate
>+    // the event to chrome (nsPIDOMWindow::GetParentTarget()).
>+    // The load event is special in that we don't ever propagate it
>+    // to chrome.
>+    nsCOMPtr<nsPIDOMWindowOuter> win = OwnerDoc()->GetWindow();
>+    EventTarget* parentTarget = win && aVisitor.mEvent->mMessage != eLoad
>+      ? win->GetParentTarget() : nullptr;
>+
>+    aVisitor.mParentTarget = parentTarget;
>+    return NS_OK;
>+  }
>+
>+  nsIContent* shadowHost = GetHost();
>+  aVisitor.mParentTarget = shadowHost;



>+
>+  if (aVisitor.mOriginalTargetIsInAnon) {
>+    nsCOMPtr<nsIContent> content(do_QueryInterface(aVisitor.mEvent->mTarget));
>+    if (content && content->GetBindingParent() == shadowHost) {
>+      aVisitor.mEventTargetAtParent = shadowHost;
>+    }
>+  }
Need to think this if this is needed. But at least shouldn't harm

Comment 44

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #43)
> Comment on attachment 8932721 [details] [diff] [review]
> (WIP) Part 3: use assignedSlot in GetEventTargetParent for node.
> 
> >+  // A nodeâs get the parent algorithm, given an event, returns the nodeâs
> >+  // assigned slot, if node is assigned, and nodeâs parent otherwise.
> Some unusual characters in the the comment?

Oh, I copied the comments from the spec, will fix this.

> 
> >+nsresult
> >+ShadowRoot::GetEventTargetParent(EventChainPreVisitor& aVisitor)
> >+{
> >+  aVisitor.mCanHandle = true;
> >+
> >+  // TODO: implement get-the-parent for ShadowRoot according to the latest spec.
> >+  // A shadow rootâs get the parent algorithm, given an event, returns null
> >+  // if eventâs composed flag is unset and shadow root is the root of eventâs
> >+  // pathâs first tupleâs item, and shadow rootâs host otherwise.
> Also here.
> 
> >+  if (!aVisitor.mEvent->mFlags.mComposed) {
> So here we could check event's originalTarget and compare whether its root
> this this ShadowRoot or so.

Ok, adding this will solve the TODO part above.

> 
> 
> >+    // If we do stop propagation, we still want to propagate
> >+    // the event to chrome (nsPIDOMWindow::GetParentTarget()).
> >+    // The load event is special in that we don't ever propagate it
> >+    // to chrome.
> >+    nsCOMPtr<nsPIDOMWindowOuter> win = OwnerDoc()->GetWindow();
> >+    EventTarget* parentTarget = win && aVisitor.mEvent->mMessage != eLoad
> >+      ? win->GetParentTarget() : nullptr;
> >+
> >+    aVisitor.mParentTarget = parentTarget;
> >+    return NS_OK;
> >+  }
> >+
> >+  nsIContent* shadowHost = GetHost();
> >+  aVisitor.mParentTarget = shadowHost;
> 
> 
> 
> >+
> >+  if (aVisitor.mOriginalTargetIsInAnon) {
> >+    nsCOMPtr<nsIContent> content(do_QueryInterface(aVisitor.mEvent->mTarget));
> >+    if (content && content->GetBindingParent() == shadowHost) {
> >+      aVisitor.mEventTargetAtParent = shadowHost;
> >+    }
> >+  }
> Need to think this if this is needed. But at least shouldn't harm

If I don't add this, some event tests time out.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Oh, of course, since we use binding parent here still. Eventually we should get rid of it.

Comment 46

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: assignedNodes implementation, v1.

Rebased on top of bug 1418002.
Made some changes now that the code is simpler, so reasking review. Thanks.

Comment 47

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Include slots in the flat tree, v1.

Rebased on top of bug 1418002 and addressed review comments. Also, added basic reftests for slot.

Comment 48

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: use assignedSlot in GetEventTargetParent for node, v1.

Comment 49

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8933226 [details] [diff] [review]
Part 2: Include slots in the flat tree, v1.

Review of attachment 8933226 [details] [diff] [review]:
-----------------------------------------------------------------

Code-wise looks good, though I have a bunch of nits. There should be also a test for a <slot> in a shadow tree, with and without assigned children (so we also test that fallback content works).

::: dom/base/ChildIterator.cpp
@@ +65,5 @@
> +    mDefaultChild(nullptr),
> +    mIsFirst(aStartAtBeginning),
> +    mIndexInInserted(0)
> +{
> +  mParentAsSlot = HTMLSlotElement::FromContentOrNull(mParent);

I'm pretty sure we assume everywhere that mParent is not null, so probably should just use FromContent.

@@ +74,5 @@
> +    mDefaultChild(aOther.mDefaultChild),
> +    mIsFirst(aOther.mIsFirst),
> +    mIndexInInserted(aOther.mIndexInInserted)
> +{
> +  mParentAsSlot = HTMLSlotElement::FromContentOrNull(mParent);

Why not mParentAsSlot(aOther.mParentAsSlot)?

@@ +79,5 @@
> +}
> +
> +ExplicitChildIterator::ExplicitChildIterator(ExplicitChildIterator&& aOther)
> +  : mParent(aOther.mParent), mChild(aOther.mChild),
> +    mDefaultChild(aOther.mDefaultChild),

Ditto re mParentAsSlot.

@@ +96,4 @@
>      MOZ_ASSERT(!mDefaultChild);
>  
> +    if (mParentAsSlot) {
> +      const nsTArray<RefPtr<nsINode>>& assignedNodes =

(I'd say that const auto& wouldn't hurt here, but Olly probably thinks differently, so nbd :P)

@@ +292,5 @@
> +      const nsTArray<RefPtr<nsINode>>& assignedNodes =
> +        mParentAsSlot->AssignedNodes();
> +      if (!assignedNodes.IsEmpty()) {
> +        mIndexInInserted = assignedNodes.Length();
> +        mChild = assignedNodes[mIndexInInserted - 1]->AsContent();

Should the AssignedNodes array just have nsIContent instead of nsINode?

::: dom/base/nsIContent.h
@@ +806,5 @@
> +  /**
> +   * Returns the assigned slot, if it exists, or the direct parent, if it's a
> +   * fallback content of a slot.
> +   */
> +  nsIContent* GetFlattenedTreeParentForMaybeAssignedNode() const;

This shouldn't be public (maybe it'd be better for it just to be a static function in the cpp file?).

::: layout/reftests/webcomponents/basic-slot-1-ref.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <style>body { color: green; }</style>

These should probably go in WPT, any reason for they not to be there?

::: layout/reftests/webcomponents/basic-slot-1.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +  <body>
> +    <slot style="color: green">This text should be green</slot>

There should be a test that also tests slots in a ShadowRoot I'd say... Otherwise these tests also pass without <slot>.

Also, I'll make the slots with display: contents also have: <slot style="color: green; border: 1px solid red;"> (the border shouldn't be visible, due to display: contents).

::: layout/style/res/ua.css
@@ +485,5 @@
>    width: 100%;
>    height: 100%;
>  }
> +
> +/* Shadow DOM v1 */

Could you leave a link to the spec here? (https://drafts.csswg.org/css-scoping/#slots-in-shadow-tree)

Comment 50

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #49)
> (I'd say that const auto& wouldn't hurt here, but Olly probably thinks
> differently, so nbd :P)

Err, Olli, ofc :)

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

If one would use auto there, it isn't clear to the reader whether the array keeps the objects in it alive. Though, in this & case one needs to also ensure the array can't be modified.
Also, seeing that we're dealing with RefPtr may in common case affect to performance, so there really isn't any reason to hide the type information.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8933226 [details] [diff] [review]
Part 2: Include slots in the flat tree, v1.

>+ExplicitChildIterator::ExplicitChildIterator(const nsIContent* aParent,
>+                                             bool aStartAtBeginning)
>+  : mParent(aParent),
>+    mChild(nullptr),
>+    mDefaultChild(nullptr),
>+    mIsFirst(aStartAtBeginning),
>+    mIndexInInserted(0)
>+{
>+  mParentAsSlot = HTMLSlotElement::FromContentOrNull(mParent);
As emilio said, mParent should be non-null

>+}
>+
>+ExplicitChildIterator::ExplicitChildIterator(const ExplicitChildIterator& aOther)
>+  : mParent(aOther.mParent), mChild(aOther.mChild),
>+    mDefaultChild(aOther.mDefaultChild),
>+    mIsFirst(aOther.mIsFirst),
>+    mIndexInInserted(aOther.mIndexInInserted)
>+{
>+  mParentAsSlot = HTMLSlotElement::FromContentOrNull(mParent);
 = mOwner.mParentAsSlot

>+ExplicitChildIterator::ExplicitChildIterator(ExplicitChildIterator&& aOther)
>+  : mParent(aOther.mParent), mChild(aOther.mChild),
>+    mDefaultChild(aOther.mDefaultChild),
>+    mIsFirst(aOther.mIsFirst),
>+    mIndexInInserted(aOther.mIndexInInserted)
>+{
>+  mParentAsSlot = HTMLSlotElement::FromContentOrNull(mParent);
ditto

GetFlattenedTreeParentForMaybeAssignedNode could indeed be a private or protected method.
Not a static since it anyhow calls non-static methods using 'this'


Btw, my guess is that keeping assignedNodes as array of nsINodes is after all simpler, since it
maps nicely to DOM API 
sequence<Node> assignedNodes(optional AssignedNodesOptions options);

Tests should be in wpt, but if you want to land sooner, conversion can be done separately.
(We'll need to add more tests, lots.)

Comment 53

[Jessica Jong [:jessica] (inactive)]

Thanks a lot, Emilio and Olli, for your review comments.
For `Part 2: Include slots in the flat tree, v1`:

- ExplicitChildIterator's explicit constructor should just use `HTMLSlotElement::FromContent` and the copy constructor  just use aOther.mParentAsSlot
- I think we should keep assignedNodes as array of nsINodes due to the reason mentioned in comment 52
- I can make GetFlattenedTreeParentForMaybeAssignedNode static, just need to have a nsINode* aNode as parameter
- Since this bug is blocking other's work (including devtools), I'll add some more reftests but will file a follow-up to convert them into wpt, if that'ok with you

Comment 54

[Emilio Cobos Álvarez (:emilio)]

Sure, that's fine :)

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Sounds good, though as I said, I don't see the reason to make GetFlattenedTreeParentForMaybeAssignedNode static.

Comment 56

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Include slots in the flat tree, v2.

Emilio, could you take a look again? I have addressed some of the review comments and splitted the tests into another separate patch.

Comment 57

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: use assignedSlot in GetEventTargetParent for node, v1.

rebased.

Comment 58

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8933979 [details] [diff] [review]
Part 2: Include slots in the flat tree, v2.

Review of attachment 8933979 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/FragmentOrElement.cpp
@@ +191,5 @@
> +  }
> +
> +  // Check if it's a fallback content.
> +  HTMLSlotElement* parentSlot =
> +    HTMLSlotElement::FromContentOrNull(GetParent());

This can use just FromContent, since we know `parent` is not null and is content on the caller.

@@ +196,5 @@
> +  if (!parentSlot) {
> +    return nullptr;
> +  }
> +
> +  if (!parentSlot->AssignedNodes().IsEmpty()) {

This can't happen, right? (Because of HasDistributedChildren). Maybe just assert it?

@@ +258,5 @@
>    if (IsRootOfAnonymousSubtree()) {
>      return parent;
>    }
>  
>    if (nsContentUtils::HasDistributedChildren(parent)) {

I wonder how useful is this check, actually... Looks to me that we could just do the slot checks here... But no big deal, it's fine to keep it for now.

Comment 59

[Emilio Cobos Álvarez (:emilio)]

Looks fine, thanks a lot jessica!

Comment 60

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: reftests, v1.

Made the tests in layout/reftest/webcomponents/ v1 compatible and added some basic slot reftests.
Basically, this fixes Bug 1421542, will mark as duplicate when this patch lands.

Note that I have renamed:
- basic-insertion-point-1.html and basic-insertion-point-1-ref.html to basic-slot-5.html and basic-slot-5-ref
- basic-insertion-point-2.html and basic-insertion-point-2-ref.html to basic-slot-6.html and basic-slot-6-ref

(basic-slot-1.html to basic-slot-4.html are new tests)

Comment 61

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: update wpt expectation data, v1.

Comment 62

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #60)
> Created attachment 8933984 [details] [diff] [review]
> Part 4: reftests, v1.
> 
> Made the tests in layout/reftest/webcomponents/ v1 compatible and added some
> basic slot reftests.
> Basically, this fixes Bug 1421542, will mark as duplicate when this patch
> lands.
> 
> Note that I have renamed:
> - basic-insertion-point-1.html and basic-insertion-point-1-ref.html to
> basic-slot-5.html and basic-slot-5-ref
> - basic-insertion-point-2.html and basic-insertion-point-2-ref.html to
> basic-slot-6.html and basic-slot-6-ref
> 
> (basic-slot-1.html to basic-slot-4.html are new tests)

Oh, I left cross-tree-selection-1.html untouched, since it's not clear yet how selection should work with shadow DOM.

Comment 63

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8933984 [details] [diff] [review]
Part 4: reftests, v1.

Review of attachment 8933984 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/webcomponents/dynamic-insertion-point-distribution-2.html
@@ +9,5 @@
>  
>        function run() {
>          host = document.getElementById("host");
> +        root = host.attachShadow({mode: 'open'});
> +        root.innerHTML = "<slot>a</slot>";

We could do this also without creating any slot, right? Which would be closer to what the old test did.

Anyway looks fine.

::: layout/reftests/webcomponents/input-transition-1.html
@@ +20,5 @@
>              setTimeout(()=>{document.documentElement.removeAttribute("class")}, 1000); // wait for the checkbox SVG image to load on Android
>            });
>        }
>  
> +      if (document.body.attachShadow) {

We can just remove the if conditions now stylo supports shadow dom, if you want.

Comment 64

[Jessica Jong [:jessica] (inactive)]

Thanks again for the review. :)

(In reply to Emilio Cobos Álvarez [:emilio] from comment #58)
> Comment on attachment 8933979 [details] [diff] [review]
> Part 2: Include slots in the flat tree, v2.
> 
> Review of attachment 8933979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/FragmentOrElement.cpp
> @@ +191,5 @@
> > +  }
> > +
> > +  // Check if it's a fallback content.
> > +  HTMLSlotElement* parentSlot =
> > +    HTMLSlotElement::FromContentOrNull(GetParent());
> 
> This can use just FromContent, since we know `parent` is not null and is
> content on the caller.

Will do.

> 
> @@ +196,5 @@
> > +  if (!parentSlot) {
> > +    return nullptr;
> > +  }
> > +
> > +  if (!parentSlot->AssignedNodes().IsEmpty()) {
> 
> This can't happen, right? (Because of HasDistributedChildren). Maybe just
> assert it?

Right, will assert it instead.

> 
> @@ +258,5 @@
> >    if (IsRootOfAnonymousSubtree()) {
> >      return parent;
> >    }
> >  
> >    if (nsContentUtils::HasDistributedChildren(parent)) {
> 
> I wonder how useful is this check, actually... Looks to me that we could
> just do the slot checks here... But no big deal, it's fine to keep it for
> now.

Ok, let's keep it for now.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #63)
> Comment on attachment 8933984 [details] [diff] [review]
> Part 4: reftests, v1.
> 
> Review of attachment 8933984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/webcomponents/dynamic-insertion-point-distribution-2.html
> @@ +9,5 @@
> >  
> >        function run() {
> >          host = document.getElementById("host");
> > +        root = host.attachShadow({mode: 'open'});
> > +        root.innerHTML = "<slot>a</slot>";
> 
> We could do this also without creating any slot, right? Which would be
> closer to what the old test did.
> 
> Anyway looks fine.

Yes, no need to change it to slot, I think I misread this. :/

> 
> ::: layout/reftests/webcomponents/input-transition-1.html
> @@ +20,5 @@
> >              setTimeout(()=>{document.documentElement.removeAttribute("class")}, 1000); // wait for the checkbox SVG image to load on Android
> >            });
> >        }
> >  
> > +      if (document.body.attachShadow) {
> 
> We can just remove the if conditions now stylo supports shadow dom, if you
> want.

Will remove the if conditions.

Comment 65

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Include slots in the flat tree, v3.

Addressed review comment 58, carrying r+.

Comment 66

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: reftests, v2.

addressed review comment 63, carrying r+.

Comment 67

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5853ea969aae1c75aa8431eb75e5782c1914142&group_state=expanded

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/069724f02e0e
Part 1: Implementation for assignedNodes. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6f130be445
Part 2: Include slots in the flat tree. r=smaug,emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d702467c676
Part 3: Fix event get-the-parent algorithm for a node. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd8c0923cfe4
Part 4: Update and add reftests for Shadow DOM v1. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa14b7a60a4
Part 5: Update web platform tests expectations. r=smaug

Comment 69

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/069724f02e0e
https://hg.mozilla.org/mozilla-central/rev/7e6f130be445
https://hg.mozilla.org/mozilla-central/rev/7d702467c676
https://hg.mozilla.org/mozilla-central/rev/cd8c0923cfe4
https://hg.mozilla.org/mozilla-central/rev/3fa14b7a60a4