Bug 58914 - Content of <summary> should be forwarded through the shadow DOM
: Content of <summary> should be forwarded through the shadow DOM
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 56967 56973
  Show dependency treegraph
 
Reported: 2011-04-19 12:02 PST by
Modified: 2011-04-21 10:34 PST (History)


Attachments
WIP (16.73 KB, patch)
2011-04-20 14:36 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
WIP Take2 (13.60 KB, patch)
2011-04-20 15:49 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
WIP Take3 - Added tests, almost ready. (58.33 KB, patch)
2011-04-20 18:37 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Ready. (54.45 KB, patch)
2011-04-20 18:51 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (62.12 KB, patch)
2011-04-20 19:00 PST, Hajime Morrita
dglazkov: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-04-19 12:02:08 PST
We are going to introduce <content> like mechanics to handle this.
------- Comment #1 From 2011-04-20 14:36:41 PST -------
Created an attachment (id=90418) [details]
WIP
------- Comment #2 From 2011-04-20 15:49:51 PST -------
Created an attachment (id=90428) [details]
WIP Take2
------- Comment #3 From 2011-04-20 18:37:14 PST -------
Created an attachment (id=90469) [details]
WIP Take3 - Added tests, almost ready.
------- Comment #4 From 2011-04-20 18:51:16 PST -------
Created an attachment (id=90471) [details]
Ready.
------- Comment #5 From 2011-04-20 19:00:54 PST -------
Created an attachment (id=90476) [details]
Patch
------- Comment #6 From 2011-04-20 20:47:23 PST -------
(From update of attachment 90476 [details])
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.
------- Comment #7 From 2011-04-20 20:51:48 PST -------
Also,
------- Comment #8 From 2011-04-20 20:52:09 PST -------
(In reply to comment #7)
> Also,

... please adjust the title in the ChangeLog.
------- Comment #9 From 2011-04-20 20:58:16 PST -------
(From update of attachment 90476 [details])
Let discuss this in person tomorrow.
------- Comment #10 From 2011-04-21 08:54:31 PST -------
(From update of attachment 90476 [details])
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.
------- Comment #11 From 2011-04-21 09:52:07 PST -------
Committed r84511: <http://trac.webkit.org/changeset/84511>
------- Comment #12 From 2011-04-21 09:57:06 PST -------
Committed r84512: <http://trac.webkit.org/changeset/84512>
------- Comment #13 From 2011-04-21 10:34:20 PST -------
http://trac.webkit.org/changeset/84511 might have broken Qt Linux Release
The following tests are not passing:
fast/html/details-open2.html
------- Comment #14 From 2011-04-21 10:34:25 PST -------
http://trac.webkit.org/changeset/84512 might have broken Qt Linux Release
The following tests are not passing:
fast/html/details-open2.html