Bug 238985 – REGRESSION(r290770): element.scrollIntoViewIfNeeded() scrolls to top even when element is already in viewport

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Nikita Vasilyev]

Created attachment 457025 [details]
[HTML] Reduction

Steps:
1. Open attached HTML reduction
2. Click anywhere on the text

Expected:
scrollTop doesn't change.

Actual:
Starting r290770, #box elements scrolls to top.

[Rob Buis]

Created attachment 457038 [details]
Patch

[Rob Buis]

Created attachment 457060 [details]
Patch

[Rob Buis]

Created attachment 457074 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Created attachment 457512 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/91797137>

[Simon Fraser (smfr)]

Comment on attachment 457512 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:2659
> +    bool intersects = exposeRect.maxX() >= visibleRect.x() && exposeRect.x() <= visibleRect.maxX();

call this intersectsInX

> Source/WebCore/rendering/RenderLayer.cpp:2664
> +        LayoutUnit intersectWidth = intersection(visibleRect, exposeRectX).width();

Seems like you could compute intersectWidth without making a rect.

> Source/WebCore/rendering/RenderLayer.cpp:-2674
> -        } else if (intersectWidth > 0) {

Keep the braces because the comment makes it a multi-line clause.

> Source/WebCore/rendering/RenderLayer.cpp:2696
> +    intersects = exposeRect.maxY() >= visibleRect.y() && exposeRect.y() <= visibleRect.maxY();

bool intersectsInY =

> Source/WebCore/rendering/RenderLayer.cpp:2700
> +        LayoutRect exposeRectY(visibleRect.x(), exposeRect.y(), visibleRect.width(), exposeRect.height());

Ditto

> Source/WebCore/rendering/RenderLayer.cpp:2710
> +        } else if (intersectHeight > 0)

Ditto

[Simon Fraser (smfr)]

<rdar://90985370>

[Rob Buis]

Created attachment 458577 [details]
Patch

[Rob Buis]

Created attachment 458590 [details]
Patch

[Rob Buis]

Created attachment 458592 [details]
Patch

[Darin Adler]

Comment on attachment 458592 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:2655
> +    bool intersectsInX = exposeRect.maxX() >= visibleRect.x() && exposeRect.x() <= visibleRect.maxX();

Does this handle the "touches but does not intersect" case correctly?

[Rob Buis]

Comment on attachment 457512 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:2659
>> +    bool intersects = exposeRect.maxX() >= visibleRect.x() && exposeRect.x() <= visibleRect.maxX();
> 
> call this intersectsInX

Done.

>> Source/WebCore/rendering/RenderLayer.cpp:2664
>> +        LayoutUnit intersectWidth = intersection(visibleRect, exposeRectX).width();
> 
> Seems like you could compute intersectWidth without making a rect.

Done.

>> Source/WebCore/rendering/RenderLayer.cpp:-2674
>> -        } else if (intersectWidth > 0) {
> 
> Keep the braces because the comment makes it a multi-line clause.

Done.

>> Source/WebCore/rendering/RenderLayer.cpp:2696
>> +    intersects = exposeRect.maxY() >= visibleRect.y() && exposeRect.y() <= visibleRect.maxY();
> 
> bool intersectsInY =

Done.

>> Source/WebCore/rendering/RenderLayer.cpp:2700
>> +        LayoutRect exposeRectY(visibleRect.x(), exposeRect.y(), visibleRect.width(), exposeRect.height());
> 
> Ditto

Done.

>> Source/WebCore/rendering/RenderLayer.cpp:2710
>> +        } else if (intersectHeight > 0)
> 
> Ditto

Done.

[EWS]

Committed r293642 (250146@main): <https://commits.webkit.org/250146@main>

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

[Rob Buis]

Comment on attachment 458592 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:2655
>> +    bool intersectsInX = exposeRect.maxX() >= visibleRect.x() && exposeRect.x() <= visibleRect.maxX();
> 
> Does this handle the "touches but does not intersect" case correctly?

I believe so, {(0,0), (10,10)} and {(11,0),(20, 10)} touch but do not intersect, matched by the expression above.

[Darin Adler]

Comment on attachment 458592 [details]
Patch

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

>>> Source/WebCore/rendering/RenderLayer.cpp:2655
>>> +    bool intersectsInX = exposeRect.maxX() >= visibleRect.x() && exposeRect.x() <= visibleRect.maxX();
>> 
>> Does this handle the "touches but does not intersect" case correctly?
> 
> I believe so, {(0,0), (10,10)} and {(11,0),(20, 10)} touch but do not intersect, matched by the expression above.

So you’re saying that we want "touches but not intersect" to return true here. Not obvious to me, but glad you thought it through.

[Simon Fraser (smfr)]

*** Bug 238567 has been marked as a duplicate of this bug. ***

[Tim Nguyen (:ntim)]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/47376