[webkit-reviews] review granted: [Bug 222968] Implement CSS overscroll-behavior for synchronous scroll : [Attachment 450319] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 222968: Implement CSS overscroll-behavior for synchronous scroll
https://bugs.webkit.org/show_bug.cgi?id=222968

Attachment 450319: Patch

https://bugs.webkit.org/attachment.cgi?id=450319&action=review




--- Comment #29 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 450319
  --> https://bugs.webkit.org/attachment.cgi?id=450319
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.