Bug 210476

<rdar://problem/61755103>

Created attachment 413140 [details] Patch

Comment on attachment 413140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413140&action=review > Source/WebCore/page/FrameView.cpp:923 > + LayoutRect viewportRect = bodyRenderer->paddingBoxRect(); It this right? The body might have a height of zero (if its children are position:aboslute). I'm not sure why this code doesn't just get the viewport rect from the FrameView. > Source/WebCore/page/FrameView.cpp:930 > + updateSnapOffsetsForScrollableArea(*this, *bodyRenderer, bodyRenderer->style(), viewportRect); I think it's wrong to pass the body here. The body is not the scrollable renderer. Maybe updateSnapOffsetsForScrollableArea() needs to take a ScrollableArea instead?

(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 413140 [details] > Patch Thanks for the review. I'm hoping to investigate the failures a bit tomorrow. My current theory is that it's a timing issue. > View in context: > https://bugs.webkit.org/attachment.cgi?id=413140&action=review > > > Source/WebCore/page/FrameView.cpp:923 > > + LayoutRect viewportRect = bodyRenderer->paddingBoxRect(); > > It this right? The body might have a height of zero (if its children are > position:aboslute). I'm not sure why this code doesn't just get the viewport > rect from the FrameView. I ended up doing this, because it wasn't clear how to propertly get the viewport rectangle from the FrameView. If there's a way to do that, that would definitely be a better approach. I suppose I still need to adjust this rectangle to make it relative to the padding box though? > > Source/WebCore/page/FrameView.cpp:930 > > + updateSnapOffsetsForScrollableArea(*this, *bodyRenderer, bodyRenderer->style(), viewportRect); > > I think it's wrong to pass the body here. The body is not the scrollable > renderer. Maybe updateSnapOffsetsForScrollableArea() needs to take a > ScrollableArea instead? This is a bit ugly, but essentially what is going on here is that this argument is mainly used to compare against child->enclosingScrollableContainerForSnapping(). The return value of that method is based entirely on what element has the scroll-snap-type property set on it. Currently, this is only settable on the body. My plan is to do something a bit nicer here when I fix https://bugs.webkit.org/show_bug.cgi?id=200643 (which I plan to do next). For instance, I could modify the code so that when setting the style on the body, it always looks like these properties are set on the root element for the scroll snapping code. My thought here was that all these fixes are more in the realm of issue 200643.

I meant to write issue https://bugs.webkit.org/show_bug.cgi?id=210469 above.

I think the main source of confusion is that the scroll snap code was written using the body to represent the document scroller, but the body can be scrollable independently from the document so there's ambiguity. Maybe fixing bug 210469 first would force cleanup here.

Created attachment 413584 [details] Patch

Comment on attachment 413584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413584&action=review > Source/WebCore/page/FrameView.cpp:949 > + viewport.move(-rootRenderer->marginRight(), -rootRenderer->marginTop()); Is marginRight() correct?

(In reply to Simon Fraser (smfr) from comment #8) > Comment on attachment 413584 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413584&action=review > > > Source/WebCore/page/FrameView.cpp:949 > > + viewport.move(-rootRenderer->marginRight(), -rootRenderer->marginTop()); > > Is marginRight() correct? Oof. That's definitely not correct, I will fix this and modify the tests so that they would have caught this.

Created attachment 413602 [details] Patch

Comment on attachment 413602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413602&action=review > LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal-with-margin.html:112 > + message.innerHTML = "<p>This test is better run under DumpRenderTree. To manually test it, place the mouse pointer<br/>" > + + "inside the red region at the top of the page, and then use the mouse wheel or a two-finger swipe to make a<br/>" > + + "small swipe gesture with some momentum.<br/><br/>" > + + "The region should scroll to show a green region.<br/><br/>" > + + "Next, perform a small scroll gesture that does not involve momentum. You should begin to see one of the colors<br/>" > + + "to the left (or right) of the current green box. When you release the wheel, the region should scroll back so<br/>" > + + "that the region is a single color.<br/><br/>" > + + "You should also be able to repeat these test steps for the vertical region below.</p>"; Personally I hate these long descriptions as they are often wrong.

(In reply to Simon Fraser (smfr) from comment #11) > Comment on attachment 413602 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413602&action=review > > > LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal-with-margin.html:112 > > + message.innerHTML = "<p>This test is better run under DumpRenderTree. To manually test it, place the mouse pointer<br/>" > > + + "inside the red region at the top of the page, and then use the mouse wheel or a two-finger swipe to make a<br/>" > > + + "small swipe gesture with some momentum.<br/><br/>" > > + + "The region should scroll to show a green region.<br/><br/>" > > + + "Next, perform a small scroll gesture that does not involve momentum. You should begin to see one of the colors<br/>" > > + + "to the left (or right) of the current green box. When you release the wheel, the region should scroll back so<br/>" > > + + "that the region is a single color.<br/><br/>" > > + + "You should also be able to repeat these test steps for the vertical region below.</p>"; > > Personally I hate these long descriptions as they are often wrong. I agree. I copied this description from the existing tests, but I will update them to be shorter and see if I can write a patch to improve these tests in particular.

Created attachment 413676 [details] Patch

Committed r269622: <https://trac.webkit.org/changeset/269622> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413676 [details].