Implement CSS overscroll-behavior for synchronous scroll

[cathiechen]

Support CSS property overscroll-behavior for synchronous scroll.

[cathiechen]

Created attachment 422685 [details]
Patch

[cathiechen]

Created attachment 423165 [details]
Patch

[cathiechen]

Created attachment 423168 [details]
Patch

[cathiechen]

Created attachment 423300 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75472790>

[cathiechen]

Created attachment 423579 [details]
Patch

[cathiechen]

Created attachment 423886 [details]
Patch

[cathiechen]

Comment on attachment 423886 [details]
Patch

I think overscroll-behahvior of sync scroll is ready to review:)

[cathiechen]

Created attachment 424158 [details]
Patch

[Nikos Mouchtaris]

Created attachment 445794 [details]
Patch

[Nikos Mouchtaris]

Created attachment 445896 [details]
Patch

[Nikos Mouchtaris]

Created attachment 445897 [details]
Patch

[Nikos Mouchtaris]

Created attachment 446466 [details]
Patch

[Simon Fraser (smfr)]

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.

[Nikos Mouchtaris]

Created attachment 446608 [details]
Patch

[Jon Lee]

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?

[Jon Lee]

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?

[Nikos Mouchtaris]

Created attachment 446805 [details]
Patch

[Nikos Mouchtaris]

(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.

[Simon Fraser (smfr)]

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?

[Nikos Mouchtaris]

Created attachment 446830 [details]
Patch

[Nikos Mouchtaris]

(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.

[Simon Fraser (smfr)]

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.

[Nikos Mouchtaris]

Created attachment 450298 [details]
Patch

[Simon Fraser (smfr)]

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.

[Nikos Mouchtaris]

Created attachment 450309 [details]
Patch

[Nikos Mouchtaris]

Created attachment 450311 [details]
Patch

[Nikos Mouchtaris]

Created attachment 450319 [details]
Patch

[Simon Fraser (smfr)]

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.

[Nikos Mouchtaris]

Created attachment 450690 [details]
Patch

[Nikos Mouchtaris]

Created attachment 450692 [details]
Patch

[Nikos Mouchtaris]

(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.

[Simon Fraser (smfr)]

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

[Nikos Mouchtaris]

Created attachment 450737 [details]
Patch

[EWS]

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].