Last Comment Bug 806506 - Implement web components ShadowRoot interface.
: Implement web components ShadowRoot interface.
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal with 7 votes (vote)
: mozilla28
Assigned To: William Chen [:wchen]
:
: Andrew Overholt [:overholt]
Mentors:
https://dvcs.w3.org/hg/webcomponents/...
Depends on: 653881 942217 945463 992521
Blocks: webcomponents 854736
  Show dependency treegraph
 
Reported: 2012-10-29 12:34 PDT by William Chen [:wchen]
Modified: 2016-07-17 22:21 PDT (History)
32 users (show)
wchen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Implement ShadowRoot interface with DOM accessor methods. (53.15 KB, patch)
2013-06-19 11:50 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Part 2: Implement innerHTML on ShadowRoot. (55.27 KB, patch)
2013-06-26 11:28 PDT, William Chen [:wchen]
mrbkap: review+
Details | Diff | Splinter Review
Implement ShadowRoot interface with DOM accessor methods. v2 (64.98 KB, patch)
2013-07-17 11:08 PDT, William Chen [:wchen]
mrbkap: feedback+
Details | Diff | Splinter Review
innerHTML diff (3.76 KB, patch)
2013-07-17 11:09 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Part 3: Implement ShadowRoot style methods. (50.19 KB, patch)
2013-07-18 14:41 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
ShadowRoot style tests. (11.60 KB, patch)
2013-07-18 14:42 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Implement ShadowRoot interface with DOM accessor methods. v3 (68.03 KB, patch)
2013-10-22 23:12 PDT, William Chen [:wchen]
mrbkap: review+
ehsan: review-
Details | Diff | Splinter Review
Selection diff (6.11 KB, patch)
2013-11-12 18:37 PST, William Chen [:wchen]
ehsan: review+
Details | Diff | Splinter Review

Comment 1 William Chen [:wchen] 2013-06-19 11:50:12 PDT
Created attachment 764907 [details] [diff] [review]
Part 1: Implement ShadowRoot interface with DOM accessor methods.
Comment 2 William Chen [:wchen] 2013-06-26 11:28:02 PDT
Created attachment 767896 [details] [diff] [review]
Part 2: Implement innerHTML on ShadowRoot.
Comment 3 Blake Kaplan (:mrbkap) 2013-07-16 16:14:19 PDT
I'm finally starting to review this. William told me that he has updated patches in his queue that he's going to post soon.
Comment 4 Blake Kaplan (:mrbkap) 2013-07-16 16:44:57 PDT
Comment on attachment 767896 [details] [diff] [review]
Part 2: Implement innerHTML on ShadowRoot.

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

Can you generate a diff of what changed when you moved the function from Element to FragmentOrElement?
Comment 5 William Chen [:wchen] 2013-07-17 11:08:49 PDT
Created attachment 777224 [details] [diff] [review]
Implement ShadowRoot interface with DOM accessor methods. v2
Comment 6 William Chen [:wchen] 2013-07-17 11:09:20 PDT
Created attachment 777225 [details] [diff] [review]
innerHTML diff
Comment 7 William Chen [:wchen] 2013-07-18 14:41:44 PDT
Created attachment 778071 [details] [diff] [review]
Part 3: Implement ShadowRoot style methods.
Comment 8 William Chen [:wchen] 2013-07-18 14:42:12 PDT
Created attachment 778074 [details] [diff] [review]
ShadowRoot style tests.
Comment 9 Blake Kaplan (:mrbkap) 2013-07-24 14:43:33 PDT
Comment on attachment 777224 [details] [diff] [review]
Implement ShadowRoot interface with DOM accessor methods. v2

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

This looks pretty good overall. I have a few comments to address though.

::: content/base/public/nsIContent.h
@@ +647,5 @@
> +   * binding is rendered in place of this node's children.
> +   *
> +   * @param aShadowRoot The ShadowRoot to be bound to this element.
> +   */
> +  virtual void SetShadowRoot(mozilla::dom::ShadowRoot* aShadowRoot) = 0;

Need to update the IID here.

::: content/base/src/Element.cpp
@@ +721,5 @@
> +Element::CreateShadowRoot(ErrorResult& aError)
> +{
> +  nsAutoScriptBlocker scriptBlocker;
> +
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(this);

You shouldn't need to do_QI here since Element already inherits nsIContent.

@@ +747,5 @@
> +  content->SetShadowRoot(shadowRoot);
> +
> +  // xblBinding takes ownership of docInfo.
> +  nsRefPtr<nsXBLBinding> xblBinding = new nsXBLBinding(shadowRoot, protoBinding);
> +  xblBinding->SetBoundElement(content);

Is support for stacked shadow roots coming in a separate patch? As far as I can tell, this simply replaces the current shadow root with the new one.

::: content/base/src/ShadowRoot.cpp
@@ +70,5 @@
> +  return mozilla::dom::ShadowRootBinding::Wrap(aCx, aScope, this);
> +}
> +
> +ShadowRoot*
> +ShadowRoot::FromNode(nsINode* aNode) {

Nit: { on its own line.

@@ +144,5 @@
> +bool
> +ShadowRoot::PrefEnabled()
> +{
> +  static bool sPrefValue =
> +    Preferences::GetBool("dom.webcomponents.enabled", false);

This should be done (at least) during XPCOM startup to avoid running code during program initialization (shared library loading, etc). Would it break things if people could turn on and off webcomponents at runtime? If not, we should also add a pref listener to update the value after startup.

::: dom/webidl/ShadowRoot.webidl
@@ +15,5 @@
> +{
> +  Element? getElementById(DOMString elementId);
> +  HTMLCollection getElementsByTagName(DOMString localName);
> +  HTMLCollection getElementsByTagNameNS(DOMString? namespace, DOMString localName);
> +  HTMLCollection getElementsByClassName(DOMString classNames);

We need to make sure that the spec is updated to look like this: its webidl looks out of date.

::: layout/generic/nsSelection.cpp
@@ +4656,5 @@
>  
>    nsRefPtr<nsPresContext>  presContext = GetPresContext();
> +
> +  // Disconnected point will create a collapsed range that selects nothing.
> +  // Repaint whatever is currently selected so that the selection is removed.

This comment isn't really clear (and the first sentence doesn't make any sense at all to me).
Comment 10 William Chen [:wchen] 2013-10-22 23:12:26 PDT
Created attachment 820825 [details] [diff] [review]
Implement ShadowRoot interface with DOM accessor methods. v3

(In reply to Blake Kaplan (:mrbkap) from comment #9)
> Comment on attachment 777224 [details] [diff] [review]
> Implement ShadowRoot interface with DOM accessor methods. v2
> 
> Review of attachment 777224 [details] [diff] [review]:
> 
> @@ +747,5 @@
> > +  content->SetShadowRoot(shadowRoot);
> > +
> > +  // xblBinding takes ownership of docInfo.
> > +  nsRefPtr<nsXBLBinding> xblBinding = new nsXBLBinding(shadowRoot, protoBinding);
> > +  xblBinding->SetBoundElement(content);
> 
> Is support for stacked shadow roots coming in a separate patch? As far as I
> can tell, this simply replaces the current shadow root with the new one.
> 
Yes, multiple shadow roots comes in a later patch.
> 
> @@ +144,5 @@
> > +bool
> > +ShadowRoot::PrefEnabled()
> > +{
> > +  static bool sPrefValue =
> > +    Preferences::GetBool("dom.webcomponents.enabled", false);
> 
> This should be done (at least) during XPCOM startup to avoid running code
> during program initialization (shared library loading, etc). Would it break
> things if people could turn on and off webcomponents at runtime? If not, we
> should also add a pref listener to update the value after startup.
> 
I've changed it to simply return the value of the preference since that what we do in a lot of other places.
> ::: dom/webidl/ShadowRoot.webidl
> @@ +15,5 @@
> > +{
> > +  Element? getElementById(DOMString elementId);
> > +  HTMLCollection getElementsByTagName(DOMString localName);
> > +  HTMLCollection getElementsByTagNameNS(DOMString? namespace, DOMString localName);
> > +  HTMLCollection getElementsByClassName(DOMString classNames);
> 
> We need to make sure that the spec is updated to look like this: its webidl
> looks out of date.
> 
Yeah, I can bring this up the next time spec discussions come up.
> ::: layout/generic/nsSelection.cpp
> @@ +4656,5 @@
> >  
> >    nsRefPtr<nsPresContext>  presContext = GetPresContext();
> > +
> > +  // Disconnected point will create a collapsed range that selects nothing.
> > +  // Repaint whatever is currently selected so that the selection is removed.
> 
> This comment isn't really clear (and the first sentence doesn't make any
> sense at all to me).
Hopefully it makes more sense now.

All other comments addressed.

The only other big change here is that I added a GetRootShadow method to nsIContent.
Comment 11 William Chen [:wchen] 2013-10-22 23:17:00 PDT
Comment on attachment 778071 [details] [diff] [review]
Part 3: Implement ShadowRoot style methods.

Moving the style patches to Bug 929885.
Comment 12 Blake Kaplan (:mrbkap) 2013-10-24 15:51:12 PDT
Comment on attachment 820825 [details] [diff] [review]
Implement ShadowRoot interface with DOM accessor methods. v3

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

I have a couple of small comments, but this looks good to me with them fixed. I'd like Ehsan to take a look at the nsSelection.cpp changes, though (my relationship with that code is rocky, at best).

::: content/base/public/FragmentOrElement.h
@@ +365,5 @@
> +
> +    /**
> +     * The root ShadowRoot of this element if it is in a shadow tree.
> +     */
> +    nsRefPtr<ShadowRoot> mRootShadow;

These two members are simply asking to be confused with each other. I suppose you need "root" somewhere in there because of <shadow> elements? I want to suggest something like mContainingShadow, but I'm not really happy with that either.

::: content/base/src/Element.cpp
@@ +734,5 @@
> +Element::CreateShadowRoot(ErrorResult& aError)
> +{
> +  nsAutoScriptBlocker scriptBlocker;
> +
> +  nsCOMPtr<nsIContent> content = this;

Looking at this again, why do you need to do this at all? Why can't you just pass |this| below? Is it the extra reference, or something else?

::: layout/base/nsCSSFrameConstructor.cpp
@@ +6596,5 @@
>        return NS_OK;
>  
>    }
>  #endif // MOZ_XUL
> +  if (aContainer && aContainer->HasFlag(NODE_IS_IN_SHADOW_TREE)) {

Nit: blank line after the #endif
Comment 13 :Ehsan Akhgari 2013-10-24 15:59:51 PDT
Comment on attachment 820825 [details] [diff] [review]
Implement ShadowRoot interface with DOM accessor methods. v3

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

Can somebody please explain the purpose of the nsSelection.cpp changes, so that I don't have to go through the entire patch?  Thanks!  :-)
Comment 14 :Ehsan Akhgari 2013-10-25 13:02:12 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #13)
> Comment on attachment 820825 [details] [diff] [review]
> Implement ShadowRoot interface with DOM accessor methods. v3
> 
> Review of attachment 820825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can somebody please explain the purpose of the nsSelection.cpp changes, so
> that I don't have to go through the entire patch?  Thanks!  :-)
Comment 15 William Chen [:wchen] 2013-10-25 13:51:18 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #14)
> (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment
> #13)
> > Comment on attachment 820825 [details] [diff] [review]
> > Implement ShadowRoot interface with DOM accessor methods. v3
> > 
> > Review of attachment 820825 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can somebody please explain the purpose of the nsSelection.cpp changes, so
> > that I don't have to go through the entire patch?  Thanks!  :-)

There is currently a bug with selection when we have a range that crosses trees (e.g. the range starts in the document and ends in an anonymous tree). The range is disconnected and the selection ends up getting cleared, but the selection still remains painted. You can reproduce this now by creating an XBL binding with some text in it and trying to select it along with text in the document. Same problem happens with shadow roots. The change in nsSelection.cpp simply clears the painted selection.
Comment 16 :Ehsan Akhgari 2013-10-28 07:28:24 PDT
Comment on attachment 820825 [details] [diff] [review]
Implement ShadowRoot interface with DOM accessor methods. v3

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

::: layout/generic/nsSelection.cpp
@@ +4670,5 @@
> +  // resulting in a range that selects nothing.
> +  if (disconnected) {
> +    // Repaint the current range with the selection removed.
> +    selectFrames(presContext, range, false);
> +  }

You're only clearing the selection for the case where the anchor and parent nodes fall into different trees here.  You want to handle the two other cases too.

Also, please add three reftests, one for each one of these cases to make sure that the selection is not painted in either one of them.
Comment 17 William Chen [:wchen] 2013-11-12 18:37:20 PST
Created attachment 831244 [details] [diff] [review]
Selection diff

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #16)
> Comment on attachment 820825 [details] [diff] [review]
> Implement ShadowRoot interface with DOM accessor methods. v3
> 
> Review of attachment 820825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsSelection.cpp
> @@ +4670,5 @@
> > +  // resulting in a range that selects nothing.
> > +  if (disconnected) {
> > +    // Repaint the current range with the selection removed.
> > +    selectFrames(presContext, range, false);
> > +  }
> 
> You're only clearing the selection for the case where the anchor and parent
> nodes fall into different trees here.  You want to handle the two other
> cases too.
> 
> Also, please add three reftests, one for each one of these cases to make
> sure that the selection is not painted in either one of them.

It should be sufficient to only check if the parent and anchor are disconnected. The focus and anchor should always be in the same tree, thus the anchor and parent should be in the same tree iff the focus and parent are in the same tree. So if the anchor and parent are not disconnected, the focus and parent should not be disconnected.

But regardless, I've changed the check to be more robust. And since there is really only one buggy case (where the parent node is in a different tree than the focus and anchor node), I've only included one reftest.
Comment 18 :Ehsan Akhgari 2013-11-13 10:20:55 PST
Comment on attachment 831244 [details] [diff] [review]
Selection diff

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

::: layout/reftests/webcomponents/cross-tree-selection-1.html
@@ +19,5 @@
> +      // Extend selection into a different node tree (from ShadowRoot into host).
> +      setTimeout(function() {
> +        selection.extend(host, 0);
> +        document.documentElement.className = '';
> +      }, 100);

Why 100?  I think 0 should do the job here, right?
Comment 19 William Chen [:wchen] 2013-12-02 02:27:49 PST
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #18)
> Comment on attachment 831244 [details] [diff] [review]
> Selection diff
> 
> Review of attachment 831244 [details] [diff] [review]:
> -----------------------------------------------------------------
> Why 100?  I think 0 should do the job here, right?
I was getting false positives in the old code. It looks like the selection wasn't being drawn before the selection is updated in the setTimeout when the timeout is 0.

https://hg.mozilla.org/integration/mozilla-inbound/rev/08afb5a033eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/28a139b0cf38

Note You need to log in before you can comment on or make changes to this bug.