Bug 209715 – scrollIntoView() erroneously scrolls non-containing block scrollers

• Component: Scrolling
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Created attachment 394857 [details]
Testcase

RenderLayer::scrollRectToVisible() walks the parent chain and calls scrollIntoView() on each enclosing layer. But this is wrong, because not all parent layers are in the containing block chain.

Attached testcase shows the bug.

[Simon Fraser (smfr)]

Needs to share code with scroll latching ancestor finding.

[Simon Fraser (smfr)]

Created attachment 394941 [details]
Patch

[Darin Adler]

Comment on attachment 394941 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:1922
> +    for (RenderLayer* nextLayer = enclosingContainingBlockLayer(*this, crossFrameBoundaries);nextLayer; nextLayer = enclosingContainingBlockLayer(*nextLayer, crossFrameBoundaries)) {

Missing space after semicolon. Also, why not use auto instead of type name here?

> Source/WebCore/rendering/RenderLayer.cpp:2896
> +                    auto* enclosingLayer = ownerElement->renderer()->enclosingLayer();
> +                    if (enclosingLayer)

Why not put this inside the if?

[Simon Fraser (smfr)]

Created attachment 394963 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 394964 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 394966 [details]
Patch

[Darin Adler]

Comment on attachment 394966 [details]
Patch

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

> Source/WebKitLegacy/win/WebView.cpp:2041
> +        if (m_gestureTargetNode && m_gestureTargetNode->renderer() && m_gestureTargetNode->renderer()->enclosingLayer()) {
> +            if (auto* layer = m_gestureTargetNode->renderer()->enclosingLayer())

Seems like a double null check on enclosingLayer.

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #7)
> Comment on attachment 394966 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394966&action=review
> 
> > Source/WebKitLegacy/win/WebView.cpp:2041
> > +        if (m_gestureTargetNode && m_gestureTargetNode->renderer() && m_gestureTargetNode->renderer()->enclosingLayer()) {
> > +            if (auto* layer = m_gestureTargetNode->renderer()->enclosingLayer())
> 
> Seems like a double null check on enclosingLayer.

Oops yeah. Will fix.

[Simon Fraser (smfr)]

Created attachment 394971 [details]
Patch

[EWS]

Committed r259248: <https://trac.webkit.org/changeset/259248>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394971 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61081100>