Closed Bug 879562 Opened 7 years ago Closed 6 years ago

Use WinMouseScrollHandler for winrt mouse wheel handling

Categories

(Core Graveyard :: Widget: WinRT, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: rsilveira, Assigned: jimm)

References

Details

(Whiteboard: [preview][ms-support][113061710520878] feature=work)

Attachments

(3 files, 2 obsolete files)

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
For consideration as a Defect or Change Story.
Blocks: metrov1it9
Blocks: 831920
No longer blocks: metrov1triage, metrov1it9
Summary: Improve pixel scroll events in imersive mode → Work - Improve pixel scroll events in imersive mode
Whiteboard: [shovel-ready] → [shovel-ready] feature=work
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 :(
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
No longer blocks: 775718
Whiteboard: [shovel-ready] feature=work → [shovel-ready][preview] feature=work
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?
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
(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.
Assignee: tabraldes → nobody
Status: ASSIGNED → NEW
Whiteboard: [shovel-ready][preview] feature=work → [preview] feature=work
Blocks: 898831
(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?
(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.
Attached patch Patch v1 (WIP) (obsolete) — Splinter Review
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!
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.
Blocks: 850737
No longer blocks: 831920
Component: Firefox Start → Widget: WinRT
Product: Firefox for Metro → Core
Summary: Work - Improve pixel scroll events in imersive mode → Work - Support pixel scroll events in imersive mode
Duplicate of this bug: 898831
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.
Assignee: nobody → jmathies
Attachment #785883 - Attachment is obsolete: true
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.
> 
> 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.
Summary: Work - Support pixel scroll events in imersive mode → Use WinMouseScrollHandler for winrt mouse wheel handling
Attached patch part 1Splinter Review
tried to break these up as best I could.
Attachment #787180 - Attachment is obsolete: true
Attachment #787637 - Flags: review?(masayuki)
Attached patch part 2Splinter Review
Attachment #787638 - Flags: review?(masayuki)
Attached patch part 3Splinter Review
Attachment #787639 - Flags: review?(masayuki)
Comment on attachment 787637 [details] [diff] [review]
part 1

Great!
Attachment #787637 - Flags: review?(masayuki) → review+
Attachment #787638 - Flags: review?(masayuki) → review+
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.
Attachment #787639 - Flags: review?(masayuki) → review+
Depends on: 903368
Whiteboard: [preview] feature=work → [preview][ms-support][113061710520878] feature=work
OS: Windows 8 Metro → Windows 8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.