1086162 - Async scrolling for Windows

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP

Prototype patch. TODO: non-e10s support, smooth scrolling support, fix scrollbars, pixel delta mode support[?]

Works on e10s, but scrollbars aren't drawing.

Comment 1

[Markus Stange [:mstange]]

(In reply to David Anderson [:dvander] from comment #0)
> Works on e10s, but scrollbars aren't drawing.

That's bug 1013377.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP (w/ smooth scrolling, sort of)

Comment 3

[Kartikaya Gupta (email:[e-mail])]

We should definitely sort out the implications of wheel events being cancelable before proceeding using this approach. Will it break web content if we start scrolling unconditionally now?

Comment 4

[Markus Stange [:mstange]]

Absolutely. For example it would make zoomable maps that are embedded on scrollable pages very hard to use. (E.g. here: http://www.trivoo.net/google-maps/ )

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: win-apz-baseline.patch

With bug 1092450 and bug 1076192 this basically works, so might as well try to get it in pref'd off for now. To turn it on there is still more work to do:
 * Experiment more with how it performs, there are mobile behaviors/heuristics that might not make sense for Desktop.
 * The smooth scrolling animation in APZ seems to be slightly different than the one on the main thread, like the main thread might be scrolling by twice the line height.
 * The hit testing/event region patches are needed so we handle events properly.

When e10s is disabled, and apzc is enabled, you still get apz - but it doesn't buy you much since events are still blocked on content. If we want to try for apz without e10s, we could look into something hacky with SetWindowsHookEx.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8521024 [details] [diff] [review]
win-apz-baseline.patch

Jim, could you look at the widget changes?

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8521024 [details] [diff] [review]
win-apz-baseline.patch

Review of attachment 8521024 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't look at the nsWindow.* changes at all (you could even split them into a separate patch if you want). The APZ changes mostly look good, but there's enough comments that I'd like to see a new version. Note that the code entangles with the changes in bug 1090398 and a little bit with bug 1055741. Nothing serious but if either of those lands first you should rebase on top and make sure stuff still builds.

::: gfx/layers/FrameMetrics.h
@@ +644,5 @@
>    // apz.printtree pref is turned on.
>    nsCString mContentDescription;
> +
> +  // The value of GetLineScrollAmount(), for scroll frames.
> +  nsIntSize mLineScrollAmount;

s/nsIntSize/LayoutDeviceIntSize/

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +539,5 @@
> +      if (apzc) {
> +        // When passing the event to the APZC, we need to apply a different
> +        // transform than the one in TransformScreenToGecko, so we need to
> +        // make a copy of the event.
> +        ScrollWheelInput copy(wheelInput);

s/copy/inputForApzc/

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -746,5 @@
>     * frame. Returns true if the smooth scroll should be advanced by one frame,
>     * or false if the smooth scroll has ended.
>     */
>    bool Sample(FrameMetrics& aFrameMetrics, const TimeDuration& aDelta) {
> -

spurious line deletion

@@ -795,5 @@
>      // This can happen if either the layout.css.scroll-behavior.damping-ratio
>      // preference is set to less than 1 (underdamped) or if a smooth scroll
>      // inherits velocity from a fling gesture.
>      if (!IsZero(overscroll)) {
> -

spurious line deletion

@@ +807,5 @@
>          velocity.y = 0;
>        }
>  
> +      if (!gfxPrefs::APZOverscrollEnabled())
> +        return false;

This shouldn't be needed. There is already a check in ::OverscrollBy that should be disabling overscroll if it is not enabled. This codepath should only be used when handing off a fling from an inner scrollable frame to an outer scrollable frame, which is still desirable here I imagine.

@@ +1487,5 @@
> +      deltaY *= scrollAmount.height;
> +      break;
> +    }
> +    default:
> +      MOZ_ASSERT_UNREACHABLE("unknown delta type");

This needs a return statement after it.

@@ +1498,5 @@
> +                            aEvent.mOrigin, ScreenPoint(0, 0), aEvent.modifiers);
> +      OnPanBegin(start);
> +
> +      PanGestureInput move(PanGestureInput::PANGESTURE_PAN, aEvent.mTime, aEvent.mTimeStamp,
> +                           aEvent.mOrigin, ScreenPoint(deltaX, deltaY), aEvent.modifiers);

Comment needed here to the effect of "we want to scale the same visible amount regardless of the zoom, so we interpret deltaX and deltaY as ScreenPixel values"

@@ +1508,5 @@
> +      break;
> +    }
> +
> +    case ScrollWheelInput::SCROLLMODE_SMOOTH: {
> +      CSSPoint delta(deltaX, -deltaY);

delta[XY] shouldn't be interpreted as a CSSPoint here. If you do it this way you'll scroll less when zoomed out. It should be a ScreenPoint (which you can then convert to a CSSPoint for the purposes of setting the smooth scroll offset).

@@ +1510,5 @@
> +
> +    case ScrollWheelInput::SCROLLMODE_SMOOTH: {
> +      CSSPoint delta(deltaX, -deltaY);
> +
> +      if (mState != SMOOTH_SCROLL) {

Add a comment to why this condition makes a difference.

@@ +1517,5 @@
> +      } else {
> +        mFrameMetrics.SetSmoothScrollOffset(mFrameMetrics.GetSmoothScrollOffset() + delta);
> +      }
> +      StartSmoothScroll();
> +      RequestContentRepaint();

I don't think you need this RequestContentRepaint call here. Any particular reason you added it?

::: layout/base/nsDisplayList.cpp
@@ +719,5 @@
> +
> +    nsSize lineScrollAmount = scrollableFrame->GetLineScrollAmount();
> +    nsIntSize lineScrollAmountInDevPixels(
> +      presContext->AppUnitsToDevPixels(lineScrollAmount.width),
> +      presContext->AppUnitsToDevPixels(lineScrollAmount.height));

Add a "static LayoutDeviceIntSize FromAppUnitsRounded(const nsSize& aSize, nscoord aAppUnitsPerDevPixel)" to the LayoutDevicePixel struct in Units.h and use that here.

::: widget/InputData.h
@@ +366,5 @@
>    ScreenIntPoint mPoint;
>  };
>  
> +class ScrollWheelInput : public InputData
> +{

This needs some comments, particularly with respect to how aDeltaX and aDeltaY should be interpreted - what do their magnitudes and signs mean?

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8521024 [details] [diff] [review]
win-apz-baseline.patch

Review of attachment 8521024 [details] [diff] [review]:
-----------------------------------------------------------------

win32 changes look familiar from my work with apzc in metro land, although it's been a while.

Are we checking return results here? When I tackled this ages ago I did a little write up on what I ran into - 

https://bugzilla.mozilla.org/show_bug.cgi?id=907100#c17

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Jim Mathies [:jimm] from comment #8)
> Are we checking return results here? When I tackled this ages ago I did a
> little write up on what I ran into - 
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=907100#c17

re: the content event problems - yeah, this patch has that problem, so we can't turn this on by default. kats' hit testing/event region work should lead to fixing those problems.

I haven't looked at the framerate yet, but definitely compositor scrolling does not feel as smooth as DOM scrolling. I'm hoping to look into that in a separate bug.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> @@ +807,5 @@
> >          velocity.y = 0;
> >        }
> >  
> > +      if (!gfxPrefs::APZOverscrollEnabled())
> > +        return false;
> 
> This shouldn't be needed. There is already a check in ::OverscrollBy that
> should be disabling overscroll if it is not enabled. This codepath should
> only be used when handing off a fling from an inner scrollable frame to an
> outer scrollable frame, which is still desirable here I imagine.

This path never goes through OverscrollBy, it uses OverscrollAnimation.

Comment 11

[Botond Ballo [:botond]]

(In reply to David Anderson [:dvander] from comment #10)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> > @@ +807,5 @@
> > >          velocity.y = 0;
> > >        }
> > >  
> > > +      if (!gfxPrefs::APZOverscrollEnabled())
> > > +        return false;
> > 
> > This shouldn't be needed. There is already a check in ::OverscrollBy that
> > should be disabling overscroll if it is not enabled. This codepath should
> > only be used when handing off a fling from an inner scrollable frame to an
> > outer scrollable frame, which is still desirable here I imagine.
> 
> This path never goes through OverscrollBy, it uses OverscrollAnimation.

Hmm... it appears I accidentally removed the conditioning of overscroll-during-a-fling on APZOverscrollEnabled() in bug 1066888. Whoops!

I'll fix that in a follow-up to bug 1066888, and then you shouldn't need this here.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: bug1086162-win-apz-scrolling.patch

APZ changes, with review comments addressed. I kept the overscroll check until that follow-up fix lands, since otherwise scrolling behaves really weird.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: bug1086162-win-apz-widget.patch

Widget changes - this version checks the event status from APZ. It also checks whether the control modifier is set so we don't break zoom-scrolling.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8521895 [details] [diff] [review]
bug1086162-win-apz-scrolling.patch

Review of attachment 8521895 [details] [diff] [review]:
-----------------------------------------------------------------

This will need to be rebased on top of bug 1055741 which is on inbound now. And actually the rebasing is not as trivial as I thought before, as it breaks how you implement the wheel scroll using pan gesture events. It's still fixable though - see my comments below.

::: gfx/ipc/GfxMessageUtils.h
@@ +719,5 @@
>  };
>  
>  template<>
> +struct ParamTraits<mozilla::LayoutDeviceIntSize>
> +{

Instead of adding this, you can change the existing one for mozilla::gfx::IntSize to be templated so that it looks like the one for mozilla::gfx::SizeTyped<T>. That will cover LayoutDeviceIntSize as well as other cases (including mozilla::gfx::IntSize, which is just a typedef to IntSizeTyped<UnknownUnits>).

::: gfx/layers/FrameMetrics.h
@@ +542,5 @@
>    {
>      mMayHaveTouchListeners = aMayHaveTouchListeners;
>    }
>  
> +  const LayoutDeviceIntSize &GetLineScrollAmount() const

& should be next to the type instead of method name

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +541,5 @@
> +        // transform than the one in TransformScreenToGecko, so we need to
> +        // make a copy of the event.
> +        ScrollWheelInput inputForApzc(wheelInput);
> +        transformToApzc = GetScreenToApzcTransform(apzc);
> +        ApplyTransform(&inputForApzc.mOrigin, transformToApzc);

This code will need to change because of bug 1055741.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +809,5 @@
>          velocity.y = 0;
>        }
>  
> +      if (!gfxPrefs::APZOverscrollEnabled())
> +         return false;

You can split this bit out into a new bug if you don't want to wait for Botond to do it.

@@ +1500,5 @@
> +                            aEvent.mOrigin, ScreenPoint(0, 0), aEvent.modifiers);
> +      OnPanBegin(start);
> +
> +      PanGestureInput move(PanGestureInput::PANGESTURE_PAN, aEvent.mTime, aEvent.mTimeStamp,
> +                           aEvent.mOrigin, ScreenPoint(deltaX, deltaY), aEvent.modifiers);

So this won't work as-is after bug 1055741, because APZCTreeManager is responsible for populating the mLocal versions of the pan start point and pan displacement, which are the ones that actually get used in OnPan. Here you are populating the ScreenPixel versions which will be ignored, and so OnPan will have no effect.

I believe that you want deltaX and deltaY to be interpreted directly as ParentLayerPixels here and populate the pan gesture's mLocalPanDisplacement with that. The mLocalPanStartPoint should be populated from the mLocalOrigin (which you will need to add) from the wheel event.

@@ +1511,5 @@
> +    }
> +
> +    case ScrollWheelInput::SCROLLMODE_SMOOTH: {
> +      // Invert the |y| axis, since it arrives flipped.
> +      LayoutDevicePoint devicePixelDelta(deltaX, -deltaY);

Isn't the x axis flipped too? Your comments in InputData.h have the deltaX and deltaY as being "consistent" (i.e. > 0 means the scroll offset needs to reduce on that axis). For the record I would prefer if you had it the other way (i.e. > 0 means the scroll offset needs to increase on that axis) and negate it if necessary when creating the input event in the widget code.

@@ +1512,5 @@
> +
> +    case ScrollWheelInput::SCROLLMODE_SMOOTH: {
> +      // Invert the |y| axis, since it arrives flipped.
> +      LayoutDevicePoint devicePixelDelta(deltaX, -deltaY);
> +      CSSPoint delta = devicePixelDelta / mFrameMetrics.mDevPixelsPerCSSPixel;

Post bug-1055741 I believe this should look like:

// We want to scroll the same proportion of the composition bounds
// regardless of the zoom of this APZC, so interpret the delta as
// a ParentLayerPixel value.
CSSPoint delta = ParentLayerPoint(deltaX, -deltaY) / mFrameMetrics.GetZoom();

::: widget/InputData.h
@@ +402,5 @@
> +  {}
> +
> +  ScrollDeltaType mDeltaType;
> +  ScrollMode mScrollMode;
> +  ScreenPoint mOrigin;

This will now need a |ParentLayerPoint mLocalOrigin| as well.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> Post bug-1055741 I believe this should look like:
> 
> // We want to scroll the same proportion of the composition bounds
> // regardless of the zoom of this APZC, so interpret the delta as
> // a ParentLayerPixel value.
> CSSPoint delta = ParentLayerPoint(deltaX, -deltaY) / mFrameMetrics.GetZoom();

Thanks for the tips on how to rebase this. The only thing I'm not sure about is this change. The line height is always computed in device pixels, so if there's an apzc scale of 3, but user zoom of 1, the line height will be off by a factor of 3. It seems like we'd have to factor the two zooms separately.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: bug1086162-win-apz-scrolling.patch

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8522827 [details] [diff] [review]
bug1086162-win-apz-scrolling.patch

Review of attachment 8522827 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed. Based on the IRC discussion (which I'll attach to the bug in a sec, for future reference) we're leaving the scroll amount like this for now as it more closely matches what layout already does with scroll wheel events, but we might change it in the future.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +510,5 @@
>        break;
> +    } case SCROLLWHEEL_INPUT: {
> +      ScrollWheelInput& wheelInput = aEvent.AsScrollWheelInput();
> +      nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(wheelInput.mOrigin,
> +                                                            &inOverscrolledApzc);

This will need a small rebase on top of bug 1090398 which is on inbound (sorry, you must really hate me right now!).

1) &inOverscrolledApzc here needs to become &hitResult
2) Just inside the if (apzc) block add a MOZ_ASSERT(hitResult == ApzcHitRegion || hitResult == ApzcContentRegion);
3) The call mInputQueue->ReceiveInputEvent will have an extra parameter as argument #2: /* aTargetConfirmed = */ hitResult == ApzcHitRegion

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1507,5 @@
> +      ParentLayerPoint delta = (devicePixelDelta / mFrameMetrics.mDevPixelsPerCSSPixel) *
> +                               mFrameMetrics.GetZoom();
> +
> +      PanGestureInput move(PanGestureInput::PANGESTURE_PAN, aEvent.mTime, aEvent.mTimeStamp,
> +                           aEvent.mOrigin, ScreenPoint(deltaX, deltaY), aEvent.modifiers);

Replace this ScreenPoint(deltaX, deltaY) with ToScreenCoordinates(delta, aEvent.mLocalOrigin) for more correctness.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: IRC conversation about the scroll amount

Saving this for posterity.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, apz changes

rebased w/ final comments applied (carrying over r+)

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, nsWindow changes

Final version of the widget changes.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 8523261 [details] [diff] [review]
part 2, nsWindow changes

Review of attachment 8523261 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +3742,5 @@
>  
> +nsEventStatus nsWindow::MaybeDispatchAsyncWheelEvent(WidgetGUIEvent* aEvent)
> +{
> +  if (aEvent->mClass != eWheelEventClass)
> +    return nsEventStatus_eIgnore;

nit - please add paren to the various if statements here.

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50952288e4a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb3fcff1e9c3

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50952288e4a4
https://hg.mozilla.org/mozilla-central/rev/cb3fcff1e9c3

Comment 25

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

Comment on attachment 8523261 [details] [diff] [review]
part 2, nsWindow changes

>+nsEventStatus nsWindow::MaybeDispatchAsyncWheelEvent(WidgetGUIEvent* aEvent)
>+{
>+  if (aEvent->mClass != eWheelEventClass)
>+    return nsEventStatus_eIgnore;
>+
>+  WidgetWheelEvent* event = aEvent->AsWheelEvent();
>+
>+  // Otherwise, scroll-zoom won't work.
>+  if (event->IsControl())
>+    return nsEventStatus_eIgnore;

This is bad. Which modifier causes what action depends on prefs. So, you should check prefs for this purpose.
http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp?rev=ee9403c598d6#5210

I think that EventStateManager::WheelPrefs should be renamed as MouseWheelPrefs and it should be accessible from both widget/ and dom/events.