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.
Created attachment 148605 [details] Patch
Comment on attachment 148605 [details] Patch abarth@: Could you please review perhaps?
(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 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?
(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.
Ok. That makes sense. I still would be good to be able to test this. Perhaps we should write an API unit test?
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.
Created attachment 148927 [details] Patch
Comment on attachment 148927 [details] Patch Please take another look. Now with tests.
Comment on attachment 148927 [details] Patch Thanks for the test!
Comment on attachment 148927 [details] Patch Clearing flags on attachment: 148927 Committed r121025: <http://trac.webkit.org/changeset/121025>
All reviewed patches have been landed. Closing bug.
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.
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.
Reverted r121025 for reason: Caused crash in EventHandler.shouldTurnVerticalTicksIntoHorizontal webkit unit test on 10.7 Committed r121036: <http://trac.webkit.org/changeset/121036>
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.
Created attachment 149802 [details] Patch
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?
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 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.
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?
Yeah, a LayoutTest would be better.
Created attachment 150023 [details] Patch
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 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 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
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
Created attachment 150245 [details] Patch
Comment on attachment 150245 [details] Patch new in this version: DRT change to fix layout test breakage and nits fixed. Please take another look.
Created attachment 151746 [details] Patch
Comment on attachment 151746 [details] Patch Clearing flags on attachment: 151746 Committed r122399: <http://trac.webkit.org/changeset/122399>