WheelEvent's deltaX/deltaY should report real amount of pixels scrolled

WheelEvent's deltaX/deltaY should report real amount of pixels scrolled

As per the specification, the DOM3 WheelEvent's deltaX/deltaY should report
the real amount of pixels scrolled. This is also the way IE and Firefox
behave.

However, Blink was reporting the number of mouse wheel ticks multiplied
by a multiplier (120) which does not necessarily match the real amount
of pixels that is scrolled.

R=arv
BUG=227454
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156603

[arv (Not doing code reviews), 2013-08-22 14:15:48]

https://codereview.chromium.org/23034006/diff/7001/LayoutTests/fast/events/wh...
File LayoutTests/fast/events/wheelevent-basic.html (right):

https://codereview.chromium.org/23034006/diff/7001/LayoutTests/fast/events/wh...
LayoutTests/fast/events/wheelevent-basic.html:57: <div id="target"
style="border:solid 1px green; width:220px; height:70px; overflow-x:scroll;
overflow-y:scroll; white-space: nowrap;">
"overflow: scroll" is shorthand for "overflow-x: scroll; overflow: y" so I don't
understand why that part was changed.

https://codereview.chromium.org/23034006/diff/7001/Source/core/dom/WheelEvent...
File Source/core/dom/WheelEvent.cpp (right):

https://codereview.chromium.org/23034006/diff/7001/Source/core/dom/WheelEvent...
Source/core/dom/WheelEvent.cpp:55: , m_wheelDelta(initializer.wheelDeltaX ?
initializer.wheelDeltaX : -initializer.deltaX, initializer.wheelDeltaY ?
initializer.wheelDeltaY : -initializer.deltaY)
Is this correct? What if initializer.wheelDeltaX was initialized to 0?

[do-not-use, 2013-08-22 14:46:03]

https://codereview.chromium.org/23034006/diff/7001/LayoutTests/fast/events/wh...
File LayoutTests/fast/events/wheelevent-basic.html (right):

https://codereview.chromium.org/23034006/diff/7001/LayoutTests/fast/events/wh...
LayoutTests/fast/events/wheelevent-basic.html:57: <div id="target"
style="border:solid 1px green; width:220px; height:70px; overflow-x:scroll;
overflow-y:scroll; white-space: nowrap;">
On 2013/08/22 14:15:49, arv wrote:
> "overflow: scroll" is shorthand for "overflow-x: scroll; overflow: y" so I
don't
> understand why that part was changed.

Oh, I did not know that. I will fix it.

https://codereview.chromium.org/23034006/diff/7001/Source/core/dom/WheelEvent...
File Source/core/dom/WheelEvent.cpp (right):

https://codereview.chromium.org/23034006/diff/7001/Source/core/dom/WheelEvent...
Source/core/dom/WheelEvent.cpp:55: , m_wheelDelta(initializer.wheelDeltaX ?
initializer.wheelDeltaX : -initializer.deltaX, initializer.wheelDeltaY ?
initializer.wheelDeltaY : -initializer.deltaY)
On 2013/08/22 14:15:49, arv wrote:
> Is this correct? What if initializer.wheelDeltaX was initialized to 0?

I am not sure I understand the question. If initializer.wheelDeltaX was
initialized to zero, then we will attempt to deduce its value from
initializer.deltaX if its was provided.

We need such fallback so that wheelEvent.wheelDeltaX() will return something
even though the event was initialized with the new deltaX.

[do-not-use, 2013-08-22 15:08:21]

https://codereview.chromium.org/23034006/diff/7001/LayoutTests/fast/events/wh...
File LayoutTests/fast/events/wheelevent-basic.html (right):

https://codereview.chromium.org/23034006/diff/7001/LayoutTests/fast/events/wh...
LayoutTests/fast/events/wheelevent-basic.html:57: <div id="target"
style="border:solid 1px green; width:220px; height:70px; overflow-x:scroll;
overflow-y:scroll; white-space: nowrap;">
On 2013/08/22 14:15:49, arv wrote:
> "overflow: scroll" is shorthand for "overflow-x: scroll; overflow: y" so I
don't
> understand why that part was changed.

Done.

https://codereview.chromium.org/23034006/diff/7001/Source/core/dom/WheelEvent...
File Source/core/dom/WheelEvent.cpp (right):

https://codereview.chromium.org/23034006/diff/7001/Source/core/dom/WheelEvent...
Source/core/dom/WheelEvent.cpp:55: , m_wheelDelta(initializer.wheelDeltaX ?
initializer.wheelDeltaX : -initializer.deltaX, initializer.wheelDeltaY ?
initializer.wheelDeltaY : -initializer.deltaY)
On 2013/08/22 14:15:49, arv wrote:
> Is this correct? What if initializer.wheelDeltaX was initialized to 0?

If the caller constructs the WheelEvent from JS and provides deltaX/deltaY but
not wheelDeltaX/wheelDeltaY then we are missing information about the number of
wheel ticks (since wheelDeltaX == ticksX * TickMultiplier).

To make sure that wheelDeltaX/wheelDeltaY do not return 0 in that case, we make
the assumption that ticksX * TickMultiplier is close to -deltaX. This is not
perfect but I don't see what else we could do here.

[arv (Not doing code reviews), 2013-08-22 15:25:15]

https://codereview.chromium.org/23034006/diff/7001/Source/core/dom/WheelEvent...
File Source/core/dom/WheelEvent.cpp (right):

https://codereview.chromium.org/23034006/diff/7001/Source/core/dom/WheelEvent...
Source/core/dom/WheelEvent.cpp:55: , m_wheelDelta(initializer.wheelDeltaX ?
initializer.wheelDeltaX : -initializer.deltaX, initializer.wheelDeltaY ?
initializer.wheelDeltaY : -initializer.deltaY)
On 2013/08/22 15:08:21, Christophe Dumez wrote:
> On 2013/08/22 14:15:49, arv wrote:
> > Is this correct? What if initializer.wheelDeltaX was initialized to 0?
> 
> If the caller constructs the WheelEvent from JS and provides deltaX/deltaY but
> not wheelDeltaX/wheelDeltaY then we are missing information about the number
of
> wheel ticks (since wheelDeltaX == ticksX * TickMultiplier).
> 
> To make sure that wheelDeltaX/wheelDeltaY do not return 0 in that case, we
make
> the assumption that ticksX * TickMultiplier is close to -deltaX. This is not
> perfect but I don't see what else we could do here.

My point was that if wheelDeltaX was initialized to 0 we treat that as false
(not present) and fall back to deltaX. There needs to be a way of knowing if it
was set to 0 or whether it is missing.

[do-not-use, 2013-08-22 15:35:05]

On 2013/08/22 15:25:15, arv wrote:
>
https://codereview.chromium.org/23034006/diff/7001/Source/core/dom/WheelEvent...
> File Source/core/dom/WheelEvent.cpp (right):
> 
>
https://codereview.chromium.org/23034006/diff/7001/Source/core/dom/WheelEvent...
> Source/core/dom/WheelEvent.cpp:55: , m_wheelDelta(initializer.wheelDeltaX ?
> initializer.wheelDeltaX : -initializer.deltaX, initializer.wheelDeltaY ?
> initializer.wheelDeltaY : -initializer.deltaY)
> On 2013/08/22 15:08:21, Christophe Dumez wrote:
> > On 2013/08/22 14:15:49, arv wrote:
> > > Is this correct? What if initializer.wheelDeltaX was initialized to 0?
> > 
> > If the caller constructs the WheelEvent from JS and provides deltaX/deltaY
but
> > not wheelDeltaX/wheelDeltaY then we are missing information about the number
> of
> > wheel ticks (since wheelDeltaX == ticksX * TickMultiplier).
> > 
> > To make sure that wheelDeltaX/wheelDeltaY do not return 0 in that case, we
> make
> > the assumption that ticksX * TickMultiplier is close to -deltaX. This is not
> > perfect but I don't see what else we could do here.
> 
> My point was that if wheelDeltaX was initialized to 0 we treat that as false
> (not present) and fall back to deltaX. There needs to be a way of knowing if
it
> was set to 0 or whether it is missing.

I see your point now. However, I don't see how we can distinguish. Also, this is
not a regular use case. People will usually either use the old API or the new
one. Why would someone initialize both wheelDeltaX and deltaX? In the normal
case where there user initializes wheelDeltaX to 0 explicitly, then deltaX is
extremely likely to be 0 as well so the fallback value would be 0.

[arv (Not doing code reviews), 2013-08-22 16:47:57]

LGTM

[commit-bot: I haz the power, 2013-08-22 17:07:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/23034006/14007

[commit-bot: I haz the power, 2013-08-22 17:18:22]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[do-not-use, 2013-08-22 18:41:13]

Still missing LGTM for the Source/web folder.

[jochen (gone - plz use gerrit), 2013-08-22 21:46:33]

lgtm

[commit-bot: I haz the power, 2013-08-22 21:48:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/23034006/14007

[commit-bot: I haz the power, 2013-08-23 00:45:22]

Change committed as 156603