Bug 157437

Summary: offsetLeft and offsetParent should adjust across shadow boundaries
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cdumez, cgarcia, cmarcelo, dbates, dino, eoconnor, esprehn+autocc, ews-watchlist, gustavo, gyuyoung.kim, kangil.han, koivisto, kondapallykalyan, mcatanzaro, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 148695    
Attachments:
Description Flags
WIP
none
Fixes the implementation
none
Fixed GTK+ build failures simon.fraser: review+

Description Ryosuke Niwa 2016-05-06 17:12:25 PDT
When a node is assigned to a slot, its offsetParent can leak a node inside a shadow tree.
Comment 1 Ryosuke Niwa 2016-05-06 19:32:30 PDT
Filed https://github.com/w3c/webcomponents/issues/497 for the spec work.
Comment 2 Radar WebKit Bug Importer 2016-05-06 19:33:23 PDT
<rdar://problem/26154021>
Comment 3 Ryosuke Niwa 2018-12-13 20:20:30 PST
Created attachment 357294 [details]
WIP
Comment 4 Ryosuke Niwa 2018-12-13 20:21:44 PST
Almost there but I still have a bug that my loop can skip over the offset parent. Basically, we need to find the first offset parent which is a shadow inclusive ancestor so I might just need to check that directly instead of relying on the retargeting algorithm.
Comment 5 Simon Fraser (smfr) 2018-12-14 10:18:26 PST
FYI, our offsetParent algorithm doesn't match the spec https://github.com/w3c/csswg-drafts/issues/409, bug 161788.
Comment 6 Ryosuke Niwa 2018-12-17 13:14:41 PST
(In reply to Simon Fraser (smfr) from comment #5)
> FYI, our offsetParent algorithm doesn't match the spec
> https://github.com/w3c/csswg-drafts/issues/409, bug 161788.

Oh, interesting. This patch mostly preserves the behavior of offsetParent though. Any quirks it has will be preserved. I'm basically walking offset parents until I find the shadow-including ancestor node.
Comment 7 Ryosuke Niwa 2018-12-17 14:39:06 PST
Created attachment 357478 [details]
Fixes the implementation
Comment 8 Ryosuke Niwa 2018-12-17 17:01:30 PST
Created attachment 357503 [details]
Fixed GTK+ build failures
Comment 9 EWS Watchlist 2018-12-17 17:02:27 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 10 Ryosuke Niwa 2018-12-17 18:19:24 PST
(In reply to Build Bot from comment #9)
> Thanks for the patch. If this patch contains new public API please make sure
> it follows the guidelines for new WebKit2 GTK+ API. See
> http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Note that this patch does not add or remove a new GTK+ API. It simply updates its implementation to match the JavaScript behavior.
Comment 11 Simon Fraser (smfr) 2018-12-17 18:32:18 PST
Comment on attachment 357503 [details]
Fixed GTK+ build failures

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

> Source/WebCore/ChangeLog:9
> +        Updates our implementation of offsetLeft, offsetTop, and offsetParent to match the latest discussion in CSS WG.

Make it clear this is about offset* in shadow roots; you're not fixing the offsetParent algorithm in general.

> Source/WebCore/dom/Element.cpp:914
> +    auto parent = makeRefPtr(offsetParent());
> +    if (!parent || !parent->isInShadowTree())
> +        return offsetLeft();
> +
> +    ASSERT(&parent->document() == &document());
> +    if (&parent->treeScope() == &treeScope())
> +        return offsetLeft();
> +
> +    double offset = offsetLeft();

It would be nice to avoid the 3 separate call sites for offsetLeft(). You could move 'double offset = offsetLeft();' to after the updateLayoutIgnorePendingStylesheets().

> Source/WebCore/dom/Element.cpp:926
> +    document().updateLayoutIgnorePendingStylesheets();

Sucks that both offsetLeft and offsetLeftForBindings() call updateLayoutIgnorePendingStylesheets().
Comment 12 Ryosuke Niwa 2018-12-17 18:35:58 PST
(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 357503 [details]
> Fixed GTK+ build failures
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357503&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Updates our implementation of offsetLeft, offsetTop, and offsetParent to match the latest discussion in CSS WG.
> 
> Make it clear this is about offset* in shadow roots; you're not fixing the
> offsetParent algorithm in general.

Phrased as follows:

Update the WebKit's treatment of shadow boundaries in offsetLeft, offsetTop, and offsetParent to match the latest discussion in CSS WG.

> > Source/WebCore/dom/Element.cpp:914
> > +    auto parent = makeRefPtr(offsetParent());
> > +    if (!parent || !parent->isInShadowTree())
> > +        return offsetLeft();
> > +
> > +    ASSERT(&parent->document() == &document());
> > +    if (&parent->treeScope() == &treeScope())
> > +        return offsetLeft();
> > +
> > +    double offset = offsetLeft();
> 
> It would be nice to avoid the 3 separate call sites for offsetLeft(). You
> could move 'double offset = offsetLeft();' to after the
> updateLayoutIgnorePendingStylesheets().

Done that.

> > Source/WebCore/dom/Element.cpp:926
> > +    document().updateLayoutIgnorePendingStylesheets();
> 
> Sucks that both offsetLeft and offsetLeftForBindings() call
> updateLayoutIgnorePendingStylesheets().

We don't need this now that we always call offsetTop/offsetLeft upfront.
Comment 13 Ryosuke Niwa 2018-12-17 18:37:27 PST
Thanks for the review!
Comment 14 Ryosuke Niwa 2018-12-17 19:52:55 PST
Committed r239313: <https://trac.webkit.org/changeset/239313>