Bug 57813 - Support a list of nodes at the top level of a shadow DOM tree
Summary: Support a list of nodes at the top level of a shadow DOM tree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 52962 58432
  Show dependency treegraph
 
Reported: 2011-04-04 21:37 PDT by Dominic Cooney
Modified: 2011-04-13 04:40 PDT (History)
9 users (show)

See Also:


Attachments
WIP (39.31 KB, patch)
2011-04-04 21:42 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (54.88 KB, patch)
2011-04-06 22:12 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (53.87 KB, patch)
2011-04-07 11:44 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (54.29 KB, patch)
2011-04-07 13:26 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (54.77 KB, patch)
2011-04-07 13:36 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (54.73 KB, patch)
2011-04-07 13:53 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (56.16 KB, patch)
2011-04-07 14:52 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (56.67 KB, patch)
2011-04-07 15:04 PDT, Dominic Cooney
dglazkov: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Cooney 2011-04-04 21:37:54 PDT
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.
Comment 1 Dominic Cooney 2011-04-04 21:42:32 PDT
Created attachment 88182 [details]
WIP
Comment 2 Dominic Cooney 2011-04-04 21:43:14 PDT
Comment on attachment 88182 [details]
WIP

(Yes, no ChangeLog. Work in progress.)
Comment 3 Dominic Cooney 2011-04-04 21:45:39 PDT
This patch breaks shadow pseudo-element selectors; still looking into that. Based on r82748.
Comment 4 Dominic Cooney 2011-04-06 22:12:22 PDT
Created attachment 88572 [details]
Patch
Comment 5 Build Bot 2011-04-06 22:52:40 PDT
Attachment 88572 [details] did not build on win:
Build output: http://queues.webkit.org/results/8373006
Comment 6 Dimitri Glazkov (Google) 2011-04-07 09:21:23 PDT
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?
Comment 7 Dominic Cooney 2011-04-07 10:43:45 PDT
(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.
Comment 8 Dominic Cooney 2011-04-07 11:44:15 PDT
Created attachment 88669 [details]
Patch
Comment 9 Build Bot 2011-04-07 13:13:44 PDT
Attachment 88669 [details] did not build on win:
Build output: http://queues.webkit.org/results/8345658
Comment 10 Build Bot 2011-04-07 13:20:01 PDT
Attachment 88669 [details] did not build on win:
Build output: http://queues.webkit.org/results/8348644
Comment 11 Dominic Cooney 2011-04-07 13:26:52 PDT
Created attachment 88682 [details]
Patch
Comment 12 Dominic Cooney 2011-04-07 13:36:54 PDT
Created attachment 88685 [details]
Patch
Comment 13 Dimitri Glazkov (Google) 2011-04-07 13:37:21 PDT
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.
Comment 14 Dominic Cooney 2011-04-07 13:53:19 PDT
Created attachment 88689 [details]
Patch
Comment 15 Dominic Cooney 2011-04-07 14:09:51 PDT
Comment on attachment 88689 [details]
Patch

I believe this one is good to go.
Comment 16 WebKit Review Bot 2011-04-07 14:24:05 PDT
Attachment 88682 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8341767
Comment 17 Dominic Cooney 2011-04-07 14:35:03 PDT
Comment on attachment 88689 [details]
Patch

Breaks chromium…
Comment 18 Build Bot 2011-04-07 14:44:45 PDT
Attachment 88689 [details] did not build on win:
Build output: http://queues.webkit.org/results/8344737
Comment 19 Roland Steiner 2011-04-07 14:50:39 PDT
Drive-by minor nit: CMakeLists.txt is not listed in the ChangeLog
Comment 20 Dominic Cooney 2011-04-07 14:52:01 PDT
Created attachment 88706 [details]
Patch
Comment 21 Dominic Cooney 2011-04-07 15:04:59 PDT
Created attachment 88712 [details]
Patch

Do not break Windows.
Comment 22 Dimitri Glazkov (Google) 2011-04-07 20:17:08 PDT
Comment on attachment 88712 [details]
Patch

ok, let's give it a whirl.
Comment 23 WebKit Commit Bot 2011-04-07 21:04:07 PDT
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
Comment 24 WebKit Review Bot 2011-04-08 01:05:35 PDT
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
Comment 25 Dominic Cooney 2011-04-08 01:11:00 PDT
Bug 58121 filed for WK2 test break.

Bug 58119 filed for GTK test break.
Comment 26 Kent Tamura 2011-04-08 09:22:54 PDT
(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 :-)