Bug 89580 – Suppress horizontal conversion of PlatformWheelEvents when hasPreciseScrollingDeltas is true

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Robert Kroeger]

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.

[Robert Kroeger]

Created attachment 148605 [details]
Patch

[Robert Kroeger]

Comment on attachment 148605 [details]
Patch

abarth@: Could you please review perhaps?

[Michal Mocny]

(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)

[Adam Barth]

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?

[Adam Barth]

(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.

[Adam Barth]

Ok.  That makes sense.  I still would be good to be able to test this.  Perhaps we should write an API unit test?

[Robert Kroeger]

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.

[Robert Kroeger]

Created attachment 148927 [details]
Patch

[Robert Kroeger]

Comment on attachment 148927 [details]
Patch

Please take another look. Now with tests.

[Adam Barth]

Comment on attachment 148927 [details]
Patch

Thanks for the test!

[WebKit Review Bot]

Comment on attachment 148927 [details]
Patch

Clearing flags on attachment: 148927

Committed r121025: <http://trac.webkit.org/changeset/121025>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Kenneth Russell]

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.

[Kenneth Russell]

Reverted r121025 for reason:

Caused crash in EventHandler.shouldTurnVerticalTicksIntoHorizontal webkit unit test on 10.7

Committed r121036: <http://trac.webkit.org/changeset/121036>

[Adam Barth]

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.

[Robert Kroeger]

Created attachment 149802 [details]
Patch

[Robert Kroeger]

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?

[Adam Barth]

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.

[Adam Barth]

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.

[Robert Kroeger]

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?

[Adam Barth]

Yeah, a LayoutTest would be better.

[Robert Kroeger]

Created attachment 150023 [details]
Patch

[Robert Kroeger]

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.

[Adam Barth]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Robert Kroeger]

Created attachment 150245 [details]
Patch

[Robert Kroeger]

Comment on attachment 150245 [details]
Patch

new in this version:  DRT change to fix layout test breakage and nits fixed. Please take another look.

[Robert Kroeger]

Created attachment 151746 [details]
Patch

[WebKit Review Bot]

Comment on attachment 151746 [details]
Patch

Clearing flags on attachment: 151746

Committed r122399: <http://trac.webkit.org/changeset/122399>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.