Bug 238985 - REGRESSION(r290770): element.scrollIntoViewIfNeeded() scrolls to top even when element is already in viewport
Summary: REGRESSION(r290770): element.scrollIntoViewIfNeeded() scrolls to top even whe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
: 238567 (view as bug list)
Depends on:
Blocks: 237747
  Show dependency treegraph
 
Reported: 2022-04-07 23:26 PDT by Nikita Vasilyev
Modified: 2022-06-23 10:15 PDT (History)
14 users (show)

See Also:


Attachments
[HTML] Reduction (1.16 KB, text/html)
2022-04-07 23:26 PDT, Nikita Vasilyev
no flags Details
Patch (4.56 KB, patch)
2022-04-08 03:53 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (5.91 KB, patch)
2022-04-08 06:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.11 KB, patch)
2022-04-08 08:36 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.09 KB, patch)
2022-04-13 00:51 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.72 KB, patch)
2022-04-29 01:52 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.48 KB, patch)
2022-04-29 08:49 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.61 KB, patch)
2022-04-29 10:22 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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. ***