879562 - Use WinMouseScrollHandler for winrt mouse wheel handling

Status: RESOLVED FIXED

(Core Graveyard :: Widget: WinRT, defect)

Description

[Rodrigo Silveira [:rsilveira] [:rodms]]

Quoting Tim from bug 811429:

We only send "line" wheel events, not "pixel" wheel events; see [2].

If we want pixel scroll, we'll have to do something similar to [1].  This should be a pretty simple change.

[1] https://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm?rev=53afe9fba5c0#4428
[2] https://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp?rev=43872f1bfbda#494

Comment 1

[Marco Mucci [:MarcoM]]

For consideration as a Defect or Change Story.

Comment 2

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

I've been working on this today.  Sending pixel scroll events from widget is easy, but for some reason we're only receiving PointerWheelChanged events on our CoreWindow when the MouseWheelDelta is greater than or equal to 120, which means that the scrolling is just as choppy as when we send line scroll events.

I've spent a bunch of time trying to find out how to get finer-grained events but haven't had any luck so far :(

Comment 3

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

Back when I was investigating this, I discovered that certain types of apps are incapable of receiving scroll information at finer granularity than "one detent." I gave my research to Jim, who filed a report with MSFT.

As far as I know, we haven't heard back. There's really nothing we can do here until we get word from MSFT on the issue (should we remove shovel-ready from the whiteboard?)

Jimm: Has there been any word on this issue?

Comment 4

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #3)
> Back when I was investigating this, I discovered that certain types of apps
> are incapable of receiving scroll information at finer granularity than "one
> detent." I gave my research to Jim, who filed a report with MSFT.
> 
> As far as I know, we haven't heard back. There's really nothing we can do
> here until we get word from MSFT on the issue (should we remove shovel-ready
> from the whiteboard?)
> 
> Jimm: Has there been any word on this issue?

This got sent to the win8 immersive people, and they are apparently busy. The request is still open. I'd suggest we go ahead and fix this such that it works on hardware we know we get the right data from.

Comment 5

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

(In reply to Jim Mathies [:jimm] from comment #4)
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #3)
> > Back when I was investigating this, I discovered that certain types of apps
> > are incapable of receiving scroll information at finer granularity than "one
> > detent." I gave my research to Jim, who filed a report with MSFT.
> > 
> > As far as I know, we haven't heard back. There's really nothing we can do
> > here until we get word from MSFT on the issue (should we remove shovel-ready
> > from the whiteboard?)
> > 
> > Jimm: Has there been any word on this issue?
> 
> This got sent to the win8 immersive people, and they are apparently busy.
> The request is still open. I'd suggest we go ahead and fix this such that it
> works on hardware we know we get the right data from.

My understanding is that there is no hardware that will give us fine-grained scroll information: It appears to be the OS that is witholding that information from our app.

Unassigning myself and removing [shovel-ready]. We'll have to wait and see what the results of the ticket with MSFT are.

Comment 6

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #5)
> My understanding is that there is no hardware that will give us fine-grained
> scroll information: It appears to be the OS that is witholding that
> information from our app.

I thought we found that some devices worked as expected, specifically the surface pro?

Comment 7

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #6)
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #5)
> > My understanding is that there is no hardware that will give us fine-grained
> > scroll information: It appears to be the OS that is witholding that
> > information from our app.
> 
> I thought we found that some devices worked as expected, specifically the
> surface pro?

Using the soft cover touch pad, I receive increments smaller than 120 for get_MouseWheelDelta.

Comment 8

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

Attachment: Patch v1 (WIP)

Jimm and I discussed this in #windev.  I'm attaching the WIP patch that I had created (oh so many weeks ago). Hopefully this still builds ;)

Jimm: Let me know how this works out on your hardware!

Comment 9

[Jim Mathies [:jimm]]

Generally this works, but we lose smooth scroll due to only sending pixel scroll events. I think we want to investigate hooking up widget/windows/MouseScrollHandler to this backend. We can intercept the raw windowing events on our window subclass and skip the winrt handling all together.

Comment 11

[Jim Mathies [:jimm]]

Attachment: use MouseScrollHandler for scrolling handling v.1

This has one hackish thing in it I'd like to try and clean up, but generally MouseScrollHandler scroll works well, and brings our scroll performance up to that of desktop.

Comment 12

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

Comment on attachment 787180 [details] [diff] [review]
use MouseScrollHandler for scrolling handling v.1

Almost looks good to me.

>-MouseScrollHandler::ProcessMessage(nsWindow* aWindow, UINT msg,
>+MouseScrollHandler::ProcessMessage(nsWindowBase* aWindow, UINT msg,
>                                    WPARAM wParam, LPARAM lParam,
>                                    MSGResult& aResult)

I'd prefer aWidget instead of aWindow if it's nsWindowBase because other shared modules use the name. aWindow sounds nsWindow*, historically.

The replacement should be done in a separated patch.

>-    nsWindow* destWindow = WinUtils::GetNSWindowPtr(underCursorWnd);
>+    nsWindowBase* destWindow = WinUtils::GetNSWindowPtr(underCursorWnd);

I think that GetNSWindowPtr() and SetNSWindowPtr() should be renamed with GetNSWindowBasePtr() and SetNSWindowBasePtr().

And it should be done in another separated patch.

>diff -r 0e90c9520e91 widget/windows/WinMouseScrollHandler.h
>--- a/widget/windows/WinMouseScrollHandler.h	Wed Aug 07 16:04:01 2013 -0500
>+++ b/widget/windows/WinMouseScrollHandler.h	Wed Aug 07 17:52:23 2013 -0500
>@@ -7,16 +7,17 @@
> #ifndef mozilla_widget_WinMouseScrollHandler_h__
> #define mozilla_widget_WinMouseScrollHandler_h__
> 
> #include "nscore.h"
> #include "nsDebug.h"
> #include "mozilla/Assertions.h"
> #include "mozilla/TimeStamp.h"
> #include <windows.h>
>+#include "nsWindowBase.h"

Don't include another header file as far as possible. Use forward declaration.
> 
> class nsWindow;

I guess that this it not necessary, anymore.

>--- a/widget/windows/nsWindow.h	Wed Aug 07 16:04:01 2013 -0500
>+++ b/widget/windows/nsWindow.h	Wed Aug 07 17:52:23 2013 -0500
>@@ -80,16 +80,17 @@
> 
>   NS_DECL_ISUPPORTS_INHERITED
> 
>   friend class nsWindowGfx;
> 
>   // nsWindowBase
>   virtual void InitEvent(nsGUIEvent& aEvent, nsIntPoint* aPoint = nullptr) MOZ_OVERRIDE;
>   virtual bool DispatchWindowEvent(nsGUIEvent* aEvent) MOZ_OVERRIDE;
>+  virtual nsWindow* GetParentWindow(bool aIncludeOwner);

I think this should return nsWindowBase*. And the name should be GetParentWindowBase().

>-  nsWindow*               GetParentWindow(bool aIncludeOwner);

Then, this should be:

nsWindow* GetParentWindow(bool aIndludeOwner)
{
  return static_cast<nsWindow*>(GetParentWindowBase(aIndludeOwner));
}

>diff -r 0e90c9520e91 widget/windows/nsWindowBase.h
>--- a/widget/windows/nsWindowBase.h	Wed Aug 07 16:04:01 2013 -0500
>+++ b/widget/windows/nsWindowBase.h	Wed Aug 07 17:52:23 2013 -0500
>@@ -6,32 +6,41 @@
> #ifndef nsWindowBase_h_
> #define nsWindowBase_h_
> 
> #include "nsBaseWidget.h"
> #include "nsGUIEvent.h"
> #include "npapi.h"
> #include <windows.h>
> 
>+class nsWindow;

Please don't use concrete sub class of nsWindowBase from this file!

>   /*
>+   * Return the parent widget, if it exists.
>+   */
>+  virtual nsWindow* GetParentWindow(bool aIncludeOwner)
>+  {
>+    return nullptr;
>+  }

See above for the name and result type.

>-// seem to indicate that this event can be triggered from other types of input
>-// (i.e. pen, touch).
>-HRESULT
>-MetroInput::OnPointerWheelChanged(UI::Core::ICoreWindow* aSender,
>-                                  UI::Core::IPointerEventArgs* aArgs)
>-{
>-#ifdef DEBUG_INPUT
>-  LogFunction();
>-#endif
>-  WRL::ComPtr<UI::Input::IPointerPoint> currentPoint;
>-  WRL::ComPtr<UI::Input::IPointerPointProperties> props;
>-  Foundation::Point position;
>-  uint64_t timestamp;
>-  float pressure;
>-  boolean horzEvent;
>-  int32_t delta;
>-
>-  aArgs->get_CurrentPoint(currentPoint.GetAddressOf());
>-  currentPoint->get_Position(&position);
>-  currentPoint->get_Timestamp(&timestamp);
>-  currentPoint->get_Properties(props.GetAddressOf());
>-  props->get_Pressure(&pressure);
>-  props->get_IsHorizontalMouseWheel(&horzEvent);
>-  props->get_MouseWheelDelta(&delta);
>-
>-  WheelEvent wheelEvent(true, NS_WHEEL_WHEEL, mWidget.Get());
>-  mModifierKeyState.Update();
>-  mModifierKeyState.InitInputEvent(wheelEvent);
>-  wheelEvent.refPoint = LayoutDeviceIntPoint::FromUntyped(MetroUtils::LogToPhys(position));
>-  wheelEvent.time = timestamp;
>-  wheelEvent.inputSource = nsIDOMMouseEvent::MOZ_SOURCE_MOUSE;
>-  wheelEvent.pressure = pressure;
>-  wheelEvent.deltaMode = nsIDOMWheelEvent::DOM_DELTA_LINE;
>-
>-  static int previousVertLeftOverDelta = 0;
>-  static int previousHorLeftOverDelta = 0;
>-  // Since we have chosen DOM_DELTA_LINE as our deltaMode, deltaX or deltaY
>-  // should be the number of lines that we want to scroll.  Windows has given
>-  // us delta, which is a more precise value, and the constant WHEEL_DELTA,
>-  // which defines the threshold of wheel movement before an action should
>-  // be taken.
>-  if (horzEvent) {
>-    wheelEvent.deltaX = delta / WHEEL_DELTA_DOUBLE;
>-    if ((delta > 0 && previousHorLeftOverDelta < 0)
>-     || (delta < 0 && previousHorLeftOverDelta > 0)) {
>-      previousHorLeftOverDelta = 0;
>-    }
>-    previousHorLeftOverDelta += delta;
>-    wheelEvent.lineOrPageDeltaX = previousHorLeftOverDelta / WHEEL_DELTA;
>-    previousHorLeftOverDelta %= WHEEL_DELTA;
>-  } else {
>-    int mouseWheelDelta = -1 * delta;
>-    wheelEvent.deltaY = mouseWheelDelta / WHEEL_DELTA_DOUBLE;
>-    if ((mouseWheelDelta > 0 && previousVertLeftOverDelta < 0)
>-     || (mouseWheelDelta < 0 && previousVertLeftOverDelta > 0)) {
>-      previousVertLeftOverDelta = 0;
>-    }
>-    previousVertLeftOverDelta += mouseWheelDelta;
>-    wheelEvent.lineOrPageDeltaY = previousVertLeftOverDelta / WHEEL_DELTA;
>-    previousVertLeftOverDelta %= WHEEL_DELTA;
>-  }
>-
>-  DispatchEventIgnoreStatus(&wheelEvent);
>-
>-  WRL::ComPtr<UI::Input::IPointerPoint> point;
>-  aArgs->get_CurrentPoint(point.GetAddressOf());
>-  mGestureRecognizer->ProcessMouseWheelEvent(point.Get(),
>-                                             wheelEvent.IsShift(),
>-                                             wheelEvent.IsControl());
>-  return S_OK;
>-}
>-

Awesome!!!

>--- a/widget/windows/winrt/MetroWidget.cpp	Wed Aug 07 16:04:01 2013 -0500
>+++ b/widget/windows/winrt/MetroWidget.cpp	Wed Aug 07 17:52:23 2013 -0500
>@@ -232,16 +234,17 @@
>     NS_WARNING("New eWindowType_toplevel window requested after FrameworkView widget created.");
>     NS_WARNING("Widget created but the physical window does not exist! Fix me!");
>     return NS_OK;
>   }
> 
>   // the main widget gets created first
>   gTopLevelAssigned = true;
>   MetroApp::SetBaseWidget(this);
>+  WinUtils::SetNSWindowPtr(mWnd, (nsWindow*)this);

Please rewrite WinUtils::SetNSWindowBasePtr(mWnd, this);

So, I think that there should be:

void WinUtils::SetNSWindowBasePtr();
nsWindowBase* WinUtils::GetNSWindowBasePtr();
nsWindow* WinUtils::GetNSWindowPtr() // for compatibility
{
  return static_cast<nsWindow*>(GetNSWindowBasePtr());
}


>@@ -269,16 +272,17 @@
> 
>   // Prevent the widget from sending additional events.
>   mWidgetListener = nullptr;
>   mAttachedWidgetListener = nullptr;
> 
>   // Release references to children, device context, toolkit, and app shell.
>   nsBaseWidget::Destroy();
>   nsBaseWidget::OnDestroy();
>+  WinUtils::SetNSWindowPtr(mWnd, nullptr);

Same.


So, I think that there should be three patches:

part 1: Change WinUtils and nsWindowBase (and nsWindow's method for the new method of nsWindowBase), first.
part 2: Replace nsWindow* and aWindow are replaced with nsWindowBase* and aWidget in WinMouseScrollHandler.
part 3: Use WinMouseScrollHandler from windows/winrt.

Comment 13

[Jim Mathies [:jimm]]

> 
> Please rewrite WinUtils::SetNSWindowBasePtr(mWnd, this);
> 
> So, I think that there should be:
> 
> void WinUtils::SetNSWindowBasePtr();
> nsWindowBase* WinUtils::GetNSWindowBasePtr();
> nsWindow* WinUtils::GetNSWindowPtr() // for compatibility
> {
>   return static_cast<nsWindow*>(GetNSWindowBasePtr());
> }
> 

This is the one hackish bit I mentioned needed cleanup. We use GetNSWindowPtr to much to convert them all to base ptr calls. I like your approach.

Comment 14

[Jim Mathies [:jimm]]

Attachment: part 1

tried to break these up as best I could.

Comment 15

[Jim Mathies [:jimm]]

Attachment: part 2

Comment 16

[Jim Mathies [:jimm]]

Attachment: part 3

Comment 17

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

Comment on attachment 787637 [details] [diff] [review]
part 1

Great!

Comment 18

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

Comment on attachment 787639 [details] [diff] [review]
part 3

>diff -r e054564a4573 widget/windows/WinMouseScrollHandler.cpp
>--- a/widget/windows/WinMouseScrollHandler.cpp	Thu Aug 08 11:57:42 2013 -0500
>+++ b/widget/windows/WinMouseScrollHandler.cpp	Thu Aug 08 11:58:12 2013 -0500
>@@ -1082,16 +1082,20 @@
>  * Device::Elantech
>  *
>  ******************************************************************************/
> 
> /* static */
> void
> MouseScrollHandler::Device::Elantech::Init()
> {
>+  // Not supported in metro mode.
>+  if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro)
>+    return;

Please use

if () {

}

even if the content is only one line.

And I guess that you need to add check in MouseScrollHandler::Device::Init() instead of here. If somebody would report bugs with specific device, we should unlock the hack only for it.

Comment 19

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/f40e7977de8d
remote:   https://hg.mozilla.org/integration/fx-team/rev/e92ad04df9ba
remote:   https://hg.mozilla.org/integration/fx-team/rev/4677ef0ec145

thanks!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f40e7977de8d
https://hg.mozilla.org/mozilla-central/rev/e92ad04df9ba
https://hg.mozilla.org/mozilla-central/rev/4677ef0ec145