Bug 89580 - Suppress horizontal conversion of PlatformWheelEvents when hasPreciseScrollingDeltas is true
Summary: Suppress horizontal conversion of PlatformWheelEvents when hasPreciseScrollin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Kroeger
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-20 10:50 PDT by Robert Kroeger
Modified: 2012-07-11 18:29 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.94 KB, patch)
2012-06-20 10:58 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (13.57 KB, patch)
2012-06-21 18:00 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (13.96 KB, patch)
2012-06-27 14:59 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (15.44 KB, patch)
2012-06-28 15:09 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (562.31 KB, application/zip)
2012-06-28 19:29 PDT, WebKit Review Bot
no flags Details
Patch (15.10 KB, patch)
2012-06-29 13:25 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff
Patch (15.01 KB, patch)
2012-07-11 12:06 PDT, Robert Kroeger
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Kroeger 2012-06-20 10:50:40 PDT
WebKit GTK and Chromium Linux force vertical wheel events to scroll horizontally when over horizontal scroll bars. This is undesirable for events originating from the ChromeOS trackpad. ChromeOS Chromium generates wheel events for trackpad scrolling with hasPreciseScrollingDeltas() == true so suppress the horizontal conversion of PlatformWheelEvents for Chrome/Linux when hasPreciseScrollingDeltas() is true.
Comment 1 Robert Kroeger 2012-06-20 10:58:17 PDT
Created attachment 148605 [details]
Patch
Comment 2 Robert Kroeger 2012-06-20 11:00:06 PDT
Comment on attachment 148605 [details]
Patch

abarth@: Could you please review perhaps?
Comment 3 Michal Mocny 2012-06-20 11:03:28 PDT
(In reply to comment #0)
> WebKit GTK and Chromium Linux force vertical wheel events to scroll horizontally when over horizontal scroll bars.

Just re-tried this to confirm that it happens when mouse is anywhere over an element which is vertically scroll-able, not just when over the scroll bars specifically.  (Though it sounds like the fix you describe will addresses both cases)
Comment 4 Adam Barth 2012-06-20 12:48:56 PDT
Comment on attachment 148605 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Suppress horizontal conversion of PlatformWheelEvents when hasPreciseScrollingDeltas is true
> +        https://bugs.webkit.org/show_bug.cgi?id=89580

Can you explain more about what problem this patch solves?

Also, it is possible to write a test?
Comment 5 Adam Barth 2012-06-20 12:49:29 PDT
(In reply to comment #0)
> WebKit GTK and Chromium Linux force vertical wheel events to scroll horizontally when over horizontal scroll bars. This is undesirable for events originating from the ChromeOS trackpad. ChromeOS Chromium generates wheel events for trackpad scrolling with hasPreciseScrollingDeltas() == true so suppress the horizontal conversion of PlatformWheelEvents for Chrome/Linux when hasPreciseScrollingDeltas() is true.

Ah, I see.  This is good information to include in the ChangeLog.
Comment 6 Adam Barth 2012-06-20 12:50:18 PDT
Ok.  That makes sense.  I still would be good to be able to test this.  Perhaps we should write an API unit test?
Comment 7 Robert Kroeger 2012-06-21 15:43:44 PDT
Comment on attachment 148605 [details]
Patch

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

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=89580
> 
> Can you explain more about what problem this patch solves?
> 
> Also, it is possible to write a test?

p2 will/does feature an improved ChangeLog and a unit test.
Comment 8 Robert Kroeger 2012-06-21 18:00:46 PDT
Created attachment 148927 [details]
Patch
Comment 9 Robert Kroeger 2012-06-21 18:01:42 PDT
Comment on attachment 148927 [details]
Patch

Please take another look. Now with tests.
Comment 10 Adam Barth 2012-06-21 18:09:15 PDT
Comment on attachment 148927 [details]
Patch

Thanks for the test!
Comment 11 WebKit Review Bot 2012-06-22 08:19:04 PDT
Comment on attachment 148927 [details]
Patch

Clearing flags on attachment: 148927

Committed r121025: <http://trac.webkit.org/changeset/121025>
Comment 12 WebKit Review Bot 2012-06-22 08:19:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Darin Adler 2012-06-22 10:10:05 PDT
Comment on attachment 148927 [details]
Patch

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

> Source/WebCore/page/EventHandler.h:232
> +    friend class ::EventHandlerTest;

This is not correct style. We never put things into WebCore that reference classes outside the namespace like this. If we really need this special friend relationship with a test harness, then this should go into WebCore’s namespace or into another namespace like WebCoreTests.
Comment 14 Kenneth Russell 2012-06-22 10:39:29 PDT
This caused crashes in the EventHandler.shouldTurnVerticalTicksIntoHorizontal test in webkit_unit_tests on Mac OS X 10.7. The first failure on the bot:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.7/builds/7829

Since this is a crash and I don't know whether the test or the code is wrong I'm rolling this out.
Comment 15 Kenneth Russell 2012-06-22 10:42:40 PDT
Reverted r121025 for reason:

Caused crash in EventHandler.shouldTurnVerticalTicksIntoHorizontal webkit unit test on 10.7

Committed r121036: <http://trac.webkit.org/changeset/121036>
Comment 16 Adam Barth 2012-06-22 10:56:20 PDT
Comment on attachment 148927 [details]
Patch

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

>> Source/WebCore/page/EventHandler.h:232
>> +    friend class ::EventHandlerTest;
> 
> This is not correct style. We never put things into WebCore that reference classes outside the namespace like this. If we really need this special friend relationship with a test harness, then this should go into WebCore’s namespace or into another namespace like WebCoreTests.

Yes, sorry I missed that.  Thanks Darin.  It looks like the patch has already been rolled out.
Comment 17 Robert Kroeger 2012-06-27 14:59:20 PDT
Created attachment 149802 [details]
Patch
Comment 18 Robert Kroeger 2012-06-27 15:04:48 PDT
Comment on attachment 149802 [details]
Patch

abarth@: 

I put the friend in the namespace suggested by Darin's comment (and fixed the crash under lion.) Could you take another look please?
Comment 19 Adam Barth 2012-06-27 17:16:07 PDT
I have a slight concern that the test in this patch reaches pretty deep into WebKit to test a very detailed piece of code.  As we refactor this code in the future, this test is likely to no longer be applicable.  Our usual approach to this issue in WebKit is to test changes using more-or-less stable APIs, such as through the public API exposed by a port or via LayoutTests.  In comment #6, when I mentioned an API unit test, I was referring to the former of these approaches.
Comment 20 Adam Barth 2012-06-27 17:46:51 PDT
Comment on attachment 149802 [details]
Patch

I feel bad that I led you down the wrong path w.r.t. testing.  If it's difficult to test this change through the API, we might want to land this without the brittle test.  In any case, I leave it up to your judgement.  Thank you for your patience.
Comment 21 Robert Kroeger 2012-06-28 07:02:39 PDT
abarth@:

I was somewhat surprised that you suggested a unit test for this instead of a LayoutTest. Maybe I should have asked first and implemented later but after too much email in a day, I just wanted to code something. :-) 

I think it would be possible to build a LayoutTest for this change (with possibly minor DRT improvements.) That would probably be better?
Comment 22 Adam Barth 2012-06-28 07:32:09 PDT
Yeah, a LayoutTest would be better.
Comment 23 Robert Kroeger 2012-06-28 15:09:16 PDT
Created attachment 150023 [details]
Patch
Comment 24 Robert Kroeger 2012-06-28 15:11:12 PDT
Comment on attachment 150023 [details]
Patch

abarth@: Please take another look. Now with a LayoutTest. 

I'm open to suggestions on how to handle the fact that the user visible behaviour is intentionally different.
Comment 25 Adam Barth 2012-06-28 15:18:22 PDT
Comment on attachment 150023 [details]
Patch

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

This looks good, but you've got some crazy indenting with tabs going on.

> LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-sideways.html:26
> +    /* border-radius: 10px; */

Why are these commented out?

> LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-sideways.html:107
> +		secondWheelScroll();
> +		return;

Looks like some tabs snuck in.

> LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-sideways.html:111
> +		debug("without precise deltas, scrollLeft: " + movingdiv.scrollLeft + " scrollTop: " + movingdiv.scrollTop);

Here too
Comment 26 WebKit Review Bot 2012-06-28 19:29:20 PDT
Comment on attachment 150023 [details]
Patch

Attachment 150023 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13111237

New failing tests:
fast/repaint/overflow-scroll-in-overflow-scroll-scrolled.html
fast/dom/shadow/wheel-event-on-input-in-shadow-dom.html
fast/repaint/table-overflow-auto-in-overflow-auto-scrolled.html
fast/events/wheelevent-direction-inverted-from-device.html
fast/events/remove-child-onscroll.html
scrollbars/scroll-rtl-or-bt-layer.html
fast/repaint/overflow-auto-in-overflow-auto-scrolled.html
fast/dom/shadow/wheel-event-in-shadow-dom.html
fast/repaint/table-overflow-scroll-in-overflow-scroll-scrolled.html
Comment 27 WebKit Review Bot 2012-06-28 19:29:24 PDT
Created attachment 150067 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 28 Robert Kroeger 2012-06-29 13:25:09 PDT
Created attachment 150245 [details]
Patch
Comment 29 Robert Kroeger 2012-06-29 13:26:22 PDT
Comment on attachment 150245 [details]
Patch

new in this version:  DRT change to fix layout test breakage and nits fixed. Please take another look.
Comment 30 Robert Kroeger 2012-07-11 12:06:36 PDT
Created attachment 151746 [details]
Patch
Comment 31 WebKit Review Bot 2012-07-11 18:29:21 PDT
Comment on attachment 151746 [details]
Patch

Clearing flags on attachment: 151746

Committed r122399: <http://trac.webkit.org/changeset/122399>
Comment 32 WebKit Review Bot 2012-07-11 18:29:27 PDT
All reviewed patches have been landed.  Closing bug.