Bug 62202 - [ShadowContentElement] should layout child whitespace between span
Summary: [ShadowContentElement] should layout child whitespace between span
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on: 62428
Blocks: 59802
  Show dependency treegraph
 
Reported: 2011-06-07 04:19 PDT by Hajime Morrita
Modified: 2011-06-23 18:35 PDT (History)
4 users (show)

See Also:


Attachments
A repro (513 bytes, text/html)
2011-06-07 04:19 PDT, Hajime Morrita
no flags Details
Patch (13.92 KB, patch)
2011-06-20 01:10 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.37 MB, application/zip)
2011-06-20 01:26 PDT, WebKit Review Bot
no flags Details
Patch (41.38 KB, patch)
2011-06-20 02:12 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.33 MB, application/zip)
2011-06-20 02:45 PDT, WebKit Review Bot
no flags Details
Updated expectations (44.16 KB, patch)
2011-06-20 21:01 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-06-07 04:19:07 PDT
Created attachment 96227 [details]
A repro

This is the first step for a series of <content> bug hunting.
Comment 1 Roland Steiner 2011-06-07 15:53:44 PDT
Adding a span unequivocally may not the correct thing to do in all scenarios. IOW, this again raises the question how we should treat child text nodes, and amongst them inter-element white-space.

Additionally/alternatively, we could extend <content> so that one can specify what goes in between child elements, in order for it to work similar to string join. In the simplest case, we could just use the children of the <content> element to have this meaning.
Comment 2 Hajime Morrita 2011-06-07 23:18:01 PDT
(In reply to comment #1)
> Adding a span unequivocally may not the correct thing to do in all scenarios. IOW, this again raises the question how we should treat child text nodes, and amongst them inter-element white-space.
> 
> Additionally/alternatively, we could extend <content> so that one can specify what goes in between child elements, in order for it to work similar to string join. In the simplest case, we could just use the children of the <content> element to have this meaning.
At least this doesn't work for <details>.
It should hold text node as well because using <content> is just an implementation detail.
And <details> is a benchmark for expressiveness of <content>.
Comment 3 Hajime Morrita 2011-06-07 23:19:23 PDT
child elements, in order for it to work similar to string join. In the simplest case, we could just use the children of the <content> element to have this meaning.
> At least this doesn't work for <details>.
> It should hold text node as well because using <content> is just an implementation detail.
> And <details> is a benchmark for expressiveness of <content>.
On the other hand, I found that handling text nodes correctly in forwarded context is really tricky...
Comment 4 Roland Steiner 2011-06-09 22:27:11 PDT
A simple way could be to view <content select="..."> as "yanking out" those elements from under the host node that it selects. A simple <content> then gets everything that is left over, text nodes and all.

This view of things should work for <details>, but I'm not sure if it can address all use cases.
Comment 5 Dimitri Glazkov (Google) 2011-06-10 09:56:29 PDT
(In reply to comment #4)
> A simple way could be to view <content select="..."> as "yanking out" those elements from under the host node that it selects. A simple <content> then gets everything that is left over, text nodes and all.

Yes! That's exactly how we want it.
Comment 6 Hajime Morrita 2011-06-20 01:10:04 PDT
Created attachment 97762 [details]
Patch
Comment 7 WebKit Review Bot 2011-06-20 01:26:16 PDT
Comment on attachment 97762 [details]
Patch

Attachment 97762 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8908423

New failing tests:
fast/html/details-add-child-2.html
fast/html/details-open4.html
fast/html/details-replace-text.html
fast/html/details-nested-1.html
fast/html/details-remove-child-1.html
fast/html/details-open-javascript.html
fast/html/details-no-summary4.html
fast/html/details-open2.html
fast/html/details-remove-child-2.html
fast/html/details-nested-2.html
fast/html/details-add-details-child-2.html
Comment 8 WebKit Review Bot 2011-06-20 01:26:21 PDT
Created attachment 97764 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 9 Hajime Morrita 2011-06-20 02:12:47 PDT
Created attachment 97765 [details]
Patch
Comment 10 Hajime Morrita 2011-06-20 02:14:57 PDT
Theoretically, there are more edge cases to handle. 
But <details> has its <content> elements only on certain position.
So we need to unveil  such potential cases with internals.createShadowElement().
But it's another story with another bug id...
Comment 11 WebKit Review Bot 2011-06-20 02:45:19 PDT
Comment on attachment 97765 [details]
Patch

Attachment 97765 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8911435

New failing tests:
fast/html/details-replace-text.html
Comment 12 WebKit Review Bot 2011-06-20 02:45:24 PDT
Created attachment 97768 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 13 Hajime Morrita 2011-06-20 21:01:35 PDT
Created attachment 97923 [details]
Updated expectations
Comment 14 Hajime Morrita 2011-06-21 22:50:20 PDT
Finally it's green!
Comment 15 Dimitri Glazkov (Google) 2011-06-22 08:06:14 PDT
Comment on attachment 97923 [details]
Updated expectations

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

So it seems that we are compensating for the fact that we don't have the final flattened tree concept (http://dev.w3.org/2006/xbl2/#the-final-flattened-tree). Perhaps we should look at this problem from the overall design perspective and solve it at that level. O(N) iterations like these always make me feel like there's something missing in bigger picture.

> Source/WebCore/ChangeLog:9
> +        ignored forward content tree hierarchies and used render-object

forwarded

> Source/WebCore/ChangeLog:13
> +        In this way, these methods can reflect hierarchies of forward (twilight) tree.

forwarded.

not twilight, right? Light tree I thought? Since we don't have either one defined anywhere, let's just zap the reference to it.

> Source/WebCore/dom/NodeRenderingContext.cpp:111
> +    size_t currentIndex = notFound;
> +    for (size_t i = 0; i < parent->inclusionCount(); ++i) {
> +        if (current == parent->inclusionAt(i)) {
> +            currentIndex = i;
> +            break;
> +        }
> +    }

Sounds like this should be ShadowContentElement::indexOfInclusion.

> Source/WebCore/rendering/RenderTreeAsText.cpp:763
> +        return ts.release();

Ouch.
Comment 16 Hajime Morrita 2011-06-23 17:54:15 PDT
(In reply to comment #15)
> (From update of attachment 97923 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97923&action=review
> 
> So it seems that we are compensating for the fact that we don't have the final flattened tree concept (http://dev.w3.org/2006/xbl2/#the-final-flattened-tree). Perhaps we should look at this problem from the overall design perspective and solve it at that level. O(N) iterations like these always make me feel like there's something missing in bigger picture.
Well, we will not have explicit tree structure for "final flattened tree".
We'll make some traversal abstraction instead.
For event, your EventDispatchMediator family helps (in my understanding.)
For render-tree creation, NodeRenderingContext should help.

I wonder if we should have some unified flattened tree traverser.
I don't yet have any good picture for that.
Comment 17 Hajime Morrita 2011-06-23 18:35:50 PDT
Committed r89649: <http://trac.webkit.org/changeset/89649>