Implement web components content element.

RESOLVED FIXED in mozilla28

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: wchen, Assigned: wchen)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla28
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#content-element
(Assignee)

Updated

4 years ago
Depends on: 806506
(Assignee)

Comment 1

4 years ago
Created attachment 767897 [details] [diff] [review]
Implement content element.
Attachment #767897 - Flags: review?(mrbkap)

Updated

4 years ago

Updated

4 years ago
Assignee: nobody → wchen
(Assignee)

Comment 2

4 years ago
Created attachment 777226 [details] [diff] [review]
Implement content element.
Attachment #767897 - Attachment is obsolete: true
Attachment #767897 - Flags: review?(mrbkap)
Attachment #777226 - Flags: review?(mrbkap)
(Assignee)

Comment 3

4 years ago
Created attachment 777228 [details] [diff] [review]
Content element tests
(Assignee)

Comment 4

4 years ago
Created attachment 785962 [details] [diff] [review]
Implement content element. v2
Attachment #777226 - Attachment is obsolete: true
Attachment #777226 - Flags: review?(mrbkap)
Attachment #785962 - Flags: review?(mrbkap)
Keywords: dev-doc-needed
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.
Attachment #785962 - Flags: review?(mrbkap) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 820832 [details] [diff] [review]
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.
Attachment #785962 - Attachment is obsolete: true
Attachment #820832 - Flags: review?(mrbkap)
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?
Attachment #820832 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b61f9909c0
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/79b61f9909c0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Depends on: 949893
You need to log in before you can comment on or make changes to this bug.