Bug 235851 - Allow history swipe in scroller with overscroll-behavior
Summary: Allow history swipe in scroller with overscroll-behavior
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikos Mouchtaris
URL:
Keywords: InRadar
Depends on:
Blocks: 235847
  Show dependency treegraph
 
Reported: 2022-01-28 19:08 PST by Nikos Mouchtaris
Modified: 2022-05-06 19:14 PDT (History)
8 users (show)

See Also:


Attachments
wip (6.98 KB, patch)
2022-02-24 16:19 PST, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (12.82 KB, patch)
2022-03-11 19:19 PST, Nikos Mouchtaris
simon.fraser: review+
Details | Formatted Diff | Diff
Patch (12.84 KB, patch)
2022-03-17 13:50 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikos Mouchtaris 2022-01-28 19:08:02 PST
Test that scroller with overscroll-behavior-x doesn't allow swipe gestures to trigger navigation.
Comment 1 Radar WebKit Bug Importer 2022-02-04 19:08:14 PST
<rdar://problem/88518462>
Comment 2 Nikos Mouchtaris 2022-02-24 16:19:24 PST
Created attachment 453149 [details]
wip
Comment 3 Nikos Mouchtaris 2022-03-11 19:19:11 PST
Created attachment 454525 [details]
Patch
Comment 4 Simon Fraser (smfr) 2022-03-16 17:03:01 PDT
Comment on attachment 454525 [details]
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-overscroll-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().
Comment 5 Nikos Mouchtaris 2022-03-17 13:50:20 PDT
Created attachment 455022 [details]
Patch
Comment 6 EWS 2022-03-18 15:00:28 PDT
Committed r291497 (248609@main): <https://commits.webkit.org/248609@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455022 [details].