RESOLVED FIXED 89580
Suppress horizontal conversion of PlatformWheelEvents when hasPreciseScrollingDeltas is true
https://bugs.webkit.org/show_bug.cgi?id=89580
Summary Suppress horizontal conversion of PlatformWheelEvents when hasPreciseScrollin...
Robert Kroeger
Reported 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.
Attachments
Patch (4.94 KB, patch)
2012-06-20 10:58 PDT, Robert Kroeger
no flags
Patch (13.57 KB, patch)
2012-06-21 18:00 PDT, Robert Kroeger
no flags
Patch (13.96 KB, patch)
2012-06-27 14:59 PDT, Robert Kroeger
no flags
Patch (15.44 KB, patch)
2012-06-28 15:09 PDT, Robert Kroeger
no flags
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
Patch (15.10 KB, patch)
2012-06-29 13:25 PDT, Robert Kroeger
no flags
Patch (15.01 KB, patch)
2012-07-11 12:06 PDT, Robert Kroeger
no flags
Robert Kroeger
Comment 1 2012-06-20 10:58:17 PDT
Robert Kroeger
Comment 2 2012-06-20 11:00:06 PDT
Comment on attachment 148605 [details] Patch abarth@: Could you please review perhaps?
Michal Mocny
Comment 3 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)
Adam Barth
Comment 4 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?
Adam Barth
Comment 5 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.
Adam Barth
Comment 6 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?
Robert Kroeger
Comment 7 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.
Robert Kroeger
Comment 8 2012-06-21 18:00:46 PDT
Robert Kroeger
Comment 9 2012-06-21 18:01:42 PDT
Comment on attachment 148927 [details] Patch Please take another look. Now with tests.
Adam Barth
Comment 10 2012-06-21 18:09:15 PDT
Comment on attachment 148927 [details] Patch Thanks for the test!
WebKit Review Bot
Comment 11 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>
WebKit Review Bot
Comment 12 2012-06-22 08:19:10 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 13 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.
Kenneth Russell
Comment 14 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.
Kenneth Russell
Comment 15 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>
Adam Barth
Comment 16 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.
Robert Kroeger
Comment 17 2012-06-27 14:59:20 PDT
Robert Kroeger
Comment 18 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?
Adam Barth
Comment 19 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.
Adam Barth
Comment 20 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.
Robert Kroeger
Comment 21 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?
Adam Barth
Comment 22 2012-06-28 07:32:09 PDT
Yeah, a LayoutTest would be better.
Robert Kroeger
Comment 23 2012-06-28 15:09:16 PDT
Robert Kroeger
Comment 24 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.
Adam Barth
Comment 25 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
WebKit Review Bot
Comment 26 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
WebKit Review Bot
Comment 27 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
Robert Kroeger
Comment 28 2012-06-29 13:25:09 PDT
Robert Kroeger
Comment 29 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.
Robert Kroeger
Comment 30 2012-07-11 12:06:36 PDT
WebKit Review Bot
Comment 31 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>
WebKit Review Bot
Comment 32 2012-07-11 18:29:27 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.