WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
78596
<shadow> should be rendered correctly
https://bugs.webkit.org/show_bug.cgi?id=78596
Summary
<shadow> should be rendered correctly
Shinya Kawanaka
Reported
2012-02-14 03:09:21 PST
<shadow> should render an older shadow subtree.
Attachments
WIP
(87.36 KB, patch)
2012-02-22 03:50 PST
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
WIP
(70.32 KB, patch)
2012-02-23 22:57 PST
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
Patch
(67.77 KB, patch)
2012-02-28 22:56 PST
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
WIP
(58.42 KB, patch)
2012-03-01 00:43 PST
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
WIP
(57.97 KB, patch)
2012-03-01 01:02 PST
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
Patch
(57.97 KB, patch)
2012-03-01 01:15 PST
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
WIP
(46.31 KB, patch)
2012-03-05 00:44 PST
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
Patch
(49.39 KB, patch)
2012-03-06 23:11 PST
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
Patch
(49.22 KB, patch)
2012-03-06 23:23 PST
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
Patch
(49.33 KB, patch)
2012-03-07 00:55 PST
,
Shinya Kawanaka
no flags
Details
Formatted Diff
Diff
Patch
(49.09 KB, patch)
2012-03-08 00:53 PST
,
Shinya Kawanaka
morrita
: review+
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1
2012-02-17 00:16:54 PST
This will be easily done if my InsertionPoint refactoring. In that case, <shadow> should be derived from InsertionPoint.
Shinya Kawanaka
Comment 2
2012-02-22 03:50:40 PST
Created
attachment 128173
[details]
WIP
Shinya Kawanaka
Comment 3
2012-02-23 22:57:03 PST
Created
attachment 128657
[details]
WIP
WebKit Review Bot
Comment 4
2012-02-23 23:34:16 PST
Comment on
attachment 128657
[details]
WIP
Attachment 128657
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/11603713
Early Warning System Bot
Comment 5
2012-02-23 23:42:51 PST
Comment on
attachment 128657
[details]
WIP
Attachment 128657
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/11576739
Philippe Normand
Comment 6
2012-02-23 23:49:07 PST
Comment on
attachment 128657
[details]
WIP
Attachment 128657
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/11608723
WebKit Review Bot
Comment 7
2012-02-24 00:16:53 PST
Comment on
attachment 128657
[details]
WIP
Attachment 128657
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/11574752
Gyuyoung Kim
Comment 8
2012-02-24 00:40:31 PST
Comment on
attachment 128657
[details]
WIP
Attachment 128657
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/11599720
Shinya Kawanaka
Comment 9
2012-02-28 22:56:17 PST
Created
attachment 129394
[details]
Patch
Hajime Morrita
Comment 10
2012-02-28 23:14:01 PST
Comment on
attachment 129394
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=129394&action=review
> Source/WebCore/ChangeLog:19 > +
Please split this up to these three changes if possible. This patch is simply to big. You can use this bug as a metabug.
> Source/WebCore/html/shadow/HTMLShadowElement.h:56 > + ShadowRoot* distributedShadowRoot;
Please follow the naming guideline.
Shinya Kawanaka
Comment 11
2012-02-29 01:12:28 PST
Let me try to split the patch.
Shinya Kawanaka
Comment 12
2012-03-01 00:43:57 PST
Created
attachment 129654
[details]
WIP
Shinya Kawanaka
Comment 13
2012-03-01 01:02:54 PST
Created
attachment 129658
[details]
WIP
Shinya Kawanaka
Comment 14
2012-03-01 01:15:51 PST
Created
attachment 129661
[details]
Patch
Hajime Morrita
Comment 15
2012-03-01 01:25:46 PST
Comment on
attachment 129661
[details]
Patch - I don't think we need to hide HTMLShadowElement behind the flag it can be like as HTMLContentElement. - Pelase split firstRendererOf() and lastRendererOf() refactoring in separate patch. Having it in the same change makes it really hard to see the actual change.
Dimitri Glazkov (Google)
Comment 16
2012-03-01 09:26:10 PST
(In reply to
comment #15
)
> (From update of
attachment 129661
[details]
) > - I don't think we need to hide HTMLShadowElement behind the flag it can be like as HTMLContentElement.
I think we do need this behind flag. Otherwise, there are visible effects of the Shadow DOM implementation even when the flag is enabled. See
bug 79935
for details.
> - Pelase split firstRendererOf() and lastRendererOf() refactoring in separate patch. > Having it in the same change makes it really hard to see the actual change.
Hajime Morrita
Comment 17
2012-03-01 22:56:59 PST
(In reply to
comment #16
)
> (In reply to
comment #15
) > > (From update of
attachment 129661
[details]
[details]) > > - I don't think we need to hide HTMLShadowElement behind the flag it can be like as HTMLContentElement. > > I think we do need this behind flag. Otherwise, there are visible effects of the Shadow DOM implementation even when the flag is enabled. See
bug 79935
for details. > > > - Pelase split firstRendererOf() and lastRendererOf() refactoring in separate patch. > > Having it in the same change makes it really hard to see the actual change.
Let me clarify, I think: - It's OK to have HTMLShadowElement.cpp/h files for regardless of ENABLE(SHADOW_DOM). - It's NOT ok to have HTMLShadowElement.idl compiled in unless we ENABLE(SHADOW_DOM). - For tag name, I think we need to fix
Bug 79935
WITHOUT hiding HTMLContentElement.h/cpp behind ENABLE(SHADOW_DOM). It should be hidden by runtime flag, not compilation flag. I guess we need parser support here. Let me investigate this. If we can pull up most of the code from HTMLContentElement to InsertionPoint, and can implement built-in shadows using only InsertionPoint without using HTMLContentElement. That would be best.
Shinya Kawanaka
Comment 18
2012-03-05 00:44:41 PST
Created
attachment 130067
[details]
WIP
Build Bot
Comment 19
2012-03-05 01:00:33 PST
Comment on
attachment 130067
[details]
WIP
Attachment 130067
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/11803879
Build Bot
Comment 20
2012-03-05 01:02:17 PST
Comment on
attachment 130067
[details]
WIP
Attachment 130067
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/11801839
Early Warning System Bot
Comment 21
2012-03-05 01:08:52 PST
Comment on
attachment 130067
[details]
WIP
Attachment 130067
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/11802866
WebKit Review Bot
Comment 22
2012-03-05 01:17:21 PST
Comment on
attachment 130067
[details]
WIP
Attachment 130067
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/11801847
Gyuyoung Kim
Comment 23
2012-03-05 01:34:21 PST
Comment on
attachment 130067
[details]
WIP
Attachment 130067
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/11802879
Gustavo Noronha (kov)
Comment 24
2012-03-05 02:33:13 PST
Comment on
attachment 130067
[details]
WIP
Attachment 130067
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/11809291
Shinya Kawanaka
Comment 25
2012-03-06 23:11:54 PST
Created
attachment 130550
[details]
Patch
Shinya Kawanaka
Comment 26
2012-03-06 23:23:14 PST
Created
attachment 130553
[details]
Patch
Hajime Morrita
Comment 27
2012-03-06 23:58:36 PST
Comment on
attachment 130553
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=130553&action=review
> Source/WebCore/html/shadow/HTMLShadowElement.cpp:64 > + if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode()))
Why not treeScope()? shadowTreeRootNode() traverses down the tree and better be avoided if possible.
> Source/WebCore/html/shadow/InsertionPoint.cpp:52 > if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode())) {
Ditto.
> Source/WebCore/html/shadow/InsertionPoint.h:71 > + ShadowRoot* shadowRootAssignedFrom;
I don't think we need this.
> Source/WebCore/html/shadow/InsertionPoint.h:76 > if (!node || node->isContentElement())
It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint(). It also even don't need to stay in Node. can be pulled down to HTMLElement.
Shinya Kawanaka
Comment 28
2012-03-07 00:46:27 PST
> > Source/WebCore/html/shadow/InsertionPoint.h:76 > > if (!node || node->isContentElement()) > > It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint(). > It also even don't need to stay in Node. can be pulled down to HTMLElement.
Let's do this in another bug? I don't think it's good idea to merge this refactoring into this patch...
Shinya Kawanaka
Comment 29
2012-03-07 00:52:11 PST
(In reply to
comment #28
)
> > > Source/WebCore/html/shadow/InsertionPoint.h:76 > > > if (!node || node->isContentElement()) > > > > It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint(). > > It also even don't need to stay in Node. can be pulled down to HTMLElement. > > Let's do this in another bug? > I don't think it's good idea to merge this refactoring into this patch...
And I want to remove isShadowElement from Node.
Shinya Kawanaka
Comment 30
2012-03-07 00:55:53 PST
Created
attachment 130561
[details]
Patch
Shinya Kawanaka
Comment 31
2012-03-08 00:53:15 PST
Created
attachment 130791
[details]
Patch
Shinya Kawanaka
Comment 32
2012-03-08 00:55:16 PST
(In reply to
comment #27
)
> (From update of
attachment 130553
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=130553&action=review
> > > Source/WebCore/html/shadow/HTMLShadowElement.cpp:64 > > + if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode())) > > Why not treeScope()? shadowTreeRootNode() traverses down the tree and better be avoided if possible.
Done.
> > > Source/WebCore/html/shadow/InsertionPoint.cpp:52 > > if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode())) { > > Ditto.
Done.
> > > Source/WebCore/html/shadow/InsertionPoint.h:71 > > + ShadowRoot* shadowRootAssignedFrom; > > I don't think we need this.
Done.
> > > Source/WebCore/html/shadow/InsertionPoint.h:76 > > if (!node || node->isContentElement()) > > It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint(). > It also even don't need to stay in Node. can be pulled down to HTMLElement.
Already fixed in another patch
Hajime Morrita
Comment 33
2012-03-08 02:02:31 PST
Comment on
attachment 130791
[details]
Patch Looks good. Let's land this once bots become green.
Shinya Kawanaka
Comment 34
2012-03-08 03:25:54 PST
Committed
r110161
: <
http://trac.webkit.org/changeset/110161
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug