Bug 58914 - Content of <summary> should be forwarded through the shadow DOM
Summary: Content of <summary> should be forwarded through the shadow DOM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 56967 56973
  Show dependency treegraph
 
Reported: 2011-04-19 12:02 PDT by Hajime Morrita
Modified: 2011-04-21 10:34 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-04-19 12:02:08 PDT
We are going to introduce <content> like mechanics to handle this.
Comment 1 Hajime Morrita 2011-04-20 14:36:41 PDT
Created attachment 90418 [details]
WIP
Comment 2 Hajime Morrita 2011-04-20 15:49:51 PDT
Created attachment 90428 [details]
WIP Take2
Comment 3 Hajime Morrita 2011-04-20 18:37:14 PDT
Created attachment 90469 [details]
WIP Take3 - Added tests, almost ready.
Comment 4 Hajime Morrita 2011-04-20 18:51:16 PDT
Created attachment 90471 [details]
Ready.
Comment 5 Hajime Morrita 2011-04-20 19:00:54 PDT
Created attachment 90476 [details]
Patch
Comment 6 Dimitri Glazkov (Google) 2011-04-20 20:47:23 PDT
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.
Comment 7 Dimitri Glazkov (Google) 2011-04-20 20:51:48 PDT
Also,
Comment 8 Dimitri Glazkov (Google) 2011-04-20 20:52:09 PDT
(In reply to comment #7)
> Also,

... please adjust the title in the ChangeLog.
Comment 9 Dimitri Glazkov (Google) 2011-04-20 20:58:16 PDT
Comment on attachment 90476 [details]
Patch

Let discuss this in person tomorrow.
Comment 10 Dimitri Glazkov (Google) 2011-04-21 08:54:31 PDT
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.
Comment 11 Hajime Morrita 2011-04-21 09:52:07 PDT
Committed r84511: <http://trac.webkit.org/changeset/84511>
Comment 12 Matthew Delaney 2011-04-21 09:57:06 PDT
Committed r84512: <http://trac.webkit.org/changeset/84512>
Comment 13 WebKit Review Bot 2011-04-21 10:34:20 PDT
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 WebKit Review Bot 2011-04-21 10:34:25 PDT
http://trac.webkit.org/changeset/84512 might have broken Qt Linux Release
The following tests are not passing:
fast/html/details-open2.html