Bug 81394 – [chromium] WebCompositorInputHandlerImpl ignores wheel event handlers for gesture scrolls that are really wheels

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[James Robinson]

There are several problems with how WebCompositorInputHandlerImpl deals with gesture events that are (in reality) wheel events:

1.) The GestureScrollBegin and GestureScrollEnd handlers calls CCInputHandler::scrollBegin with a scroll type of Gesture, which means that we don't test for wheel handlers.  This should use ScrollInputType Wheel
2.) We aren't testing for handlers at all in GestureScrollUpdate, even though we may be scrolling content that has a wheel handler "under" the mouse pointer.  This is valid for touch events, where the finger doesn't move relative to the content being scrolled, but it is *not* valid for wheel scrolling.  We need to hit test at every new offset.

[Nat Duca]

Does this block turning on the thread?

[James Robinson]

I think the short answer is yes, from what I understand.

[W. James MacLean]

Created attachment 132380 [details]
Patch

[W. James MacLean]

Comment on attachment 132380 [details]
Patch

Ooops, sorry - applied patch to wrong bug number ...

[WebKit Review Bot]

Attachment 132380 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/TouchFlingPlatformGestureCurve.cpp:66:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/TouchFlingPlatformGestureCurve.cpp:72:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/TouchFlingPlatformGestureCurve.cpp:74:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Simplest solution to (2) is to do what the MouseWheel branch in WCIHI does today and send the complete scrollBegin()/scrollBy()/scrollEnd() cycle on every GestureScrollUpdate.

[James Robinson]

Created attachment 132422 [details]
Patch

[W. James MacLean]

Lgtm ... I'm surprised a bit by the direction reversal, but whatever works!

[James Robinson]

Enne pointed out that this converts WCIHI over to ChromeOS's semantics for GestureScroll*, which are completely different from Chrome on Android's semantics for GestureScroll* despite the fact that they use the exact same event types.  So this patch will badly break chrome on android.

I could #ifdef the logic and have two copies - one for ChromeOS semantics and one for Android.

I still don't see what possible benefit there is for overloading GestureScroll* to mean wheel.

[James Robinson]

Comment on attachment 132422 [details]
Patch

Now that I've thought about it more this is just wrong and not worth breaking Android for.  Will need to iterate some more on how events are going to be handled in chromeos.

[James Robinson]

https://bugs.webkit.org/show_bug.cgi?id=81462 takes care of the behavior required for this patch without teh ugleh.