Bug 99552

Summary: The order of resolving distribution in tree composition is wrong
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: webcomponents-bugzilla, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 96986    
Attachments:
Description Flags
Repro
none
WIP
none
Patch none

Shinya Kawanaka
Reported 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
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
DetailsFormatted DiffDiff
Patch (10.32 KB, patch)
2012-10-23 00:03 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1 2012-10-17 00:29:18 PDT
Hmm... It seems the current algorithm works correctly.
Shinya Kawanaka
Comment 2 2012-10-17 05:44:06 PDT
Hmm... I became not sure it's working correctly. At least, we have to have a test.
Shinya Kawanaka
Comment 3 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.
Shinya Kawanaka
Comment 4 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.
Shinya Kawanaka
Comment 5 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...!!
Shinya Kawanaka
Comment 6 2012-10-22 22:44:00 PDT
Created attachment 170065 [details] WIP
Shinya Kawanaka
Comment 7 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.
Shinya Kawanaka
Comment 8 2012-10-22 23:56:05 PDT
Ahhhhhhh... I have a very stupid mistake in the WIP patch :-/
Shinya Kawanaka
Comment 9 2012-10-23 00:03:18 PDT
Created attachment 170070 [details] Patch
Dimitri Glazkov (Google)
Comment 10 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.
WebKit Review Bot
Comment 11 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>
WebKit Review Bot
Comment 12 2012-10-23 09:39:19 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.