Support a list of nodes at the top level of a shadow DOM tree

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dominic Cooney]

Element::setShadowRoot takes an Node. However Element is already a container, so it should permit multiple nodes at the top level of a shadow. This is more flexible for internal shadow (validation message could use this) and required to implement an XBL-style component model.

[Dominic Cooney]

Created attachment 88182 [details]
WIP

[Dominic Cooney]

Comment on attachment 88182 [details]
WIP

(Yes, no ChangeLog. Work in progress.)

[Dominic Cooney]

This patch breaks shadow pseudo-element selectors; still looking into that. Based on r82748.

[Dominic Cooney]

Created attachment 88572 [details]
Patch

[Build Bot]

Attachment 88572 [details] did not build on win:
Build output: http://queues.webkit.org/results/8373006

[Dimitri Glazkov (Google)]

Comment on attachment 88572 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88572&action=review

I wonder if to minimize impact, the first patch should retain shadowRoot as Node* and internalize creation and managing of ShadowRoot inside setShadowRoot?

We need to look at SVGShadowTreeRootElement and consider converting it to ShadowRoot. Not right now, but as some next step.

> Source/WebCore/dom/Element.cpp:1053
> +    bool hasParentStyle = parentNodeForRendering() ? parentNodeForRendering()->renderStyle() : false;

Because it's here, I wonder if -ForRendering is the wrong suffix. It's more like ForRenderingAndStyle.

> Source/WebCore/dom/Element.cpp:1145
> +Node* Element::shadowRoot() const

Return ShadowRoot*?

> Source/WebCore/dom/Element.cpp:1150
>  void Element::setShadowRoot(PassRefPtr<Node> node)

Can this just take ShadowRoot?

> Source/WebCore/dom/Node.cpp:1425
> +        || (!parentRenderer->canHaveChildren()
> +            && !(isShadowRoot() || parentNode()->isShadowBoundary()))

Keep this one one line.

> Source/WebCore/dom/Node.h:206
> +    // FIXME: remove this when all shadow roots are ShadowRoots

Period at the end of a sentence.

> Source/WebCore/html/HTMLKeygenElement.cpp:81
> +    shadow->parserAddChild(select);

We are not the parser. We shouldn't be calling this. Use appendChild.

> Source/WebCore/html/HTMLMeterElement.cpp:235
> +    shadow->parserAddChild(bar);

Ditto.

> Source/WebCore/html/HTMLProgressElement.cpp:141
> +    bar->parserAddChild(m_value);

Ditto.

> Source/WebCore/html/HTMLProgressElement.cpp:143
> +    shadow->parserAddChild(bar);

Ditto.

> Source/WebCore/html/RangeInputType.cpp:201
> +    shadow->parserAddChild(SliderThumbElement::create(element()->document()));

Use appendChild.

> Source/WebCore/rendering/RenderSlider.cpp:181
> +    return toSliderThumbElement(static_cast<Element*>(node())->shadowRoot()->firstChild());

Probably need to check for shadowRoot existence here first.

> Source/WebKit/mac/DOM/WebDOMOperations.mm:48
> +#import <WebCore/ShadowFragment.h>

ShadowRoot?

> Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleNodeHandle.cpp:43
> +#include <WebCore/ShadowFragment.h>

ShadowRoot?

[Dominic Cooney]

(In reply to comment #6)
> (From update of attachment 88572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88572&action=review
> 
> I wonder if to minimize impact, the first patch should retain shadowRoot as Node* and internalize creation and managing of ShadowRoot inside setShadowRoot?

I could do this, but I like it this way because now internal shadows can start to use multiple children in shadows.

Also, if we make setShadowRoot() create the ShadowRoot, that breaks symmetry with shadowRoot(). We could make shadowRoot() do [ShadowRoot instance]->firstChild(), but it feels like back to square one.

> We need to look at SVGShadowTreeRootElement and consider converting it to ShadowRoot. Not right now, but as some next step.

+1

> 
> > Source/WebCore/dom/Element.cpp:1053
> > +    bool hasParentStyle = parentNodeForRendering() ? parentNodeForRendering()->renderStyle() : false;
> 
> Because it's here, I wonder if -ForRendering is the wrong suffix. It's more like ForRenderingAndStyle.

Done

> > Source/WebCore/dom/Element.cpp:1145
> > +Node* Element::shadowRoot() const
> 
> Return ShadowRoot*?

I would like to, but per your comments on the first patch, the patch bloats a bit if I do this, because shadowRoot() callers need to #include "ShadowRoot.h" to grok ShadowRoot* is assignable to Node*. Happy to make it so… WDYT?

> > Source/WebCore/dom/Element.cpp:1150
> >  void Element::setShadowRoot(PassRefPtr<Node> node)
> 
> Can this just take ShadowRoot?

Done. Does it make sense to make shadowRoot return ShadowRoot* for symmetry?
 
> > Source/WebCore/dom/Node.cpp:1425
> > +        || (!parentRenderer->canHaveChildren()
> > +            && !(isShadowRoot() || parentNode()->isShadowBoundary()))
> 
> Keep this one one line.

Done.

> > Source/WebCore/dom/Node.h:206
> > +    // FIXME: remove this when all shadow roots are ShadowRoots
> 
> Period at the end of a sentence.

Done.

> > Source/WebCore/html/HTMLKeygenElement.cpp:81
> > +    shadow->parserAddChild(select);
> 
> We are not the parser. We shouldn't be calling this. Use appendChild.

Done.

> > Source/WebCore/html/HTMLMeterElement.cpp:235
> > +    shadow->parserAddChild(bar);
> 
> Ditto.

Done.

> > Source/WebCore/html/HTMLProgressElement.cpp:141
> > +    bar->parserAddChild(m_value);
> 
> Ditto.

Done.

> > Source/WebCore/html/HTMLProgressElement.cpp:143
> > +    shadow->parserAddChild(bar);
> 
> Ditto.

Done.

> > Source/WebCore/html/RangeInputType.cpp:201
> > +    shadow->parserAddChild(SliderThumbElement::create(element()->document()));
> 
> Use appendChild.

Done.

> > Source/WebCore/rendering/RenderSlider.cpp:181
> > +    return toSliderThumbElement(static_cast<Element*>(node())->shadowRoot()->firstChild());
> 
> Probably need to check for shadowRoot existence here first.

Done. Added an assertion to HTMLKeygenElement::shadowSelect, since that should always exist.

> > Source/WebKit/mac/DOM/WebDOMOperations.mm:48
> > +#import <WebCore/ShadowFragment.h>
> 
> ShadowRoot?

My hair just turned white. I will do a clean build and rebuild the test shells.

> > Source/WebKit2/WebProcess/InjectedBundle/DOM/InjectedBundleNodeHandle.cpp:43
> > +#include <WebCore/ShadowFragment.h>
> 
> ShadowRoot?

Done.

New patch soon.

[Dominic Cooney]

Created attachment 88669 [details]
Patch

[Build Bot]

Attachment 88669 [details] did not build on win:
Build output: http://queues.webkit.org/results/8345658

[Build Bot]

Attachment 88669 [details] did not build on win:
Build output: http://queues.webkit.org/results/8348644

[Dominic Cooney]

Created attachment 88682 [details]
Patch

[Dominic Cooney]

Created attachment 88685 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 88682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88682&action=review

Super close! One more rinse cycle.

> Source/WebCore/dom/ShadowRoot.cpp:35
> +PassRefPtr<ShadowRoot> ShadowRoot::create(Document* document)
> +{
> +    return adoptRef(new ShadowRoot(document));
> +}

This can be inlined in the header. See http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/shadow/SliderThumbElement.h&l=76.

> Source/WebCore/html/HTMLKeygenElement.cpp:130
> +    return static_cast<HTMLSelectElement*>(shadowRoot()->firstChild());

I'd do the same check for shadowRoot() here as you did for slider thumb.

> Source/WebCore/html/RangeInputType.cpp:200
> +    RefPtr<ShadowRoot> shadow = ShadowRoot::create(element()->document());

Whoops. This one's not used anymore.

[Dominic Cooney]

Created attachment 88689 [details]
Patch

[Dominic Cooney]

Comment on attachment 88689 [details]
Patch

I believe this one is good to go.

[WebKit Review Bot]

Attachment 88682 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8341767

[Dominic Cooney]

Comment on attachment 88689 [details]
Patch

Breaks chromium…

[Build Bot]

Attachment 88689 [details] did not build on win:
Build output: http://queues.webkit.org/results/8344737

[Roland Steiner]

Drive-by minor nit: CMakeLists.txt is not listed in the ChangeLog

[Dominic Cooney]

Created attachment 88706 [details]
Patch

[Dominic Cooney]

Created attachment 88712 [details]
Patch

Do not break Windows.

[Dimitri Glazkov (Google)]

Comment on attachment 88712 [details]
Patch

ok, let's give it a whirl.

[WebKit Commit Bot]

Comment on attachment 88712 [details]
Patch

Rejecting attachment 88712 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
ng file Source/WebCore/html/shadow/SliderThumbElement.h
patching file Source/WebCore/rendering/MediaControlElements.cpp
patching file Source/WebCore/rendering/RenderSlider.cpp
patching file Source/WebCore/rendering/RenderSlider.h
patching file Source/WebKit/chromium/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/chromium/src/WebElement.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Dimitri Glazkov', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/8346842

[WebKit Review Bot]

http://trac.webkit.org/changeset/83256 might have broken SnowLeopard Intel Release (WebKit2 Tests)
The following tests are not passing:
fast/dom/HTMLKeygenElement/keygen.html

[Dominic Cooney]

Bug 58121 filed for WK2 test break.

Bug 58119 filed for GTK test break.

[Kent Tamura]

(In reply to comment #24)
> http://trac.webkit.org/changeset/83256 might have broken SnowLeopard Intel Release (WebKit2 Tests)
> The following tests are not passing:
> fast/dom/HTMLKeygenElement/keygen.html

I forgot to mention that I landed the patch as r83256 :-)