Bug 58914 – Content of <summary> should be forwarded through the shadow DOM

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

We are going to introduce <content> like mechanics to handle this.

[Hajime Morrita]

Created attachment 90418 [details]
WIP

[Hajime Morrita]

Created attachment 90428 [details]
WIP Take2

[Hajime Morrita]

Created attachment 90469 [details]
WIP Take3 - Added tests, almost ready.

[Hajime Morrita]

Created attachment 90471 [details]
Ready.

[Hajime Morrita]

Created attachment 90476 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 90476 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        - Introduced ShadowContentElement which hosts forwarded childrem of <summary>

childrem->children

> Source/WebCore/ChangeLog:11
> +        - The parent look up is also aware of node forwarding. If the visual parent node has

look up -> lookup

> Source/WebCore/ChangeLog:12
> +          a shadow root, the node is possibly forwarde to ShadowContentElement

forwarde -> forwarded

> Source/WebCore/dom/NodeVisualParentLookupResult.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

At least some of the code here comes from Node.cpp, so we should probably clone the copyrights.

> Source/WebCore/dom/NodeVisualParentLookupResult.h:34
> +class NodeVisualParentLookupResult {

The name is awkward. The point of the class is to determine the parent node for rendering purposes and whether a renderer for it is necessary. So perhaps NodeRendererFinder? Or something like that.

> Source/WebCore/dom/NodeVisualParentLookupResult.h:51
> +    ContainerNode* found() const { return m_found; }

parentNodeForRenderingAndStyle?

> Source/WebCore/dom/ShadowRoot.cpp:101
> +void ShadowRoot::didHostChildrenChanged()

hostChildrenChanged

> Source/WebCore/dom/ShadowRoot.cpp:119
> +    for (Node* n = firstChild(); n; n = n->traverseNextNode(this)) {
> +        // FIXME: This should be replaced with tag-name checking once <content> is ready.
> +        // See also http://webkit.org/b/56973
> +        if (n->isShadowBoundary())
> +            return toContainerNode(n);
> +    }

This really bugs me, but I don't know if my idea would work. It seems that instead of re-traversing this all the time during child renderer creation, we should store the created renderers in a map, and then pull from that map as we create the renderer for <content> element.

[Dimitri Glazkov (Google)]

Also,

[Dimitri Glazkov (Google)]

(In reply to comment #7)
> Also,

... please adjust the title in the ChangeLog.

[Dimitri Glazkov (Google)]

Comment on attachment 90476 [details]
Patch

Let discuss this in person tomorrow.

[Dimitri Glazkov (Google)]

Comment on attachment 90476 [details]
Patch

I slept on it and I am now generally okay with the approach as a first stab. I would still like to explore the role of <content> being more active:
1) light children attach first, but all of their renderers are stashed in to a map
2) shadow DOM attaches next
3) any time a <content> element is found, it looks in the map, collects matching renderers, and inserts them into the render tree in its place.

Also, keep the new class in Node.cpp for now. We can split it out later. NodeRenderParentDetector seems like a good name.

[Hajime Morrita]

Committed r84511: <http://trac.webkit.org/changeset/84511>

[Matthew Delaney]

Committed r84512: <http://trac.webkit.org/changeset/84512>

[WebKit Review Bot]

http://trac.webkit.org/changeset/84511 might have broken Qt Linux Release
The following tests are not passing:
fast/html/details-open2.html

[WebKit Review Bot]

http://trac.webkit.org/changeset/84512 might have broken Qt Linux Release
The following tests are not passing:
fast/html/details-open2.html