Bug 32029 - Mousewheel event delta has reversed sign on Linux Chrome
Summary: Mousewheel event delta has reversed sign on Linux Chrome
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-01 11:44 PST by Evan Stade
Modified: 2015-05-07 16:41 PDT (History)
7 users (show)

See Also:


Attachments
fix (1.47 KB, patch)
2009-12-01 11:48 PST, Evan Stade
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Evan Stade 2009-12-01 11:48:10 PST
Created attachment 44092 [details]
fix
Comment 2 WebKit Review Bot 2009-12-01 11:52:20 PST
style-queue ran check-webkit-style on attachment 44092 [details] without any errors.
Comment 3 Evan Martin 2009-12-01 12:31:44 PST
[I am not a reviewer]
This looks good to me, but perhaps it would benefit from a comment on the counterintuitive notion that WebKit expect +X to mean to the left.
Comment 4 Darin Fisher (:fishd, Google) 2009-12-01 23:21:28 PST
Comment on attachment 44092 [details]
fix

r=me, the windows version has this comment:

// Windows is <- -/+ ->, WebKit <- +/- ->.
Comment 5 Eric Seidel (no email) 2009-12-02 12:06:15 PST
Comment on attachment 44092 [details]
fix

I'm not sure why WebKit is reversed that way.

Probably due to:
    wkGetWheelEventDeltas(event, &m_deltaX, &m_deltaY, &continuous);
http://trac.webkit.org/browser/trunk/WebCore/platform/mac/WheelEventMac.mm#L46

I'll CC Hyatt, as he wrote that code.
Comment 6 WebKit Commit Bot 2009-12-02 15:22:02 PST
Comment on attachment 44092 [details]
fix

Clearing flags on attachment: 44092

Committed r51614: <http://trac.webkit.org/changeset/51614>
Comment 7 WebKit Commit Bot 2009-12-02 15:22:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Oliver Hunt 2009-12-02 15:42:53 PST
It is not sufficient to point to a website for a manual test. Manual tests should be in WebCore/manual-tests

So the website version of the test, should be rewritten to test exactly what this is fixing, and then be landed in the tree.

Further there is no reason that this should not be tested automatically.  The absence of existing automatic tests is not justification to not write an automated test, if necessary your patch should include additions to DRT to allow wheel event generation (i'm really surprised it doesn't already).  This is how DRT came to provide the functionality it does: the first time anyone encounters a untested area DRT is extended to make such testing possible.
Comment 9 Evan Stade 2009-12-02 15:59:16 PST
With respect to testing this automatically and DRT: DRT (or for chromium, Test Shell/EventSendingController) generates its own WebInputEvent objects, it does not start with a platform object (for gtk, GdkEvent*) and use this code to convert it, hence layout tests do not and can not cover this code without major structural changes. On the other hand, infrastructure to test event conversion code is largely in place in the Chromium tree in the form of the Automation/UIControls infrastructure, which is used in our automated test suite (interactive tests, browser tests, UI tests, etc.).

When I said "manual test", I simply meant "this is how I know the patch is correct", I didn't intend that this should be added to WebCore/manual-tests. In light of the fact that it can be automatically tested (within Chromium), I think it should not be a manual test.
Comment 10 Eric Seidel (no email) 2009-12-02 16:05:09 PST
I think the distinction being lost here is that this code appears only to affect the conversion between OS event objects and WebCore event objects.  It does not affect event objects which are synthesized wholly within WebCore, and thus untestable.

At some point we should either c++ unit test these sort of platform -> webcore event conversion routines or add layout tests for them.  Unfortunately we don't yet have a working c++ unit testing framework.  Bug 21010.
Comment 11 Evan Stade 2009-12-02 16:07:07 PST
yes, the c++ unit testing framework would be ideal
Comment 12 Martin Robinson 2015-05-07 16:41:30 PDT
This seems to be an old Chromium bug.