Bug 238985

Summary: REGRESSION(r290770): element.scrollIntoViewIfNeeded() scrolls to top even when element is already in viewport
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: ScrollingAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, clopez, darin, esprehn+autocc, ews-watchlist, fred.wang, glenn, jonlee, kondapallykalyan, pdr, rbuis, simon.fraser, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://github.com/web-platform-tests/wpt/pull/33573
https://bugs.webkit.org/show_bug.cgi?id=238567
Bug Depends on:    
Bug Blocks: 237747    
Attachments:
Description Flags
[HTML] Reduction
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Nikita Vasilyev 2022-04-07 23:26:34 PDT 
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.
Comment 1 Rob Buis 2022-04-08 03:53:20 PDT 
Created attachment 457038 [details]
Patch
Comment 2 Rob Buis 2022-04-08 06:07:33 PDT 
Created attachment 457060 [details]
Patch
Comment 3 Rob Buis 2022-04-08 08:36:32 PDT 
Created attachment 457074 [details]
Patch
Comment 4 EWS Watchlist 2022-04-08 08:38:44 PDT 
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
Comment 5 Rob Buis 2022-04-13 00:51:10 PDT 
Created attachment 457512 [details]
Patch
Comment 6 Radar WebKit Bug Importer 2022-04-14 23:27:13 PDT 
<rdar://problem/91797137>
Comment 7 Simon Fraser (smfr) 2022-04-28 10:26:01 PDT 
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
Comment 8 Simon Fraser (smfr) 2022-04-28 16:49:35 PDT 
<rdar://90985370>
Comment 9 Rob Buis 2022-04-29 01:52:27 PDT 
Created attachment 458577 [details]
Patch
Comment 10 Rob Buis 2022-04-29 08:49:20 PDT 
Created attachment 458590 [details]
Patch
Comment 11 Rob Buis 2022-04-29 10:22:48 PDT 
Created attachment 458592 [details]
Patch
Comment 12 Darin Adler 2022-04-29 13:14:31 PDT 
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?
Comment 13 Rob Buis 2022-04-29 22:25:32 PDT 
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.
Comment 14 EWS 2022-04-29 23:08:39 PDT 
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].
Comment 15 Rob Buis 2022-04-29 23:24:54 PDT 
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.
Comment 16 Darin Adler 2022-04-30 13:43:47 PDT 
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.
Comment 17 Simon Fraser (smfr) 2022-06-23 10:15:41 PDT 
*** Bug 238567 has been marked as a duplicate of this bug. ***