| Summary: | The shadow element is not reprojected to a nested shadowRoot | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Steve Orvell <sorvell> | ||||||||||||||||||||||||||
| Component: | DOM | Assignee: | Shinya Kawanaka <shinyak> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | allan.jensen, cmarcelo, dglazkov, dominicc, gustavo, hayato, macpherson, menard, peter+ews, philn, shinyak, webcomponents-bugzilla, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | HasReduction | ||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||
| Bug Depends on: | 100353 | ||||||||||||||||||||||||||||
| Bug Blocks: | 96986 | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
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.
I think we can punt on this for now, possibly forever. (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. I'm now thinking implementing this is not so hard... Created attachment 170094 [details]
WIP
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 on attachment 170094 [details]
WIP
This looks promising!
Created attachment 170338 [details]
WIP
Now shadow-reprojection works in some case, though we have a lot of failing tests. <shadow> in <shadow> does not work again :-/ Created attachment 170352 [details]
WIP
The last WIP patch passes all the existing fast/dom/shadow tests. I'll add more tests and refine the code tomorrow. Created attachment 170527 [details]
WIP
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. 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.
Created attachment 170566 [details]
WIP
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. > 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 on attachment 170566 [details] WIP Attachment 170566 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14593126 Comment on attachment 170566 [details] WIP Attachment 170566 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14555512 Comment on attachment 170566 [details] WIP Attachment 170566 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14551008 Comment on attachment 170566 [details] WIP Attachment 170566 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14566506 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 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 on attachment 170566 [details] WIP Attachment 170566 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14554656 Created attachment 170766 [details]
WIP
It started to pass the tests again. 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. Created attachment 170782 [details]
WIP
This version works correctly including ComposedShadowTreeWalker. I'll refine the code. Created attachment 170788 [details]
Patch
Created attachment 170792 [details]
Patch
Fixed ChangeLog mojibake (In reply to comment #33) > Fixed ChangeLog mojibake Sounds delicious. Created attachment 170801 [details]
Patch
Minor updates: removed a debug code, and ChangeLog update. Created attachment 170807 [details]
Patch
Added a testcase about inert InsertionPoint, and fixed a bug related to it. Hmm... No one did review? :-( (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 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? (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 on attachment 170807 [details] Patch Clearing flags on attachment: 170807 Committed r132760: <http://trac.webkit.org/changeset/132760> All reviewed patches have been landed. Closing bug. |
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.