Skip to content

REGRESSION: CSS scroll-behavior: smooth with overflow: hidden breaks JS scrollTo/scrollLeft/scrollTop #1387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 0 commits into from
Jun 10, 2022
Merged

REGRESSION: CSS scroll-behavior: smooth with overflow: hidden breaks JS scrollTo/scrollLeft/scrollTop #1387

merged 0 commits into from
Jun 10, 2022

Conversation

nmoucht
Copy link
Contributor

@nmoucht nmoucht commented Jun 8, 2022 •
edited by webkit-early-warning-system
Loading

ed7fed1

REGRESSION: CSS scroll-behavior: smooth with overflow: hidden breaks JS scrollTo/scrollLeft/scrollTop
https://bugs.webkit.org/show_bug.cgi?id=238497
<rdar://90990040 >

Reviewed by Simon Fraser.

Add scrollable area to frame view's list of scrollable areas if necessary in scrollToOffset.
This is necessary as Document::runScrollSteps() looks through the list of scrollable areas
to service animations on the scrollable area. It is necessary to add the scrollable area
in scrollToOffset as for a scrollable area that is not user scrollable (such as a scrollable
area with overflow: hidden) it would not be added, so the animation would not occur.

* LayoutTests/fast/scrolling/smooth-scroll-with-overflow-hidden-expected.txt: Added.
* LayoutTests/fast/scrolling/smooth-scroll-with-overflow-hidden.html: Added.
* Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:
(WebCore::ScrollingCoordinator::absoluteEventTrackingRegionsForFrame const):
* Source/WebCore/platform/ScrollableArea.h:
(WebCore::ScrollableArea::isVisibleToHitTesting const):
* Source/WebCore/rendering/RenderLayerScrollableArea.cpp:
(WebCore::RenderLayerScrollableArea::clear):
(WebCore::RenderLayerScrollableArea::scrollToOffset):
(WebCore::RenderLayerScrollableArea::isVisibleToHitTesting const):
(WebCore::RenderLayerScrollableArea::updateScrollableAreaSet):
(WebCore::RenderLayerScrollableArea::registerScrollableArea):
* Source/WebCore/rendering/RenderLayerScrollableArea.h:

Canonical link: https://commits.webkit.org/251454@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295448 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@nmoucht nmoucht self-assigned this Jun 8, 2022
@nmoucht nmoucht added Safari 15 Scrolling Bugs related to main thread and off-main thread scrolling labels Jun 8, 2022
@nmoucht nmoucht requested a review from smfr June 8, 2022 20:05
smfr
smfr requested changes Jun 8, 2022
View reviewed changes
@nmoucht nmoucht force-pushed the eng/REGRESSION-CSS-scroll-behavior-smooth-with-overflow-hidden-breaks-JS-scrollToscrollLeftscrollTop branch from 5429687 to a7916ee Compare June 8, 2022 21:36
@nmoucht nmoucht requested a review from smfr June 8, 2022 21:38
smfr
smfr requested changes Jun 8, 2022
View reviewed changes
@nmoucht nmoucht force-pushed the eng/REGRESSION-CSS-scroll-behavior-smooth-with-overflow-hidden-breaks-JS-scrollToscrollLeftscrollTop branch from a7916ee to a5f952a Compare June 8, 2022 22:59
@nmoucht nmoucht requested a review from smfr June 8, 2022 23:00
@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Jun 9, 2022
smfr
smfr requested changes Jun 9, 2022
View reviewed changes
Copy link
Contributor

@smfr smfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered that RenderListBox also needs to override visibleToHitTesting. This is used for scrollable <select> lists which need to get into the non-fast scrollable area on macOS. There's a debug overlay to show this non-fast scrollable area in MiniBrowser so you can tell if this is working correctly.

@nmoucht nmoucht removed merging-blocked Applied to prevent a change from being merged Scrolling Bugs related to main thread and off-main thread scrolling Safari 15 labels Jun 9, 2022
@nmoucht nmoucht force-pushed the eng/REGRESSION-CSS-scroll-behavior-smooth-with-overflow-hidden-breaks-JS-scrollToscrollLeftscrollTop branch from a5f952a to 35ba4a4 Compare June 9, 2022 23:48
@nmoucht nmoucht added Safari 15 Scrolling Bugs related to main thread and off-main thread scrolling labels Jun 9, 2022
@nmoucht
Copy link
Contributor Author

nmoucht commented Jun 9, 2022

I remembered that RenderListBox also needs to override visibleToHitTesting. This is used for scrollable <select> lists which need to get into the non-fast scrollable area on macOS. There's a debug overlay to show this non-fast scrollable area in MiniBrowser so you can tell if this is working correctly.

I overrode RenderListBox to always be true. I'm not too familiar if RenderListBox, so I'm not sure if it can have style that makes this not the case. This does cause it to properly be added to the non-fast scrollable area.

@nmoucht nmoucht requested a review from smfr June 9, 2022 23:50
smfr
smfr requested changes Jun 10, 2022
View reviewed changes
@nmoucht nmoucht force-pushed the eng/REGRESSION-CSS-scroll-behavior-smooth-with-overflow-hidden-breaks-JS-scrollToscrollLeftscrollTop branch from 35ba4a4 to c250a2b Compare June 10, 2022 00:18
@nmoucht nmoucht requested a review from smfr June 10, 2022 00:20
@nmoucht nmoucht added the merge-queue Applied to send a pull request to merge-queue label Jun 10, 2022
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/REGRESSION-CSS-scroll-behavior-smooth-with-overflow-hidden-breaks-JS-scrollToscrollLeftscrollTop branch from c250a2b to ed7fed1 Compare June 10, 2022 08:05
@webkit-early-warning-system webkit-early-warning-system merged commit ed7fed1 into WebKit:main Jun 10, 2022
@webkit-early-warning-system
Copy link
Collaborator

Committed r295448 (251454@main): https://commits.webkit.org/251454@main

Reviewed commits have been landed. Closing PR #1387 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label Jun 10, 2022
@nmoucht nmoucht deleted the eng/REGRESSION-CSS-scroll-behavior-smooth-with-overflow-hidden-breaks-JS-scrollToscrollLeftscrollTop branch June 10, 2022 08:46
@jcfilben jcfilben mentioned this pull request Jun 13, 2022
Merged
4 tasks
@hellofanny hellofanny mentioned this pull request Jun 24, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smfr smfr smfr approved these changes

Assignees

@nmoucht nmoucht

Labels
Scrolling Bugs related to main thread and off-main thread scrolling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nmoucht @webkit-early-warning-system @smfr