Bug 111001 – Disabling rubber-banding at page top/bottom should not be limited to initial scroll events

• Component: UI Events
• Status: ASSIGNED

[Conrad Shultz]

API introduced as part of the fix for bug 110006 allows rubber-banding to be selectively suppressed for initial scroll events but does not allow suppression of rubber-banding within a scroll session (i.e. if one scrolls to a page extreme and then continues scrolling beyond the boundary).

[Conrad Shultz]

Created attachment 190571 [details]
Patch

[Beth Dakin]

Comment on attachment 190571 [details]
Patch

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

I chatted with Sam about this, and ultimately he convinced me that it is just a little to weird to hardcode behavior in WebCore that honors PlatformWheelEventPhaseBegan for horizontal swipes but not for vertical swipes. Ultimately it is just hardcoding a little too much client-specific preference down in client-agnostic code. Instead, you should add new SPI to to WebKit2 that indicates which directions should ignore or honor the BeganPhase when deciding who will handle the wheel event. Perhaps something like WKPageShouldForwardScrollEventsAtTopWithPhase(WKScrollPhase), and then you will have to define a new enum for WKScrollPhase, and of course you'll have three other functions for bottom, left, and right. Then that info can be plumbed down to WebCore, and you can consult that state information in the you are editing in this patch.

Sorry to make your life a little harder here! But I think this will be the best.

> Source/WebCore/page/scrolling/ScrollingTree.cpp:327
> +bool ScrollingTree::wheelEventMayBeHandledExternally(const PlatformWheelEvent& wheelEvent)

What about shouldForwardWheelEvent().

[Conrad Shultz]

(In reply to comment #2)
> (From update of attachment 190571 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190571&action=review
> 
> I chatted with Sam about this, and ultimately he convinced me that it is just a little to weird to hardcode behavior in WebCore that honors PlatformWheelEventPhaseBegan for horizontal swipes but not for vertical swipes. Ultimately it is just hardcoding a little too much client-specific preference down in client-agnostic code. Instead, you should add new SPI to to WebKit2 that indicates which directions should ignore or honor the BeganPhase when deciding who will handle the wheel event. Perhaps something like WKPageShouldForwardScrollEventsAtTopWithPhase(WKScrollPhase), and then you will have to define a new enum for WKScrollPhase, and of course you'll have three other functions for bottom, left, and right. Then that info can be plumbed down to WebCore, and you can consult that state information in the you are editing in this patch.
> 
> Sorry to make your life a little harder here! But I think this will be the best.

I agree, but this runs into a problem. All the various enums representing phase (e.g. WebWheelEvent::Phase, PlatformWheelEventPhase, and the platform-specific NSEventPhase) have PhaseNone == 0. Unfortunately, perfectly valid events (e.g. legacy mouse wheels, momentum scroll events) may be of PhaseNone depending on the platform, which make an API based on phase as currently implemented rather problematic. (E.g. if a client says setShouldForwardScrollEventsAtTopWithPhase(PhaseNone), how does WebKit distinguish between "forward events of PhaseNone" and "forward no events.")

I chatted with Beth a little about this and we came up with three possible, all slightly suboptimal, solutions:

1) Define a new purpose-built enum that parallels the existing enums but begins with 1 instead of 0. This seems undesirable since it would greatly increase the likelihood of confusion on the part of developers and creates the (outside) possibility that the underlying real enum could expand to the point of overflowing this new enum.

2) Use the existing enum types in conjunction with a bool to accomplish a similar goal. This seems undesirable since it would greatly complicate the API and just feels hackish.

3) Remove the concept of event phase from the API entirely and focus instead on the logical conditions under which events should be forwarded. For example, I propose something like:

enum class WKWheelEventForwardingCondition {
    WKWheelEventForwardNever,
    WKWheelEventForwardUnlessNavigating,
    WKWheelEventForwardAlways
}

(The second option would impact only horizontal wheel events and would allow back/forward swiping but not rubber-banding.)

I'm inclined toward the third option as one that would provide the best balance between API simplicity and flexibility. Sam - thoughts? Other suggestions?

> 
> > Source/WebCore/page/scrolling/ScrollingTree.cpp:327
> > +bool ScrollingTree::wheelEventMayBeHandledExternally(const PlatformWheelEvent& wheelEvent)
> 
> What about shouldForwardWheelEvent().

How about shouldForwardWheelEventToClient(), to distinguish from, e.g., thunking to the main thread?