741968 - Support mouse wheel scrolling in WinRT widget

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect)

Description

[Brian R. Bondy [:bbondy]]

The mouse wheel doesn't currently do anything in Metro Fennec, it should scroll the page.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, we're going to remove nsMouseScrollEvent from widget. See bug 719320.

Comment 3

[Jim Mathies [:jimm]]

Doing an mc merge today which brought in the new w3c wheel events. I'm pretty sure this totally broke. :/ Will try to test this on the tablet when I get a chance this weekend.

Comment 4

[Jim Mathies [:jimm]]

Tim, we'd really like to get this fix in asap if possible for the preview, can you make this a priority for today?

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch

This patch makes "horizontal" scrolling be in the X direction and "vertical" in the Y.  Also it uses `InitMouseEvent` to initialize the wheel event.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 668219 [details] [diff] [review]
Patch

This works fine, lets get it landed.

The scrolling seems kinda slow, I wonder if that's a pref we can tweak, or something in the values winrt is sending us, or maybe it's just my system. We can file a follow up on that though if it's wide spread.

Comment 7

[Brian R. Bondy [:bbondy]]

sweet!

Comment 8

[Brian R. Bondy [:bbondy]]

Hey Tim I think maybe we need to set the mouse button like this:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=741968&attachment=611893

Currently it's always being set to 0

Comment 9

[Brian R. Bondy [:bbondy]]

Maybe it doesn't matter for scrolling

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Brian R. Bondy [:bbondy] from comment #8)
> Hey Tim I think maybe we need to set the mouse button like this:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=741968&attachment=611893
> 
> Currently it's always being set to 0

I believe the `InitMouseEvent` call should handle that.

https://hg.mozilla.org/projects/elm/file/9a65b4e8099d/widget/windows/winrt/MetroWidget.cpp#l910

Comment 11

[Brian R. Bondy [:bbondy]]

Yup good call, glad this will be fixed in tomorrow's nightly for preview users :D  This is their biggest complaint.

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

https://hg.mozilla.org/projects/elm/rev/80b92e2a5bc0

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like the patch doesn't set lineOrPageDeltaX and lineOrPageDeltaY. They are necessary for dispatching DOMMouseScrollEvent.

And why the refPoint is always 0.0? the cursor position is important for wheel event to decide the event target.

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> Looks like the patch doesn't set lineOrPageDeltaX and lineOrPageDeltaY. They
> are necessary for dispatching DOMMouseScrollEvent.
> 
> And why the refPoint is always 0.0? the cursor position is important for
> wheel event to decide the event target.

I'll write a follow-up patch that addresses these issues.  Thanks!

Comment 15

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Followup patch

I'm currently building this patch to test it.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> Looks like the patch doesn't set lineOrPageDeltaX and lineOrPageDeltaY. They
> are necessary for dispatching DOMMouseScrollEvent.

From the other uses of `lineOrPageDeltaX` and `lineOrPageDeltaY` that I've found, it seems that they should just be set to the same value as `deltaX` and `deltaY`.  Is this correct?

> And why the refPoint is always 0.0? the cursor position is important for
> wheel event to decide the event target.

I've removed the lines that set `refPoint` back to (0,0).  The values set in `InitMousePoint` are now the ones we send in the `WheelEvent`.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 668258 [details] [diff] [review]
Followup patch

Hmm, I think that current code doesn't support high-resolution scrolling, right?

>diff --git a/widget/windows/winrt/MetroWidget.cpp b/widget/windows/winrt/MetroWidget.cpp
>--- a/widget/windows/winrt/MetroWidget.cpp
>+++ b/widget/windows/winrt/MetroWidget.cpp
>@@ -975,32 +975,32 @@ MetroWidget::InitMouseEvent(PointerEvent

>   static int previousVertLeftOverDelta = 0;
>   static int previousHorLeftOverDelta = 0;
>   if (currentProps->IsHorizontalMouseWheel) {
>     wheelEvent.deltaX = (currentProps->MouseWheelDelta + previousHorLeftOverDelta) / WHEEL_DELTA * -1;

I think that this should be:

>     wheelEvent.deltaX =
>       static_cast<double>(currentProps->MouseWheelDelta + previousHorLeftOverDelta) / WHEEL_DELTA;

deltaX is double. So, you need to cast the first int value to double. Then, it supports high resolution scroll such as scrolling per 0.1 line.

And I think that you shouldn't revert the sign for horizontal scroll. Doesn't the positive native delta value mean to scroll to right?

>+    wheelEvent.lineOrPageDeltaX = wheelEvent.deltaX;

I think that this needs static_cast<int32_t> for preventing the warning.

>     previousHorLeftOverDelta = (currentProps->MouseWheelDelta + previousHorLeftOverDelta) % WHEEL_DELTA;

Then, I think that this static variable isn't necessary for this use.

However, then, lineOrPageDeltaX cannot be computed correctly. It never becomes 1 or -1 if the deltaX is always smaller than 1 or larger than -1.

So, I think the block should be:

>   static double previousHorLeftOverDelta = 0.0;
>   if (currentProps->IsHorizontalMouseWheel) {
>     wheelEvent.deltaX = static_cast<double>(currentProps->MouseWheelDelta) / WHEEL_DELTA;
>     if ((wheelEvent.deltaX > 0.0 && previousHorLeftOverDelta < 0.0) ||
>         (wheelEvent.deltaX < 0.0 && previousHorLeftOverDelta > 0.0) {
>       previousHorLeftOverDelta = 0.0;
>     }
>     wheelEvent.lineOrPageDeltaX = static_cast<int32_t>(wheelEvent.deltaX + previousHorLeftOverDelta);
>     previousHorLeftOverDelta = wheelEvent.deltaX - wheelEvent.lineOrPageDeltaX;
>   }

Please write similar code for vertical scroll.

You can test the wheel events in here:
https://bug719320.bugzilla.mozilla.org/attachment.cgi?id=650444

FYI:
https://developer.mozilla.org/en-US/docs/DOM/MouseScrollEvent
https://developer.mozilla.org/en-US/docs/en-US/DOM/DOM_event_reference/DOMMouseScroll
https://developer.mozilla.org/en-US/docs/en-US/DOM/DOM_event_reference/MozMousePixelScroll

https://developer.mozilla.org/en-US/docs/DOM/WheelEvent
https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/wheel

Comment 17

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Followup patch

I think I now understand the intent of `deltaX/Y` versus `lineOrPageDeltaX/Y`.

This patch should set those values appropriately and avoid any nasty double/int casting issues.

Comment 18

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Followup patch

This patch avoids the positive/negative numbers issues in the last patch.  Sorry for the bug spam!

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 668639 [details] [diff] [review]
Followup patch

>+#include <math.h>

You don't need this.

>   if (currentProps->IsHorizontalMouseWheel) {
>-    wheelEvent.deltaX = (currentProps->MouseWheelDelta + previousHorLeftOverDelta) / WHEEL_DELTA * -1;
>-    previousHorLeftOverDelta = (currentProps->MouseWheelDelta + previousHorLeftOverDelta) % WHEEL_DELTA;
>+    wheelEvent.deltaX = static_cast<double>(currentProps->MouseWheelDelta)
>+                      / WHEEL_DELTA_DOUBLE;

Isn't the result of (int / double) double? I mean don't you remove the cast because WHEEL_DELTA_DOUBLE is double. Then, looks like the two lines become a line.

>+    previousHorLeftOverDelta += currentProps->MouseWheelDelta;
>+    if (abs(previousHorLeftOverDelta) >= WHEEL_DELTA) {

Hmm, I don't think this if statement is necessary. E.g., 60 / WHEEL_DELTA is 0. Even if it were necessary, you should use NS_ABS() instead of abs().

>+      wheelEvent.lineOrPageDeltaX = previousHorLeftOverDelta / WHEEL_DELTA;
>+      previousHorLeftOverDelta %= WHEEL_DELTA;
>+    }

Comment 20

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Followup patch

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19)
> Comment on attachment 668639 [details] [diff] [review]
> Followup patch
> 
> >+#include <math.h>
> 
> You don't need this.

Removed.

> >   if (currentProps->IsHorizontalMouseWheel) {
> >-    wheelEvent.deltaX = (currentProps->MouseWheelDelta + previousHorLeftOverDelta) / WHEEL_DELTA * -1;
> >-    previousHorLeftOverDelta = (currentProps->MouseWheelDelta + previousHorLeftOverDelta) % WHEEL_DELTA;
> >+    wheelEvent.deltaX = static_cast<double>(currentProps->MouseWheelDelta)
> >+                      / WHEEL_DELTA_DOUBLE;
> 
> Isn't the result of (int / double) double? I mean don't you remove the cast
> because WHEEL_DELTA_DOUBLE is double. Then, looks like the two lines become
> a line.

I had included the cast to make it more obvious what was happening here. Writing out the cast isn't strictly necessary since it will happen implicitly.  Removed.

> >+    previousHorLeftOverDelta += currentProps->MouseWheelDelta;
> >+    if (abs(previousHorLeftOverDelta) >= WHEEL_DELTA) {
> 
> Hmm, I don't think this if statement is necessary. E.g., 60 / WHEEL_DELTA is
> 0. Even if it were necessary, you should use NS_ABS() instead of abs().
> 
> >+      wheelEvent.lineOrPageDeltaX = previousHorLeftOverDelta / WHEEL_DELTA;
> >+      previousHorLeftOverDelta %= WHEEL_DELTA;
> >+    }

I was trying to be more explicit for the sake of people who read this code later.  In this case, it's probably easier to read without the "if" statement.  Removed.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 669230 [details] [diff] [review]
Followup patch

Looks fine, thank you.

Comment 22

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

https://hg.mozilla.org/projects/elm/rev/86797fe85f2f

Comment 24

[Matt Brubeck (:mbrubeck)]

Resolving bugs in the Firefox for Metro product that are fixed on the elm branch.  Sorry for the bugspam.  Search your email for "bugspam-elm" if you want to find and delete all of these messages at once.