It seems that snapping children are using the body as their snap container in this case instead of propagating up to the root.
Created attachment 419291 [details] Patch
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>)?
(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.
Created attachment 419859 [details] Patch
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.
(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.
See EventHandler::handleWheelEventInAppropriateEnclosingBox(), findEnclosingScrollableContainer(() etc.
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()?
(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.
<rdar://problem/74235002>
Created attachment 420805 [details] Patch
Committed r273073: <https://commits.webkit.org/r273073> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420805 [details].
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*>.