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 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 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).

(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 on attachment 778071 [details] [diff] [review] Part 3: Implement ShadowRoot style methods. Moving the style patches to Bug 929885.

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 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! :-)

(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! :-)

(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 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.

(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 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?

(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

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