WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Robert Kroeger
Comment 1
2012-06-20 10:58:17 PDT
Created
attachment 148605
[details]
Patch
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
Created
attachment 148927
[details]
Patch
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
Created
attachment 149802
[details]
Patch
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
Created
attachment 150023
[details]
Patch
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
Created
attachment 150245
[details]
Patch
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
Created
attachment 151746
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug