Bug 99228 - The shadow element is not reprojected to a nested shadowRoot
Summary: The shadow element is not reprojected to a nested shadowRoot
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: HasReduction
Depends on: 100353
Blocks: 96986
  Show dependency treegraph
 
Reported: 2012-10-12 18:18 PDT by Steve Orvell
Modified: 2012-10-28 21:08 PDT (History)
14 users (show)

See Also:


Attachments
Reduction showing reprojection of shadow element with expected output. (750 bytes, text/html)
2012-10-12 18:18 PDT, Steve Orvell
no flags Details
WIP (14.41 KB, patch)
2012-10-23 02:38 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
WIP (9.90 KB, patch)
2012-10-24 00:51 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
WIP (16.00 KB, patch)
2012-10-24 03:09 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
WIP (19.51 KB, patch)
2012-10-24 18:01 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
WIP (28.65 KB, patch)
2012-10-24 23:55 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
WIP (37.01 KB, patch)
2012-10-25 16:35 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
WIP (39.77 KB, patch)
2012-10-25 18:36 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (63.19 KB, patch)
2012-10-25 19:36 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (43.79 KB, patch)
2012-10-25 19:51 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (43.73 KB, patch)
2012-10-25 21:02 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (46.20 KB, patch)
2012-10-25 21:53 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Orvell 2012-10-12 18:18:23 PDT
Created attachment 168525 [details]
Reduction showing reprojection of shadow element with expected output.

If a host has 2 shadowRoots (A-old and A) and a nested shadowRoot (B) and B's light dom contains a shadow element that should display A-old, then A-old is not distributed to an insertion point in B that should accept its contents.
Comment 1 Shinya Kawanaka 2012-10-16 22:07:11 PDT
I found that we have to change the current algorithm to reproject <shadow>.

Currently, we resolve <content> from the youngest shadow root. However, when we reproject <shadow>, we have to resolve <content> and <shadow> from the oldest shadow root, since we have to know what elements are distributed to <shadow>.

This is an example.
host ------- SR -------------- SR (younger)
 |- div (A)   |-div (C)         |-div -------- SR
 |- div (B)   |-content[B]         |-shadow     |-content[div]#target
              |-shadow

When we resolve  content[div]#target, we have to resolve all the other shadow and content.
This might be a drastic spec change.
Comment 2 Dimitri Glazkov (Google) 2012-10-17 12:45:44 PDT
I think we can punt on this for now, possibly forever.
Comment 3 Shinya Kawanaka 2012-10-17 21:48:54 PDT
(In reply to comment #2)
> I think we can punt on this for now, possibly forever.

When we adopt OOP model to understand reprojection, reprojecting shadow seems like seeing a private field of a base class. (reprojecting content is just passing arguments to super class.)

I think it would be better to postpone implementing this until we strongly require this.
Comment 4 Shinya Kawanaka 2012-10-23 00:39:20 PDT
I'm now thinking implementing this is not so hard...
Comment 5 Shinya Kawanaka 2012-10-23 02:38:00 PDT
Created attachment 170094 [details]
WIP
Comment 6 Shinya Kawanaka 2012-10-23 02:39:02 PDT
This WIP includes code related to https://bugs.webkit.org/show_bug.cgi?id=99552 also.
I've confirmed that distribution works, but we have to fix ComposedShadowTreeWalker also.
Comment 7 Dimitri Glazkov (Google) 2012-10-23 09:04:06 PDT
Comment on attachment 170094 [details]
WIP

This looks promising!
Comment 8 Shinya Kawanaka 2012-10-24 00:51:37 PDT
Created attachment 170338 [details]
WIP
Comment 9 Shinya Kawanaka 2012-10-24 00:52:38 PDT
Now shadow-reprojection works in some case, though we have a lot of failing tests.
Comment 10 Shinya Kawanaka 2012-10-24 00:54:37 PDT
<shadow> in <shadow> does not work again :-/
Comment 11 Shinya Kawanaka 2012-10-24 03:09:32 PDT
Created attachment 170352 [details]
WIP
Comment 12 Shinya Kawanaka 2012-10-24 03:11:01 PDT
The last WIP patch passes all the existing fast/dom/shadow tests. I'll add more tests and refine the code tomorrow.
Comment 13 Shinya Kawanaka 2012-10-24 18:01:22 PDT
Created attachment 170527 [details]
WIP
Comment 14 Shinya Kawanaka 2012-10-24 18:03:19 PDT
Now writing a composed shadow tree walker test, and it's discovering a lot of failures. I need more time to make it review-ready.
Comment 15 Hayato Ito 2012-10-24 23:25:36 PDT
We have to update the AncestorChainWalker also.

AncestorChainWalker::parent()

    if (!m_node->isShadowRoot()) {
        m_node = m_node->parentNode(); (*)
        m_distributedNode = m_node;
        m_isCrossingInsertionPoint = false;
        return;
    }

(*) Here, we should preserve m_node as a m_distributed node before assigning m_node->parentNode() to m_node if the following condition is met:

   m_node->parenetNode()->isShadowRoot() && toShadowRoot(m_node->parentNode())->isAssignedTo()

since m_node might be re-projected. So we should keep it as a m_distributed_node.

Let me take a look at other places also. I am afraid that we have to update other places.



(In reply to comment #14)
> Now writing a composed shadow tree walker test, and it's discovering a lot of failures. I need more time to make it review-ready.
Comment 16 Shinya Kawanaka 2012-10-24 23:55:47 PDT
Created attachment 170566 [details]
WIP
Comment 17 Hayato Ito 2012-10-24 23:57:52 PDT
I've taken a look at EventDispatcher::ensureEventAncestors.
I expect that we don't have to update EventDispatcher as long as we can calculate ancestors chain in AncestorChainWalker correctly.

(In reply to comment #15)
> We have to update the AncestorChainWalker also.
> 
> AncestorChainWalker::parent()
> 
>     if (!m_node->isShadowRoot()) {
>         m_node = m_node->parentNode(); (*)
>         m_distributedNode = m_node;
>         m_isCrossingInsertionPoint = false;
>         return;
>     }
> 
> (*) Here, we should preserve m_node as a m_distributed node before assigning m_node->parentNode() to m_node if the following condition is met:
> 
>    m_node->parenetNode()->isShadowRoot() && toShadowRoot(m_node->parentNode())->isAssignedTo()
> 
> since m_node might be re-projected. So we should keep it as a m_distributed_node.
> 
> Let me take a look at other places also. I am afraid that we have to update other places.
> 
> 
> 
> (In reply to comment #14)
> > Now writing a composed shadow tree walker test, and it's discovering a lot of failures. I need more time to make it review-ready.
Comment 18 Shinya Kawanaka 2012-10-24 23:58:32 PDT
> We have to update the AncestorChainWalker also.
> 
Done.

What we have to fix now is:
  - ComposedShadowTreeWalker::nextSibling(), previousSibling().

If you find other places where we have to fix, please tell me.
Comment 19 Early Warning System Bot 2012-10-24 23:59:53 PDT
Comment on attachment 170566 [details]
WIP

Attachment 170566 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14593126
Comment 20 Early Warning System Bot 2012-10-25 00:01:39 PDT
Comment on attachment 170566 [details]
WIP

Attachment 170566 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14555512
Comment 21 EFL EWS Bot 2012-10-25 00:27:56 PDT
Comment on attachment 170566 [details]
WIP

Attachment 170566 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14551008
Comment 22 WebKit Review Bot 2012-10-25 00:35:02 PDT
Comment on attachment 170566 [details]
WIP

Attachment 170566 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14566506
Comment 23 Peter Beverloo (cr-android ews) 2012-10-25 00:45:10 PDT
Comment on attachment 170566 [details]
WIP

Attachment 170566 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14562497
Comment 24 Hayato Ito 2012-10-25 00:45:56 PDT
Comment on attachment 170566 [details]
WIP

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

> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:67
> +static inline InsertionPoint* resolveReprojection(const Node* projectedNode, const Node* baseNode)

Do we need two parameters? it seems one parameter, projectedNode, is enough.

> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:86
> +            return resolveShadowProjection(root->assignedTo());

I am afraid that this does not resolve reprojection for shadow.
If we resolve reprojection recursively, we need two nodes as state, one is an insertion point and the other is a re-projected node.

> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:223
> +    InsertionPoint* insertionPoint = resolveReprojection(node, node);

One parameter seems to be enough.

> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:291
> +        if (InsertionPoint* insertionPoint = resolveReprojection(node, node)) {

Ditto.
Comment 25 Build Bot 2012-10-25 07:25:28 PDT
Comment on attachment 170566 [details]
WIP

Attachment 170566 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14554656
Comment 26 Shinya Kawanaka 2012-10-25 16:35:49 PDT
Created attachment 170766 [details]
WIP
Comment 27 Shinya Kawanaka 2012-10-25 16:37:23 PDT
It started to pass the tests again.
Comment 28 Shinya Kawanaka 2012-10-25 16:39:25 PDT
Comment on attachment 170766 [details]
WIP

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

> Source/WebCore/css/StyleResolver.cpp:1547
> +        }

memo: This is necessary because the direct child of shadow root can be reprojected to somewhere now. In that case, parentElement() will return 0.

> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:108
> +

This should die.
Comment 29 Shinya Kawanaka 2012-10-25 18:36:11 PDT
Created attachment 170782 [details]
WIP
Comment 30 Shinya Kawanaka 2012-10-25 18:41:46 PDT
This version works correctly including ComposedShadowTreeWalker.
I'll refine the code.
Comment 31 Shinya Kawanaka 2012-10-25 19:36:43 PDT
Created attachment 170788 [details]
Patch
Comment 32 Shinya Kawanaka 2012-10-25 19:51:46 PDT
Created attachment 170792 [details]
Patch
Comment 33 Shinya Kawanaka 2012-10-25 19:52:14 PDT
Fixed ChangeLog mojibake
Comment 34 Dimitri Glazkov (Google) 2012-10-25 20:04:42 PDT
(In reply to comment #33)
> Fixed ChangeLog mojibake

Sounds delicious.
Comment 35 Shinya Kawanaka 2012-10-25 21:02:45 PDT
Created attachment 170801 [details]
Patch
Comment 36 Shinya Kawanaka 2012-10-25 21:03:25 PDT
Minor updates: removed a debug code, and ChangeLog update.
Comment 37 Shinya Kawanaka 2012-10-25 21:53:52 PDT
Created attachment 170807 [details]
Patch
Comment 38 Shinya Kawanaka 2012-10-25 21:54:40 PDT
Added a testcase about inert InsertionPoint, and fixed a bug related to it.
Comment 39 Shinya Kawanaka 2012-10-28 17:57:36 PDT
Hmm... No one did review? :-(
Comment 40 Dimitri Glazkov (Google) 2012-10-28 18:24:58 PDT
(In reply to comment #38)
> Added a testcase about inert InsertionPoint, and fixed a bug related to it.

I'll try to get this in a couple of hours. Sorry :-\
Comment 41 Dimitri Glazkov (Google) 2012-10-28 20:57:40 PDT
Comment on attachment 170807 [details]
Patch

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

> Source/WebCore/dom/ElementShadow.cpp:135
>  InsertionPoint* ElementShadow::insertionPointFor(const Node* node) const

That method got whittled away to nothing! Can probably remove it.

> Source/WebCore/html/shadow/InsertionPoint.h:135
> +inline Element* parentElementForDistribution(const Node* node)

Is this guy still being used somewhere?
Comment 42 Shinya Kawanaka 2012-10-28 21:08:18 PDT
(In reply to comment #41)
> (From update of attachment 170807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170807&action=review
> 
> > Source/WebCore/dom/ElementShadow.cpp:135
> >  InsertionPoint* ElementShadow::insertionPointFor(const Node* node) const
> 
> That method got whittled away to nothing! Can probably remove it.
> 
> > Source/WebCore/html/shadow/InsertionPoint.h:135
> > +inline Element* parentElementForDistribution(const Node* node)
> 
> Is this guy still being used somewhere?

Thanks for reviewing!

I'll make follow-up patches after landing this.
Comment 43 WebKit Review Bot 2012-10-28 21:08:23 PDT
Comment on attachment 170807 [details]
Patch

Clearing flags on attachment: 170807

Committed r132760: <http://trac.webkit.org/changeset/132760>
Comment 44 WebKit Review Bot 2012-10-28 21:08:30 PDT
All reviewed patches have been landed.  Closing bug.