Bug 157437 – offsetLeft and offsetParent should adjust across shadow boundaries

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

When a node is assigned to a slot, its offsetParent can leak a node inside a shadow tree.

[Ryosuke Niwa]

Filed https://github.com/w3c/webcomponents/issues/497 for the spec work.

[Radar WebKit Bug Importer]

<rdar://problem/26154021>

[Ryosuke Niwa]

Created attachment 357294 [details]
WIP

[Ryosuke Niwa]

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.

[Simon Fraser (smfr)]

FYI, our offsetParent algorithm doesn't match the spec https://github.com/w3c/csswg-drafts/issues/409, bug 161788.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 357478 [details]
Fixes the implementation

[Ryosuke Niwa]

Created attachment 357503 [details]
Fixed GTK+ build failures

[EWS Watchlist]

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

[Ryosuke Niwa]

(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.

[Simon Fraser (smfr)]

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().

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Thanks for the review!

[Ryosuke Niwa]

Committed r239313: <https://trac.webkit.org/changeset/239313>