Bug 48697 – Provide means to store shadow DOM nodes on Element

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dimitri Glazkov (Google)]

We need a way to associate shadow DOM subtrees with an element. Currently, it's done by hard-coding a RefPtr to a node into a custom-built RenderObject. Instead, we should move it to the Element, probably to ElementRareData?

[Alexey Proskuryakov]

Hmm, but isn't shadow tree more of a rendering concept than a DOM one? I don't know much about how it works and why, but from your description, it sounds like the current design is more logical.

[Dimitri Glazkov (Google)]

(In reply to comment #1)
> Hmm, but isn't shadow tree more of a rendering concept than a DOM one? I don't know much about how it works and why, but from your description, it sounds like the current design is more logical.

Sorry, I abbreviated description too much :)

Shadow DOM is just DOM. In no situation having DOM nodes' lifetime being managed by a RenderObject is a good thing.

Moving shadow DOM nodes to live in DOM would allow a much more natural and less hand-coded way to build and maintain elements that need shadow DOM.

[Alexey Proskuryakov]

I'm not sure if I'm convinced. Shadow DOM is not just DOM, it's an implementation detail of element's renderer. If anything, we should be further isolating it from regular DOM.

Its lifetime is quite different, too, because shadow DOM nodes never have wrappers.

[Dimitri Glazkov (Google)]

(In reply to comment #3)
> I'm not sure if I'm convinced. Shadow DOM is not just DOM, it's an implementation detail of element's renderer. If anything, we should be further isolating it from regular DOM.
> 
> Its lifetime is quite different, too, because shadow DOM nodes never have wrappers.

Whoa, I thought you were asking for clarification here, not actually arguing about validity of current design.

Shadow DOM is just DOM, period. There's nothing to argue here. The way it is implemented currently is backwards.

The absence of JS wrappers has little to do with the matter, since shadow DOM elements still participate in the rest of lifecycle, including events, attachment, detachment, tree and document insertion/removal. Getting it right in the current design is extremely difficult, tricky, and brittle. Arguing for this being hand-coded instead of using standard DOM machinery seems... Halloweeney? :)

[Alexey Proskuryakov]

Hyatt says that shadow DOM will be just DOM as we move to XBL, so I recede. I still think that for its current use, existing design is appropriate.

[Darin Adler]

(In reply to comment #0)
> We need a way to associate shadow DOM subtrees with an element. Currently, it's done by hard-coding a RefPtr to a node into a custom-built RenderObject. Instead, we should move it to the Element, probably to ElementRareData?

Given what Hyatt has said to me many times about this, I think this is OK.

Putting the root of the shadow DOM tree into ElementRareData seems OK.

[Dimitri Glazkov (Google)]

Created attachment 74035 [details]
Patch

[Dimitri Glazkov (Google)]

Created attachment 74038 [details]
Patch

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> Created an attachment (id=74038) [details]
> Patch

This one does what the bug says, but I am pursuing a slightly different approach. If it doesn't pan out, I'll flip this patch to r?.

[Kent Tamura]

Comment on attachment 74038 [details]
Patch

Looks good.  Any objection to r+?

[Hajime Morrita]

Comment on attachment 74038 [details]
Patch

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

> WebCore/dom/Element.cpp:895
> +void Element::removedFromTree(bool deep)

Just for curious: is the order correct?
In a naive view, ContainerNode::removedFromTree() should be called afte shadow->removeFromTree() because
it should be reverse order of what insertedIntoTree() does.

> WebCore/dom/Element.cpp:@
>  void Element::detach()

Same question as one for removedFromTree().

[Dimitri Glazkov (Google)]

(In reply to comment #11)
> (From update of attachment 74038 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74038&action=review
> 
> > WebCore/dom/Element.cpp:895
> > +void Element::removedFromTree(bool deep)
> 
> Just for curious: is the order correct?
> In a naive view, ContainerNode::removedFromTree() should be called afte shadow->removeFromTree() because
> it should be reverse order of what insertedIntoTree() does.
> 
> > WebCore/dom/Element.cpp:@
> >  void Element::detach()
> 
> Same question as one for removedFromTree().

Ah, good question. Think of shadowRoot as an extra child. The order in which the children are inserted/attached is the same (and not the reverse) as their removal/detachment.

[Hajime Morrita]

(In reply to comment #12)
> Ah, good question. Think of shadowRoot as an extra child. The order in which the children are inserted/attached is the same (and not the reverse) as their removal/detachment.
Makes sense. Thank you for the clarification.

[Dimitri Glazkov (Google)]

(In reply to comment #10)
> (From update of attachment 74038 [details])
> Looks good.  Any objection to r+?

Yup, this is ready for review. I thought I could make this depend on bug 50184, to make the operation of setting shadowHost/shadowRoot atomic, but turns out I can't twine them together until all current shadow DOM consumers are converted to the new model.

[Kent Tamura]

Comment on attachment 74038 [details]
Patch

ok

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:

fast/loader/recursive-before-unload-crash.html
fast/images/load-img-with-empty-src.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74038 [details]
Patch

Rejecting patch 74038 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 1
ERROR: Working directory has local commits, pass --force-clean to continue.

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

[WebKit Commit Bot]

Comment on attachment 74038 [details]
Patch

Rejecting patch 74038 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/DOMSVGAltGlyphElement.o /Projects/CommitQueue/WebKitBuild/Debug/DerivedSources/WebCore/DOMSVGAltGlyphElement.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/DOMSVGAngle.o /Projects/CommitQueue/WebKitBuild/Debug/DerivedSources/WebCore/DOMSVGAngle.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
(19 failures)


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

[Eric Seidel (no email)]

More local commits trouble, see bug 49798.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:

fast/images/load-img-with-empty-src.html
fast/preloader/script.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:

compositing/iframes/overlapped-nested-iframes.html
fast/dom/Geolocation/timestamp.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[Dimitri Glazkov (Google)]

(In reply to comment #21)
> The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:
> 
> compositing/iframes/overlapped-nested-iframes.html
> fast/dom/Geolocation/timestamp.html
> 
> Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

I don't get it. Is commit queue choking on this patch? Does anybody know how to do a bot-Heimlich?

[Eric Seidel (no email)]

The tree has been perma-red, and there are at least 3 badly flaky tests at the moment.  Basically the cq is useless when the tree is this bad.  If someone fixes it, the queue and bots will start workign again. :)

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:

fast/frames/flattening/iframe-flattening-fixed-height.html
canvas/philip/tests/2d.text.draw.align.center.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74038 [details]
Patch

Clearing flags on attachment: 74038

Committed r73114: <http://trac.webkit.org/changeset/73114>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:

animations/combo-transform-translate+scale.html
fast/preloader/script.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.