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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 450298: Patch

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




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