Bug 222968

Created attachment 422685 [details] Patch

Created attachment 423165 [details] Patch

Created attachment 423168 [details] Patch

Created attachment 423300 [details] Patch

<rdar://problem/75472790>

Created attachment 423579 [details] Patch

Created attachment 423886 [details] Patch

Comment on attachment 423886 [details] Patch I think overscroll-behahvior of sync scroll is ready to review:)

Created attachment 424158 [details] Patch

Created attachment 445794 [details] Patch

Created attachment 445896 [details] Patch

Created attachment 445897 [details] Patch

Created attachment 446466 [details] Patch

Comment on attachment 446466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446466&action=review > Source/WebCore/page/EventHandler.cpp:2922 > +static bool shouldBlockScrollChainingWithScrollableArea(const ScrollableArea* scrollableArea, FloatSize& biasedDelta) I don't like "scroll chaining". > Source/WebCore/page/EventHandler.cpp:2930 > + biasedDelta.setWidth(scrollableArea->horizontalOverscrollBehavior()!= OverscrollBehavior::Auto ? 0 : biasedDelta.width()); > + biasedDelta.setHeight(scrollableArea->verticalOverscrollBehavior()!= OverscrollBehavior::Auto ? 0 : biasedDelta.height()); Spacing here. > Source/WebCore/page/EventHandler.cpp:2943 > + if (frameView && (frameView->horizontalOverscrollBehavior() != OverscrollBehavior::Auto || frameView->verticalOverscrollBehavior() != OverscrollBehavior::Auto)) { > + if (frameView->horizontalOverscrollBehavior() != OverscrollBehavior::Auto && !biasedDelta.height() && biasedDelta.width()) > + return true; > + if ((frameView->horizontalOverscrollBehavior() != OverscrollBehavior::Auto && frameView->verticalOverscrollBehavior() != OverscrollBehavior::Auto) || (frameView->horizontalOverscrollBehavior()!= OverscrollBehavior::Auto && !biasedDelta.height()) || (frameView->verticalOverscrollBehavior()!= OverscrollBehavior::Auto && !biasedDelta.width())) > + return true; > + biasedDelta.setWidth(frameView->horizontalOverscrollBehavior()!= OverscrollBehavior::Auto ? 0 : biasedDelta.width()); > + biasedDelta.setHeight(frameView->verticalOverscrollBehavior()!= OverscrollBehavior::Auto ? 0 : biasedDelta.height()); There are so many "horizontalOverscrollBehavior() != OverscrollBehavior::Auto" checks in this code. Maybe add a helper function that makes this more readable. > Source/WebCore/page/EventHandler.cpp:3064 > + adjustedWheelEvent = adjustedWheelEvent.copyWithDeltaAndVelocity(filteredDelta, adjustedWheelEvent.scrollingVelocity()); > + handledEvent = processWheelEventForScrolling(adjustedWheelEvent, scrollableArea, handling); > + processWheelEventForScrollSnap(adjustedWheelEvent, scrollableArea); I find it odd that this main thread code implements overscroll behavior by zeroing out the deltas on the events, but the scrolling thread code does it by cutting of the ancestor node traversal. Let's be consistent.

Created attachment 446608 [details] Patch

Comment on attachment 446608 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446608&action=review > Source/WebCore/page/EventHandler.cpp:3127 > + if (boxScrollableArea->horizontalOverscrollBehavior() != OverscrollBehavior::Auto && !filteredPlatformDelta.height()) can this use hasHorizontalOverscrollBehavior? > Source/WebCore/page/mac/EventHandlerMac.mm:818 > + if ((scrollableArea->horizontalOverscrollBehavior() != OverscrollBehavior::Auto && scrollableArea->verticalOverscrollBehavior() != OverscrollBehavior::Auto) || (scrollableArea->horizontalOverscrollBehavior() != OverscrollBehavior::Auto && !wheelEvent.delta().height()) || (scrollableArea->verticalOverscrollBehavior() != OverscrollBehavior::Auto && !wheelEvent.delta().width())) can this use has{Horizontal,Vertical}OverscrollBehavior?

Comment on attachment 446608 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446608&action=review > Source/WebCore/page/EventHandler.cpp:3127 > + if (boxScrollableArea->horizontalOverscrollBehavior() != OverscrollBehavior::Auto && !filteredPlatformDelta.height()) can this use hasHorizontalOverscrollBehavior? > Source/WebCore/page/mac/EventHandlerMac.mm:818 > + if ((scrollableArea->horizontalOverscrollBehavior() != OverscrollBehavior::Auto && scrollableArea->verticalOverscrollBehavior() != OverscrollBehavior::Auto) || (scrollableArea->horizontalOverscrollBehavior() != OverscrollBehavior::Auto && !wheelEvent.delta().height()) || (scrollableArea->verticalOverscrollBehavior() != OverscrollBehavior::Auto && !wheelEvent.delta().width())) can this use has{Horizontal,Vertical}OverscrollBehavior?

Created attachment 446805 [details] Patch

(In reply to Simon Fraser (smfr) from comment #14) > Comment on attachment 446466 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=446466&action=review > > > Source/WebCore/page/EventHandler.cpp:3064 > > + adjustedWheelEvent = adjustedWheelEvent.copyWithDeltaAndVelocity(filteredDelta, adjustedWheelEvent.scrollingVelocity()); > > + handledEvent = processWheelEventForScrolling(adjustedWheelEvent, scrollableArea, handling); > > + processWheelEventForScrollSnap(adjustedWheelEvent, scrollableArea); > > I find it odd that this main thread code implements overscroll behavior by > zeroing out the deltas on the events, but the scrolling thread code does it > by cutting of the ancestor node traversal. Let's be consistent. The traversal is getting cut off in EventHandler::handleWheelEventInAppropriateEnclosingBox based on the result of shouldBlockScrollPropogationWithScrollableArea.

Comment on attachment 446805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446805&action=review > Source/WebCore/page/EventHandler.cpp:2922 > +static bool shouldBlockScrollPropogationWithScrollableArea(const ScrollableArea* scrollableArea, FloatSize& biasedDelta) "Propogation" Maybe this should be a function on ScrollableArea? And can we find out how to do this without having to modify event deltas?

Created attachment 446830 [details] Patch

(In reply to Simon Fraser (smfr) from comment #20) > Comment on attachment 446805 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=446805&action=review > > > Source/WebCore/page/EventHandler.cpp:2922 > > +static bool shouldBlockScrollPropogationWithScrollableArea(const ScrollableArea* scrollableArea, FloatSize& biasedDelta) > > "Propogation" > > Maybe this should be a function on ScrollableArea? And can we find out how > to do this without having to modify event deltas? I think we have to modify the deltas for the case where we are only blocking in one direction, we have to zero out the delta for that direction.

Comment on attachment 446830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446830&action=review > Source/WebCore/page/EventHandler.cpp:3029 > + PlatformWheelEvent adjustedWheelEvent = PlatformWheelEvent(event); auto > Source/WebCore/page/EventHandler.cpp:3057 > + FloatSize filteredDelta(wheelEvent.deltaX(), wheelEvent.deltaY()); auto filteredDelta = wheelEvent.delta() > Source/WebCore/page/EventHandler.cpp:3058 > + scrollableArea.shouldBlockScrollPropagation(filteredDelta); That looks like a no-op until you notice that shouldBlockScrollPropagation() modifies the delta. Can we apply the same cleanup we did in bug 220139? > Source/WebCore/page/EventHandler.cpp:3096 > + FloatSize biasedDelta(wheelEvent.deltaX(), wheelEvent.deltaY()); auto biasedDelta = .... > Source/WebCore/platform/ScrollableArea.cpp:811 > +bool ScrollableArea::shouldBlockScrollPropagation(FloatSize& biasedDelta) const This code should be changed to be more similar to the async structure (separate out the question from the modification). > LayoutTests/fast/scrolling/sync-scroll-overscroll-behavior-element.html:1 > +<!doctype html> Same comments about these tests as in bug 220139.

Created attachment 450298 [details] Patch

Comment on attachment 450298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450298&action=review I'd like to see one more round. Also, we need to test this code path. You need some tests that either don't have async overflow scroll enabled. > Source/WebCore/page/EventHandler.cpp:3103 > + if (boxScrollableArea->hasHorizontalOverscrollBehavior() && !filteredPlatformDelta.height()) Need ScrollableArea::horizontalOverscrollBehaviorPreventsPropagation() etc just like the async code path. Again here, the lack of symmetry between vertical and horizontal is confusing. > Source/WebCore/page/FrameView.h:706 > + bool hasHorizontalOverscrollBehavior() const final { return horizontalOverscrollBehavior() != OverscrollBehavior::Auto; } > + bool hasVerticalOverscrollBehavior() const final { return verticalOverscrollBehavior() != OverscrollBehavior::Auto; } Can't these live on ScrollableArea? > Source/WebCore/page/mac/EventHandlerMac.mm:819 > + if ((scrollableArea->hasHorizontalOverscrollBehavior() && scrollableArea->hasVerticalOverscrollBehavior()) || (scrollableArea->hasHorizontalOverscrollBehavior() && !wheelEvent.delta().height()) || (scrollableArea->hasVerticalOverscrollBehavior() && !wheelEvent.delta().width())) > + return nullptr; I feel like this could be wrapped up a helper called something like overscrollBehaviorPreventsScrollPropagation(FloatSize delta). Also it should use horizontalOverscrollBehaviorPreventsPropagation() terminology. > Source/WebCore/platform/ScrollableArea.cpp:821 > +bool ScrollableArea::shouldBlockScrollPropagation(const FloatSize& biasedDelta) const Oh it's here. Why didn't EventHandlerMac call this? > Source/WebCore/platform/ScrollableArea.h:375 > + virtual bool hasHorizontalOverscrollBehavior() const { return false; } > + virtual bool hasVerticalOverscrollBehavior() const { return false; } horizontalOverscrollBehaviorPreventsPropagation etc > Source/WebCore/rendering/RenderLayer.cpp:2312 > +// ASSERT_UNUSED(foundAncestor, foundAncestor); Why? Also you can't land this.

Created attachment 450309 [details] Patch

Created attachment 450311 [details] Patch

Created attachment 450319 [details] Patch

Comment on attachment 450319 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450319&action=review I'd still like the tests to be rewritten. > Source/WebCore/page/FrameView.h:706 > + bool horizontalOverscrollBehaviorPreventsPropagation() const final { return horizontalOverscrollBehavior() != OverscrollBehavior::Auto; } > + bool verticalOverscrollBehaviorPreventsPropagation() const final { return verticalOverscrollBehavior() != OverscrollBehavior::Auto; } Why aren't these just on the base class? horizontalOverscrollBehavior() is virtual, so this doesn't need to be. > Source/WebCore/page/mac/EventHandlerMac.mm:819 > + if (scrollableArea->shouldBlockScrollPropagation(wheelEvent.delta())) > + return nullptr; Is this the right place to call this, allowing a "over scroll-behavior: contain" scroller to rubberband? > Source/WebCore/platform/ScrollableArea.cpp:822 > + return ((horizontalOverscrollBehaviorPreventsPropagation() || verticalOverscrollBehaviorPreventsPropagation()) && ((horizontalOverscrollBehaviorPreventsPropagation() && verticalOverscrollBehaviorPreventsPropagation()) || (horizontalOverscrollBehaviorPreventsPropagation() && !biasedDelta.height()) || (verticalOverscrollBehaviorPreventsPropagation() && !biasedDelta.width()))); I'd wrap this line more. > Source/WebCore/platform/ScrollableArea.h:378 > + virtual bool horizontalOverscrollBehaviorPreventsPropagation() const { return false; } > + virtual bool verticalOverscrollBehaviorPreventsPropagation() const { return false; } Can't these just be: bool horizontalOverscrollBehaviorPreventsPropagation() const { return horizontalOverscrollBehavior() != OverscrollBehavior::Auto; } bool verticalOverscrollBehaviorPreventsPropagation() const { return verticalOverscrollBehavior() != OverscrollBehavior::Auto; } > Source/WebCore/rendering/RenderLayerScrollableArea.h:241 > + bool horizontalOverscrollBehaviorPreventsPropagation() const final { return horizontalOverscrollBehavior() != OverscrollBehavior::Auto; } > + bool verticalOverscrollBehaviorPreventsPropagation() const final { return verticalOverscrollBehavior() != OverscrollBehavior::Auto; } And you can remove these. > LayoutTests/platform/ios/TestExpectations:821 > +# Overscroll-behavior is not supported on iOS yet: https://bugs.webkit.org/show_bug.cgi?id=233788 > +fast/scrolling/sync-scroll-overscroll-behavior-element.html [ Skip ] > +fast/scrolling/sync-scroll-overscroll-behavior-iframe.html [ Skip ] > +fast/scrolling/sync-scroll-overscroll-behavior-unscrollable-element.html [ Skip ] > +fast/scrolling/sync-scroll-overscroll-behavior-unscrollable-iframe.html [ Skip ] Not true any more.

Created attachment 450690 [details] Patch

Created attachment 450692 [details] Patch

(In reply to Simon Fraser (smfr) from comment #29) > Comment on attachment 450319 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450319&action=review > > I'd still like the tests to be rewritten. Tracked in https://bugs.webkit.org/show_bug.cgi?id=235847. > > > Source/WebCore/page/FrameView.h:706 > > + bool horizontalOverscrollBehaviorPreventsPropagation() const final { return horizontalOverscrollBehavior() != OverscrollBehavior::Auto; } > > + bool verticalOverscrollBehaviorPreventsPropagation() const final { return verticalOverscrollBehavior() != OverscrollBehavior::Auto; } > > Why aren't these just on the base class? horizontalOverscrollBehavior() is > virtual, so this doesn't need to be. > Fixed. > > Source/WebCore/page/mac/EventHandlerMac.mm:819 > > + if (scrollableArea->shouldBlockScrollPropagation(wheelEvent.delta())) > > + return nullptr; > > Is this the right place to call this, allowing a "over scroll-behavior: > contain" scroller to rubberband? Added similar code to async in handleWheelEventInAppropriateEnclosingBox to allow node to handle rubber banding. > > > Source/WebCore/platform/ScrollableArea.cpp:822 > > + return ((horizontalOverscrollBehaviorPreventsPropagation() || verticalOverscrollBehaviorPreventsPropagation()) && ((horizontalOverscrollBehaviorPreventsPropagation() && verticalOverscrollBehaviorPreventsPropagation()) || (horizontalOverscrollBehaviorPreventsPropagation() && !biasedDelta.height()) || (verticalOverscrollBehaviorPreventsPropagation() && !biasedDelta.width()))); > > I'd wrap this line more. > Fixed. > > Source/WebCore/platform/ScrollableArea.h:378 > > + virtual bool horizontalOverscrollBehaviorPreventsPropagation() const { return false; } > > + virtual bool verticalOverscrollBehaviorPreventsPropagation() const { return false; } > > Can't these just be: > > bool horizontalOverscrollBehaviorPreventsPropagation() const { return > horizontalOverscrollBehavior() != OverscrollBehavior::Auto; } > bool verticalOverscrollBehaviorPreventsPropagation() const { return > verticalOverscrollBehavior() != OverscrollBehavior::Auto; } Fixed. > > > Source/WebCore/rendering/RenderLayerScrollableArea.h:241 > > + bool horizontalOverscrollBehaviorPreventsPropagation() const final { return horizontalOverscrollBehavior() != OverscrollBehavior::Auto; } > > + bool verticalOverscrollBehaviorPreventsPropagation() const final { return verticalOverscrollBehavior() != OverscrollBehavior::Auto; } > > And you can remove these. Fixed. > > > LayoutTests/platform/ios/TestExpectations:821 > > +# Overscroll-behavior is not supported on iOS yet: https://bugs.webkit.org/show_bug.cgi?id=233788 > > +fast/scrolling/sync-scroll-overscroll-behavior-element.html [ Skip ] > > +fast/scrolling/sync-scroll-overscroll-behavior-iframe.html [ Skip ] > > +fast/scrolling/sync-scroll-overscroll-behavior-unscrollable-element.html [ Skip ] > > +fast/scrolling/sync-scroll-overscroll-behavior-unscrollable-iframe.html [ Skip ] > > Not true any more. Fixed.

Comment on attachment 450692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450692&action=review > Source/WebCore/platform/ScrollableArea.h:379 > + bool overscrollBehaviorAllowRubberBand() const { return horizontalOverscrollBehavior() != OverscrollBehavior::None || verticalOverscrollBehavior() != OverscrollBehavior::None; } overscrollBehaviorAllowRubberBand -> overscrollBehaviorAllowsRubberBand

Created attachment 450737 [details] Patch

Committed r289074 (246780@main): <https://commits.webkit.org/246780@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450737 [details].