854736 - Implement web components content element.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[William Chen [:wchen]]

https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#content-element

Comment 1

[William Chen [:wchen]]

Attachment: Implement content element.

Comment 2

[William Chen [:wchen]]

Attachment: Implement content element.

Comment 3

[William Chen [:wchen]]

Attachment: Content element tests

Comment 4

[William Chen [:wchen]]

Attachment: Implement content element. v2

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 785962 [details] [diff] [review]
Implement content element. v2

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

Overall, this looks really good. I have a couple of non-nits (and a few nits) but nothing big to fix that I found.

::: content/base/src/ChildIterator.cpp
@@ +44,5 @@
> +  };
> +};
> +
> +static inline MatchedNodes
> +GetMatchedNodesForPoint(nsIContent* aContent)

Right now, <xbl:children> that are in custom elements will be treated by the iterator as possible insertion points. We need to add some state to them so that we'll treat them like any other element.

::: content/base/src/ShadowRoot.cpp
@@ +187,5 @@
> +}
> +
> +static bool
> +SeekUntilFound(ExplicitChildIterator& aIterator, nsIContent* aNeedle,
> +               nsIContent* aBound = nullptr) {

Nit: { on its own line.

Also, this should really just use a modified version of FlattenedChildIterator::Seek (which should be moved to ExplicitChildIterator and take an aBound parameter).

@@ +340,5 @@
> + * Returns whether the web components pool population algorithm
> + * on the host would contain |aContent|.
> + */
> +static bool
> +IsPooledNode(nsIContent* aContent, nsIContent* aContainer, nsIContent* aHost) {

Nits: { on its own line and nuke the else-after-returns.

::: content/base/src/nsContentUtils.cpp
@@ +6452,5 @@
> +
> +  return false;
> +}
> +
> +

Uber-nit: nuke the extra newline.

::: content/html/content/src/HTMLContentElement.cpp
@@ +78,5 @@
> +      // If the content element is being inserted into a ShadowRoot,
> +      // add this element to the list of insertion points.
> +      mIsInsertionPoint = true;
> +      shadowRoot->AddInsertionPoint(this);
> +      shadowRoot->MatchAllNodes();

How does this code interact with building custom elements? Are we going to redistribute all children once per content element? I'm especially wondering about this in the declarative case, but it might also be important for the imperative case.

@@ +116,5 @@
> +  mMatchedNodes.Clear();
> +}
> +
> +static bool
> +IsValidContentSelectors(nsCSSSelector* aSelector) {

Nit: { on its own line.

@@ +158,5 @@
> +                                             getter_Transfers(mSelectorList));
> +
> +    // We don't want to return an exception if parsing failed because
> +    // the spec does not define it as an exception case.
> +    if (NS_SUCCEEDED(rv)) {

If the value fails to parse, should we be setting mValidSelector to false? It doesn't even look like the spec mentions what to do in this case.

Comment 6

[William Chen [:wchen]]

Attachment: Implement content element. v3

(In reply to Blake Kaplan (:mrbkap) from comment #5)
> Comment on attachment 785962 [details] [diff] [review]
> Implement content element. v2
> 
> Review of attachment 785962 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, this looks really good. I have a couple of non-nits (and a few
> nits) but nothing big to fix that I found.
> 
> ::: content/base/src/ChildIterator.cpp
> @@ +44,5 @@
> > +  };
> > +};
> > +
> > +static inline MatchedNodes
> > +GetMatchedNodesForPoint(nsIContent* aContent)
> 
> Right now, <xbl:children> that are in custom elements will be treated by the
> iterator as possible insertion points. We need to add some state to them so
> that we'll treat them like any other element.
> 
I think it's OK just to leave <xbl:children> the way they are since they don't really interfere with web component content insertion points. Web components doesn't distribute any nodes into them.
> ::: content/html/content/src/HTMLContentElement.cpp
> @@ +78,5 @@
> > +      // If the content element is being inserted into a ShadowRoot,
> > +      // add this element to the list of insertion points.
> > +      mIsInsertionPoint = true;
> > +      shadowRoot->AddInsertionPoint(this);
> > +      shadowRoot->MatchAllNodes();
> 
> How does this code interact with building custom elements? Are we going to
> redistribute all children once per content element? I'm especially wondering
> about this in the declarative case, but it might also be important for the
> imperative case.
> 
The way it works in this patch is that when the content elements are bound to a ShadowRoot tree, it flips a flag on the ShadowRoot to indicate that nodes need to be redistributed. It then does the redistribution when the mutation observer gets called. So if the shadow tree is constructed first, we will only get on ContentAppended call to the mutation observer when the shadow tree is populated.
> 
> @@ +158,5 @@
> > +                                             getter_Transfers(mSelectorList));
> > +
> > +    // We don't want to return an exception if parsing failed because
> > +    // the spec does not define it as an exception case.
> > +    if (NS_SUCCEEDED(rv)) {
> 
> If the value fails to parse, should we be setting mValidSelector to false?
> It doesn't even look like the spec mentions what to do in this case.

I interpreted the set of selectors as being empty if it failed to parse which is a matching condition. But you're right, the spec is unclear and it can go either way.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 820832 [details] [diff] [review]
Implement content element. v3

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

This looks really good. r=mrbkap with the following comments addressed.

::: content/base/src/ShadowRoot.h
@@ +94,5 @@
>  protected:
>    nsCOMPtr<nsIContent> mHost;
> +
> +  // An array of content insertion points that are a descendant of the ShadowRoot
> +  // sorted in tree order.

I'd add a comment here making it explicit that the insertion points are responsible for notifying the shadow root when they're removed from the tree (and that therefore it's safe to hold weak references here).

@@ +102,5 @@
> +
> +  // A boolean that indicates that an insertion point was added or removed
> +  // from this ShadowRoot and that the nodes need to be redistributed into
> +  // the insertion points.
> +  bool mInsertionPointChanged;

Please specify that when this is set, we rely on the mutation observer listeners to read it right after.

::: content/base/src/nsContentUtils.cpp
@@ +6551,5 @@
>  
>  bool
> +nsContentUtils::IsContentInsertionPoint(const nsIContent* aContent)
> +{
> +  if (aContent->IsActiveChildrenElement()) {

Just to be clear to future readers of this code, I'd add comments "XBL case" and "web components case".

::: content/html/content/src/HTMLContentElement.cpp
@@ +92,5 @@
> +    if (rootShadow) {
> +      rootShadow->RemoveInsertionPoint(this);
> +
> +      // Remove all the assigned nodes now that the
> +      // insertion point no longer a descendant of

...now that the insertion point *is* no longer...

@@ +152,5 @@
> +    mSelectorList = nullptr;
> +
> +    nsresult rv = parser.ParseSelectorString(aValue,
> +                                             doc->GetDocumentURI(),
> +                                             0, // XXX get the line number!

Please either file a followup or just point to bug 11240 here.

@@ +214,5 @@
> +  nsIDocument* doc = OwnerDoc();
> +  ShadowRoot* rootShadow = GetRootShadow();
> +  nsIContent* host = rootShadow->GetHost();
> +
> +  TreeMatchContext matchingContext(false, nsRuleWalker::eRelevantLinkUnvisited,

Do we need to do all of this setup if mSelectorList is null?

Comment 8

[William Chen [:wchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79b61f9909c0

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79b61f9909c0