Bug 221407

Summary:

Root scroll snapping broken when overflow:scroll is set on the <body>

Product:

WebKit

Reporter:

Martin Robinson <mrobinson>

Component:

Scrolling

Assignee:

Martin Robinson <mrobinson>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

changseok, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

218115, 222179

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Martin Robinson

Reported 2021-02-04 08:25:15 PST

It seems that snapping children are using the body as their snap container in this case instead of propagating up to the root.

Attachments
Patch (15.62 KB, patch) 2021-02-04 09:44 PST, Martin Robinson	no flags	Details Formatted Diff Diff
Patch (20.83 KB, patch) 2021-02-10 09:47 PST, Martin Robinson	no flags	Details Formatted Diff Diff
Patch (17.84 KB, patch) 2021-02-18 01:05 PST, Martin Robinson	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Martin Robinson

Comment 1 2021-02-04 09:44:36 PST

Created attachment 419291 [details] Patch

Simon Fraser (smfr)

Comment 2 2021-02-08 09:10:47 PST

Comment on attachment 419291 [details] Patch How does this work when the body is independently scrollable (overflow:hidden on the <html>, overflow:scroll on the <body>)?

Martin Robinson

Comment 3 2021-02-10 01:26:25 PST

(In reply to Simon Fraser (smfr) from comment #2) > Comment on attachment 419291 [details] > Patch > > How does this work when the body is independently scrollable > (overflow:hidden on the <html>, overflow:scroll on the <body>)? My proposed change indeed breaks the case where the body is independently scrollable. After doing a bit of investigation, I think the bug here is more subtle. It looks like when we are finding the scrolling container for a given element, we are walking directly up the RenderTree. This means that WebKit will return the wrong container when absolute positioning results in a containing block that is an ancestor of the nearest scroll container. That seems to be what's happening in the WPT test. We should instead walk up the tree according to containing blocks.

Martin Robinson

Comment 4 2021-02-10 09:47:23 PST

Created attachment 419859 [details] Patch

Martin Robinson

Comment 5 2021-02-10 09:49:49 PST

I've uploaded a new version of this change which should fix the actual problem. When looking for the scrolling container for a snapping element we need to search the containing block ancestry. Overflow:scroll does not establish a new containing block, so it is possible for an overflow:scroll ancestor to have a snapping descendant that has a mutual ancestor as the real snap container. I have also added a test to verify that this does not break snapping in the case of an independently scrollable body. This test would have caught the error in my previous patch.

Simon Fraser (smfr)

Comment 6 2021-02-10 10:18:48 PST

(In reply to Martin Robinson from comment #3) > (In reply to Simon Fraser (smfr) from comment #2) > > Comment on attachment 419291 [details] > > Patch > > > > How does this work when the body is independently scrollable > > (overflow:hidden on the <html>, overflow:scroll on the <body>)? > > My proposed change indeed breaks the case where the body is independently > scrollable. > > After doing a bit of investigation, I think the bug here is more subtle. It > looks like when we are finding the scrolling container for a given element, > we are walking directly up the RenderTree. This means that WebKit will > return the wrong container when absolute positioning results in a containing > block that is an ancestor of the nearest scroll container. That seems to be > what's happening in the WPT test. We should instead walk up the tree > according to containing blocks. I've noted this ambiguity elsewhere. We mix all of DOM ancestor walks, containing block walks, and renderer ancestor tree walks. We might even have some z-order ancestor walks somewhere.

Simon Fraser (smfr)

Comment 7 2021-02-10 10:21:40 PST

See EventHandler::handleWheelEventInAppropriateEnclosingBox(), findEnclosingScrollableContainer(() etc.

Simon Fraser (smfr)

Comment 8 2021-02-10 10:22:55 PST

Comment on attachment 419859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419859&action=review > Source/WebCore/rendering/RenderObject.cpp:468 > + if (candidate->hasOverflowClip()) Should this use canBeScrolledAndHasScrollableArea()?

Martin Robinson

Comment 9 2021-02-10 11:06:12 PST

(In reply to Simon Fraser (smfr) from comment #8) > Comment on attachment 419859 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419859&action=review > > > Source/WebCore/rendering/RenderObject.cpp:468 > > + if (candidate->hasOverflowClip()) > > Should this use canBeScrolledAndHasScrollableArea()? Unfortunately, I don't think so, because `RenderBox::canBeScrolledAndHasScrollableArea` relies on `canBeProgramaticallyScrolled()` which returns false for overflow:hidden. The naming of these methods could use some love, I think. There's a FIXME over `canBeProgramaticallyScrolled`. // FIXME: This is badly named. overflow:hidden can be programmatically scrolled, yet this returns false in that case. Even ::hasOverflowClip could probably use a rename. It must date from before overflow:clip because the comment for it says this: ADD_BOOLEAN_BITFIELD(hasOverflowClip, HasOverflowClip); // Set in the case of overflow:auto/scroll/hidden I am thinking it might be good to rename that in a followup.

Radar WebKit Bug Importer

Comment 10 2021-02-11 08:26:13 PST

<rdar://problem/74235002>

Martin Robinson

Comment 11 2021-02-18 01:05:56 PST

Created attachment 420805 [details] Patch

EWS

Comment 12 2021-02-18 04:02:07 PST

Committed r273073: <https://commits.webkit.org/r273073> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420805 [details].

Darin Adler

Comment 13 2021-02-18 09:25:07 PST

Comment on attachment 420805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420805&action=review > Source/WebCore/rendering/RenderObject.cpp:463 > + auto* candidate = container(); > + while (candidate) { We have an excellent opportunity here for slightly greater loop clarity using a for loop: for (auto* candidate = container(); candidate; candidate = candidate->container()) { > Source/WebCore/rendering/RenderObject.cpp:469 > + return static_cast<RenderBox*>(candidate); This should be using downcast<RenderBox>, not static_cast<RenderBox*>.

Note You need to log in before you can comment on or make changes to this bug.