Bug 221407

Summary: Root scroll snapping broken when overflow:scroll is set on the <body>
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: ScrollingAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer
Priority: P2 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
DetailsFormatted DiffDiff
Patch (20.83 KB, patch)
2021-02-10 09:47 PST, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Patch (17.84 KB, patch)
2021-02-18 01:05 PST, Martin Robinson 		no flags
DetailsFormatted DiffDiff
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.