Bug 99552 - The order of resolving distribution in tree composition is wrong
Summary: The order of resolving distribution in tree composition is wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on:
Blocks: 96986
  Show dependency tree / graph
 
Reported: 2012-10-17 00:20 PDT by Shinya Kawanaka
Modified: 2012-10-23 09:39 PDT (History)
2 users (show)

See Also:

Attachments
Repro (463 bytes, text/html)
2012-10-22 21:18 PDT, Shinya Kawanaka 		no flags Details
WIP (9.15 KB, patch)
2012-10-22 22:44 PDT, Shinya Kawanaka 		no flags Details | Formatted Diff | Diff
Patch (10.32 KB, patch)
2012-10-23 00:03 PDT, Shinya Kawanaka 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-10-17 00:20:47 PDT 
When we have the following shadow dom, we should resolve distribution with the SR3 -> SR2 -> SR1 -> SR4 order.

host -- SR1 -- SR2 -- SR3
                |
                |- A ------ SR4
                   |         |
                   |- B      | -- content
                   |
                   |- content
Comment 1 Shinya Kawanaka 2012-10-17 00:29:18 PDT 
Hmm... It seems the current algorithm works correctly.
Comment 2 Shinya Kawanaka 2012-10-17 05:44:06 PDT 
Hmm... I became not sure it's working correctly. At least, we have to have a test.
Comment 3 Shinya Kawanaka 2012-10-22 21:13:34 PDT 
According to the current spec (W3C Editor's Draft 23 October 2012), we have to resolve <content> first.

4. Repeat while TREE exists:
  1. Let POINT be the first encountered active shadow insertion point in TREE, in tree order
  2. Run the distribution algorithm, supplying POOL and TREE as input
  3. If POINT exists:
    ....

https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html

We're resolving both <shadow> and <content> in tree order.
Comment 4 Shinya Kawanaka 2012-10-22 21:18:48 PDT 
Created attachment 170055 [details]
Repro

We have to show like the following:
  C
  B
  A

However, the first <shadow> selects all div now.
Comment 5 Shinya Kawanaka 2012-10-22 22:29:18 PDT 
Now I have a patch for this, but this causes 30+ errors in fast/dom/shadow/.
It seems the tests are wrong...!!
Comment 6 Shinya Kawanaka 2012-10-22 22:44:00 PDT 
Created attachment 170065 [details]
WIP
Comment 7 Shinya Kawanaka 2012-10-22 22:51:21 PDT 
(In reply to comment #5)
> Now I have a patch for this, but this causes 30+ errors in fast/dom/shadow/.
> It seems the tests are wrong...!!

Oops, not all the tests are wrong. My patch still has some bug maybe.
Comment 8 Shinya Kawanaka 2012-10-22 23:56:05 PDT 
Ahhhhhhh... I have a very stupid mistake in the WIP patch :-/
Comment 9 Shinya Kawanaka 2012-10-23 00:03:18 PDT 
Created attachment 170070 [details]
Patch
Comment 10 Dimitri Glazkov (Google) 2012-10-23 09:11:04 PDT 
Comment on attachment 170070 [details]
Patch

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

> Source/WebCore/html/shadow/ContentDistributor.cpp:107
> +        if (ElementShadow* shadow = firstActiveShadowInsertionPointInOldestShadowRoot->parentNode()->isElementNode() ? toElement(firstActiveShadowInsertionPointInOldestShadowRoot->parentNode())->shadow() : 0)

I think you can turn this into a helper function -- you use it twice in this code, and it might be good to have a nice name explaining the purpose.
Comment 11 WebKit Review Bot 2012-10-23 09:39:16 PDT 
Comment on attachment 170070 [details]
Patch

Clearing flags on attachment: 170070

Committed r132237: <http://trac.webkit.org/changeset/132237>
Comment 12 WebKit Review Bot 2012-10-23 09:39:19 PDT 
All reviewed patches have been landed.  Closing bug.