[webkit-reviews] review granted: [Bug 235851] Add test for swipe navigation with overscroll-behavior : [Attachment 454525] 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 235851: Add test for swipe navigation with overscroll-behavior
https://bugs.webkit.org/show_bug.cgi?id=235851

Attachment 454525: Patch

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




--- Comment #4 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 454525
  --> https://bugs.webkit.org/attachment.cgi?id=454525
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454525&action=review

> Source/WebCore/ChangeLog:3
> +	   Add test for swipe navigation with overscroll-behavior

The title should be about the code change, not about adding the test.

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:439
> +    ScrollPropagationInfo propagation;
> +    if (horizontalOverscrollBehaviorPreventsPropagation() ||
verticalOverscrollBehaviorPreventsPropagation()) {
> +	   if (horizontalOverscrollBehaviorPreventsPropagation() &&
!delta.height() && delta.width()) {
> +	       propagation.shouldBlockScrollPropagation = true;
> +	       propagation.isHandled = false;
> +	       return propagation;
> +	   }
> +
> +	   if ((horizontalOverscrollBehaviorPreventsPropagation() &&
verticalOverscrollBehaviorPreventsPropagation())
> +	       || (horizontalOverscrollBehaviorPreventsPropagation() &&
!delta.height())
> +	       || (verticalOverscrollBehaviorPreventsPropagation() &&
!delta.width())) {
> +	       propagation.shouldBlockScrollPropagation = true;
> +	       propagation.isHandled = true;
> +	   }
> +    }
> +    return propagation;

This is a little hard to follow. Maybe an early return if neither access
prevents propagation?

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:44
> +    bool shouldBlockScrollPropagation = false;
> +    bool isHandled = false;

Normally we use C++ initializer style: bool shouldBlockScrollPropagation {
false };

>
LayoutTests/scrollingcoordinator/mac/latching/horizontal-overflow-back-swipe-ov
erscroll-behavior.html:56
> +	       eventSender.monitorWheelEvents();
> +	       eventSender.mouseMoveTo(x, y);
> +	       eventSender.mouseScrollByWithWheelAndMomentumPhases(1, 0,
"began", "none");
> +	       eventSender.mouseScrollByWithWheelAndMomentumPhases(10, 0,
"changed", "none");
> +	       eventSender.mouseScrollByWithWheelAndMomentumPhases(10, 0,
"changed", "none");
> +	       await UIHelper.animationFrame();
> +	       eventSender.mouseScrollByWithWheelAndMomentumPhases(10, 0,
"changed", "none");
> +	       eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0,
"ended", "none");
> +	       return UIHelper.waitForScrollCompletion();

I prefer the await UIHelper.mouseWheelSequence(wheelEventSquence); technique;
see if that works without the intermediate UIHelper.animationFrame().