1152011 - APZ no longer clamps wheel scrolling to the scrollable rect

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

This is a regression from introducing WheelScrollAnimation. If the animation is in progress, it arbitrarily accumulates new scroll deltas without bothering to clamp the final destination. If you scroll too far, the scrollbar appears stuck and won't budge in the opposite direction until the animation times out.

Comment 1

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

Attachment: fix

Comment 2

[Botond Ballo [:botond]]

Comment on attachment 8589254 [details] [diff] [review]
fix

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

This can be done more simply: just return false in WheelScrollAnimation::DoSample() if adjustedOffset is zero.

Comment 3

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

Attachment: v2

That will end up being slightly different because it stops the animation and starts a new one. But for all practical purposes I doubt it matters and it is indeed simpler.

Comment 4

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

(Also, the original patch will work if both axes scroll at the same time, and the current one won't.)

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8589869 [details] [diff] [review]
v2

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

Discussed this on IRC. Summary: starting a new animation instead of re-using it doesn't make a noticeable difference, and the scenario where two axes are scrolling separately can be addressed by zeroing out the velocity along an axis if it gets into overscroll (AdjustDisplacement already does this to Axis::mVelocity, but WheelScrollAnimation currently doesn't use Axis::mVelocity). We can address the latter in a follow-up if we want.

::: gfx/layers/apz/src/WheelScrollAnimation.cpp
@@ +43,5 @@
>    mApzc.mX.AdjustDisplacement(displacement.x, adjustedOffset.x, overscroll.x);
>    mApzc.mY.AdjustDisplacement(displacement.y, adjustedOffset.y, overscroll.y,
>                                !aFrameMetrics.AllowVerticalScrollWithWheel());
> +  if (FuzzyEqualsAdditive(adjustedOffset.x, 0.0f) &&
> +      FuzzyEqualsAdditive(adjustedOffset.y, 0.0f))

Let's move IsZero() from AsyncPanZoomController.cpp into APZUtils.h and use it.

Comment 6

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

Attachment: v3

Discussed on IRC, but recap: the above approach ends up not working, since mFinalDestination has to be clamped, and has to be clamped without racing with the input thread (i.e., on Update, not Sample).

This combines both approaches to cover both bases. I'll do a separate patch in another bug for syncing the axis velocity.

Comment 7

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

Attachment: part 1, lock fix

(Related to the next patch, since it Update() will require the lock being taken.)

Comment 8

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

Attachment: part 2, clamp

Same as the original patch, except integrating the IsZero() suggestion.

I couldn't see an easy way to merge DisplacementWillOverscroll into ClampOriginToScrollRange, or vice versa. For DisplacementWillOverscroll, the origin is implicit, and it wants to return more data than the Clamp operation provides.

Comment 9

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

Attachment: part 2, clamp

Simply clamping the destination turns out to not be great either, because we could end up with a very small fractional difference between the final destination and the actual destination the timing function computed.

The main thread simply forces a final scroll to the destination to make sure it's correct. The latest patch here does that for APZ as well.

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8590436 [details] [diff] [review]
part 1, lock fix

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1540,5 @@
>      case ScrollWheelInput::SCROLLMODE_SMOOTH: {
> +      // The lock must be held across the entire update operation, so the
> +      // compositor doesn't end the animation before we get a chance to
> +      // update it.
> +      ReentrantMonitorAutoEnter lock(mMonitor);

Based on the "Intentional scoping for mutex" comment inside SetState(), it shouldn't be called while the monitor is held, but I see there are existing places where we don't obey this. 

Let's do this for now, and I'll file a follow-up to clarify whether not holding the mutex during SetState() is important, and if so, clean up the places where we do.

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8590558 [details] [diff] [review]
part 2, clamp

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

r+ w/comments. Thanks!

::: gfx/layers/apz/src/APZUtils.h
@@ +7,5 @@
>  #ifndef mozilla_layers_APZUtils_h
>  #define mozilla_layers_APZUtils_h
>  
>  #include <stdint.h>                     // for uint32_t
> +#include "mozilla/gfx/Point.h"

Also include "mozilla/FloatingPoint.h" for FuzzyEqualsAdditive().

::: gfx/layers/apz/src/Axis.cpp
@@ +411,5 @@
>  }
>  
> +CSSCoord Axis::ClampOriginToScrollableRect(CSSCoord aOrigin)
> +{
> +  CSSToParentLayerScale zoom = GetFrameMetrics().GetZoom().ToScaleFactor();

For non-root scroll frames, the zoom can be different along the x and y axes, so ScaleFactor2D::ToScaleFactor() will assert.

Instead, give Axis a virtual function GetScaleForAxis(ScaleFactor2D), and override it in AxisX to return the x-scale, and in AxisY to return the y-scale.

::: gfx/layers/apz/src/Axis.h
@@ +171,5 @@
> +  /**
> +   * Clamp a point to the page's scrollable bounds. That is, a scroll
> +   * destination to the returned point will not contain any overscroll.
> +   */
> +  CSSCoord ClampOriginToScrollableRect(CSSCoord aOrigin);

Make this method const.

::: gfx/layers/apz/src/WheelScrollAnimation.cpp
@@ +50,5 @@
> +  } else {
> +    nsPoint position = PositionAt(now);
> +    displacement =
> +      (CSSPoint::FromAppUnits(position) - aFrameMetrics.GetScrollOffset()) * zoom;
> +  }

Little neater:

bool finished = IsFinished(now);
nsPoint sampleDestiantion = finished ? mDestination : PositionAt(now);
ParentLayerPoint displacement = (CSSPoint::FromAppUnits(sampleDestination) - aFrameMetrics.GetScrollOffset()) * zoom;

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> > +CSSCoord Axis::ClampOriginToScrollableRect(CSSCoord aOrigin)
> > +{
> > +  CSSToParentLayerScale zoom = GetFrameMetrics().GetZoom().ToScaleFactor();
> 
> For non-root scroll frames, the zoom can be different along the x and y
> axes, so ScaleFactor2D::ToScaleFactor() will assert.
> 
> Instead, give Axis a virtual function GetScaleForAxis(ScaleFactor2D), and
> override it in AxisX to return the x-scale, and in AxisY to return the
> y-scale.

Another option is to take and return ParentLayerCoords, and have the caller adjust by the zoom.

Comment 13

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

w/ comments addressed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/186d8005fa1f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5541f3f3b0cb

Comment 14

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> Based on the "Intentional scoping for mutex" comment inside SetState(), it
> shouldn't be called while the monitor is held, but I see there are existing
> places where we don't obey this. 
> 
> Let's do this for now, and I'll file a follow-up to clarify whether not
> holding the mutex during SetState() is important, and if so, clean up the
> places where we do.

Filed bug 1153979.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/186d8005fa1f
https://hg.mozilla.org/mozilla-central/rev/5541f3f3b0cb