Closed Bug 998025 Opened 6 years ago Closed 6 years ago

Provide some sort of overscroll termination effect when overscroll handoff is not possible

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: kats, Assigned: botond)

References

Details

(Whiteboard: [ucid:graphics13])

Attachments

(13 files, 30 obsolete files)

4.23 KB, patch
kats
: review+
Details | Diff | Splinter Review
6.92 KB, patch
kats
: review+
Details | Diff | Splinter Review
2.78 KB, patch
kats
: review+
Details | Diff | Splinter Review
7.46 KB, patch
kats
: review+
Details | Diff | Splinter Review
3.59 KB, patch
botond
: review+
Details | Diff | Splinter Review
16.16 KB, patch
botond
: review+
Details | Diff | Splinter Review
5.25 KB, patch
kats
: review+
Details | Diff | Splinter Review
16.56 KB, patch
kats
: review+
Details | Diff | Splinter Review
22.51 KB, patch
botond
: review+
Details | Diff | Splinter Review
4.65 KB, patch
botond
: review+
Details | Diff | Splinter Review
35.44 KB, patch
botond
: review+
Details | Diff | Splinter Review
4.45 KB, patch
kats
: review+
Details | Diff | Splinter Review
1.99 KB, patch
botond
: review+
Details | Diff | Splinter Review
This bug is to track the UX request (aimed for B2G 2.0) to provide a "bounce" effect when you reach the end of a scrolling container. Except it won't be "bounce" because of legal reasons, it'll be something else.

For a first cut this would extend the overscroll handoff code and the effect would be applied to the last scrollable frame in the handoff chain. Subsequent bugs can be filed to tune the behaviour once that is done.
Assigning to botond for now per our discussions.
Assignee: nobody → botond
Hypothesis: overscroll resistance is best described as a 2D constraint
(spring), because:

* Springs respond to velocity
* Springs snap back to a resting position
* Springs offer increased resistance to increased extension (e.g. springs
  "fight back" when you stretch them too far).

Inputs for 2D Verlet-integrated springs:

* particle 0 (free particle)
  * mass
  * pos x
  * pos y
  * prev pos x
  * prev pos y
  * delta T
* particle 1 (anchor particle)
  * mass: Infinity
  * pos x
  * pos y
  * prev pos x
  * prev pos y
  * delta T
* Spring 0 (snap-back spring)
  * particle 0
  * particle 1
  * resting length: 0
  * stiffness

Fling velocity/user drag force would be applied via F / Hooke's law
https://en.wikipedia.org/wiki/Hooke%27s_law to find spring length at time.
Snap-back could use same equation, but solving for F instead of length.
Scenarios for overscroll:

1. fling velocity would cause an eventual resting point outside of the bounds of the scroll. In this case, the "left over" force is applied to the overscroll physics (which respond by moving back to their resting point).

2. user drags "past" the normal bound of scrolling. E.g. I am at the top of a scrolling view and pull down. The further I pull down, the more the overscroll physics resist, until overscroll reaches full extension. Upon release of finger, overscroll physics snap back to resting position.
Just to be clear, when we talk about what we won't do ("bounce") we mean that overscroll, once released, will just go back to the 0 position, rather than go past it in the other direction, perhaps bouncing back and forth a few times?
Also, how is this effect different from the "overscroll" effect when getting to the end of the edge effect list and getting the stretch and snap (sorry, don't know what we're calling all of these things - by the edge effect I mean when we change between apps just dragging on the edge)
(In reply to Milan Sreckovic [:milan, travelling, maybe slow to respond] from comment #4)
> Just to be clear, when we talk about what we won't do ("bounce") we mean
> that overscroll, once released, will just go back to the 0 position, rather
> than go past it in the other direction, perhaps bouncing back and forth a
> few times?

It's possible for spring-based physics to bounce back and forth around resting point, or not, depending on how you tune mass and spring stiffness.
(In reply to Milan Sreckovic [:milan, travelling, maybe slow to respond] from comment #5)
> Also, how is this effect different from the "overscroll" effect when getting
> to the end of the edge effect list and getting the stretch and snap (sorry,
> don't know what we're calling all of these things - by the edge effect I
> mean when we change between apps just dragging on the edge)

Great question. Sheets = the edge gesture to move between apps and app states. In theory, it would be elegant to tie the spacial model of sheets into overscroll effect, but I think we'll have to keep the 2 things separate because of technical constraints:

* Overscroll is handled at the Gecko level, where sheets are managed by Gaia as separate iframes.
* Overscroll will need to work on nested scrolling views, since we frequently use multiple scrolling views within the same sheet (see Music app tabs for an example). Sheets are specifically system-level iframes rather than any scrolling view.

In the full sheets model, you can think of sheets as representing page refreshes. Clicking a link will slide over a new sheet, for example. It'll be a couple releases before we get to the full sheets model, however.
I discussed the use of shaders to accomplish these effects with some other people on the gfx team. The general consensus was that pretty much all of the effects are doable, but some are much more expensive than others. Things like blurring or averaging require readbacks and will be more expensive. Discontinuous transform functions will also be more expensive than continuous functions, because they generally require branching in the shader code. A lot of this depends on the implementation details but it would be good to have a list of effects that would be acceptable rather than just having The One Perfect Effect.

The perf hit will also vary across GPUs and some effects may be too slow on some GPUs. It would be worth thinking about whether in such cases it would be better to have a cheaper effect across the board, or to disable the effects on some devices and leave them enabled on others.

Also with respect to implementing the shader and hooking it up to the APZ code, Bas brought up tiling as a possible obstacle. Since we draw individual tiles with separate DrawQuad calls, we will have to add some more machinery to TiledContentClient that can adjust the shader as needed for different tiles.
The other thing that came up is for hit detection. If the user taps on some element while an overscroll effect is applied, we probably can't or don't want to map that back to the element the user visually tapped on. We have three sane choices:

1) user interaction cancels the effect (i.e. the effect immediately ends and the tap goes to whatever is now at the tap point)
2) user interaction pretends the effect isn't there (i.e. the tap goes to where the element will be after the effect is over)
3) user interaction is ignored when an effect is present (i.e. tap does nothing)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> The other thing that came up is for hit detection. If the user taps on some
> element while an overscroll effect is applied, we probably can't or don't
> want to map that back to the element the user visually tapped on.

Here's my take from UX side...

Try: 3) user interaction is ignored when an effect is present (i.e. tap does nothing)

Why:
* The other approaches break the continuity of the physics model.
  It feels like a "time skip" or "action at a distance".
* It's a temporary state that wants to "spring back" to a normal state on its own.
  You're only there if velocity is high or you're actively tugging past boundaries of page.
  In both cases, you're going to have an easier time hitting the correct thing when overscroll springs back.
* Principle of least surprise. It's a pretty impressive feat to both tug the overscroll
  down and tap an element. Chances are if it is triggered, its not intentional.
Makes sense, I think we can roll with that.
(In reply to Gordon Brander :gordonb from comment #6)
> (In reply to Milan Sreckovic [:milan, travelling, maybe slow to respond]
> from comment #4)
> > Just to be clear, when we talk about what we won't do ("bounce") we mean
> > that overscroll, once released, will just go back to the 0 position, rather
> > than go past it in the other direction, perhaps bouncing back and forth a
> > few times?
> 
> It's possible for spring-based physics to bounce back and forth around
> resting point, or not, depending on how you tune mass and spring stiffness.

Good.  No IP issues with doing that back and forth? What is the IP issue? :)
(In reply to Gordon Brander :gordonb from comment #7)
> (In reply to Milan Sreckovic [:milan, travelling, maybe slow to respond]
> from comment #5)
> > Also, how is this effect different from the "overscroll" effect when getting
> > to the end of the edge effect list and getting the stretch and snap (sorry,
> > don't know what we're calling all of these things - by the edge effect I
> > mean when we change between apps just dragging on the edge)
> 
> Great question. Sheets = the edge gesture to move between apps and app
> states. In theory, it would be elegant to tie the spacial model of sheets
> into overscroll effect, but I think we'll have to keep the 2 things separate
> because of technical constraints:

I meant from the "this is what it feels like to the user" :)  We have this elastic behavior when we get past the last screen, it let's me stretch it, it snaps back, doesn't bounce.  Ignoring completely the implementation details (Gecko/Gaia division, etc.) how is the feel/behavior of what we want for overscroll to be the same or different from what we have for the edge effect?
Attachment #8417665 - Attachment description: bug998025-part1.patch → Part 1 - Remove some unused methods in Axis and AsyncPanZoomController
Comment on attachment 8417665 [details] [diff] [review]
Part 1 - Remove some unused methods in Axis and AsyncPanZoomController

Drive by.
Attachment #8417665 - Flags: feedback+
This patch implements support in Axis for being in an overscrolled state, and support in AsyncPanZoomController/APZCTreeManager for taking this state into account while panning (including handoff situations).

Flings don't know about overscroll yet, and the snap-back animation needs to be put into place. Also, there is no rendering difference yet between an overscrolled state and a non-overscrolled state.
This patch is currently a bit rough around the edges. Physics-wise the snap-back starts at a fixed velocity and then accelerates as the animation advances; we are likely to want to change or at least fine-tune it. The parameters are hard-coded or piggy-back off of vaguely related prefs for now; once we figure out what the exact model is, we can give the parameters their own prefs.

Updated list of things that still need to be done:
  - Actual rendering effects
  - Resistance while panning or flinging in overscroll
  - Cancelling overscroll on touch events
Attachment #8421948 - Attachment description: Part 7 - Snap-back animation to relieve scroll → Part 7 - Snap-back animation to relieve overscroll
Added to Part 4 something that I overlooked: scheduling a composite (and if appropriate, requesting a repaint) while panning into overscroll.

I also added a work around to bug 1009825, which I ran into while testing the patches.
Attachment #8421340 - Attachment is obsolete: true
This patch allows visualizing the overscroll state that is maintained and manipulated by the other patches by translating the layer by the overscroll amount.

With the patches so far, you can now see overscrolling in action, though there is definitely polishing to do UX-wise.

Once we figure out what the actual overscroll effect that we want is, we can replace this patch with one that implements that.
This patch adds "resistance" to pans and flings in overscroll. The physics is subject to revision after feedback from UX; currently it simply tries to avoid allowing overscrolling by more that the composition length. (With the translation effect from Part 8, this amounts to avoiding scrolling the entire content out of view.)
Asking for feedback to make sure I have the right idea about ignoring events beyond APZ. In particular, I'm wondering if the call sites in TabParent and MetroInput handle the "ignoreEvent = true" case correctly.
Attachment #8422753 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8422753 [details] [diff] [review]
Part 10 - Ignore touch/mouse events when in an overscrolled state

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

Thinking about this and what I plan to do in bug 1009733 I would rather line up the code so that the nsEventStatus return values of everything actually make sense per the docs (see http://mxr.mozilla.org/mozilla-central/source/widget/EventForwards.h#18)

This means:
- APZCTreeManager::ReceiveInputEvent would return:
-- nsEventStatus_eConsumeNoDefault in cases where the touch point hits an APZC in overscroll
-- nsEventStatus_eConsumeDefault in cases where the touch point hits an APZC not in overscroll
-- nsEventStatus_eIgnore in cases where the touch point doesn't hit any APZC at all

This would remove the need to pass around the out-param in TabParent, RenderFrameParent and Metro* code. Currently APZCTreeManager just returns whatever the APZC instance returns, but we can disconnect that because none of the APZCTM callers do anything useful with that return value anyway. Instead, they can now check and drop the event if APZCTM returns eConsumeNoDefault.

Does that sound reasonable?
Attachment #8422753 - Flags: feedback?(bugmail.mozilla) → feedback-
Oh sorry, missed the Metro code. Let me read through that and get back to you.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #29)
> Currently APZCTreeManager just returns
> whatever the APZC instance returns, but we can disconnect that because none
> of the APZCTM callers do anything useful with that return value anyway.

There are two places in MetroInput where the return value from APZCTM::ReceiveInputEvent is used [1] [2]. However, these uses may be consistent with your interpretations - I don't think I understand the metro event handling code well enough to say.

On a different note: is the return value of APZC::ReceiveInputEvent used anywhere besides 
  - determining the return value of APZCTM::ReceiveInputEvent (the use we are proposing to remove)
  - in TestAsyncPanZoomController
If not, is there point in keeping that around for the sole benefit of test code?

> Does that sound reasonable?

Sounds good to me if there are no issues with the Metro call sites.

[1] http://dxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp?from=MetroInput.cpp#1309
[2] http://dxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp?from=MetroInput.cpp#1433
Looking at the call sites of ApzReceiveInputEvent in metro code I think the following would make sense with my suggested changes in comment 29:

- in HandleFirstTouchStartEvent
-- eConsumeNoDefault => just return early
-- eConsumeDefault or eIgnore => do what the code currently does

- in HandleFirstTouchMoveEvent
-- eConsumeNoDefault => just return early
-- eConsumeDefault => do what the code currently does for eConsumeNoDefault
-- eIgnore => do what the code currently does for !eConsumeNoDefault
(this effectively means adding an early return and replacing the eConsumeNoDefault check with eConsumeDefault instead)

- in DeliverNextQueuedTouchEvent: same as HandleFirstTouchMoveEvent

- in DispatchTouchCancel: no change; ignore return value.

As for the return value of APZC::ReceiveInputEvent, I'm happy to take it out but I'd rather do that after I deal with (or as part of) bug 1009733 in case I end up still needing it for something there.
Revised approach for ignoring touch/mouse events when in an overscrolled state.

In addition to the changes Kats suggested, I realized that we want to ignore not just touch-start events while in an overscrolled state, but all the events in the touch block, so I had to add a new piece of state to APZC which tracks whether the current touch block is in an overscrolled APZC.
Attachment #8422753 - Attachment is obsolete: true
Fixed a mistake in the Part 7 patch where I applied a CSS -> Screen scale in the wrong direction (as if it were a Screen -> CSS scale).

By the way, this mistake would have been caught if we had strongly-typed coordinate classes (bug 923512).
Attachment #8421948 - Attachment is obsolete: true
This is a prototype of the overscroll rendering effect requested by UX (translation + shrink).

Proper handling of background color still needs to be implemented, and the physics can definitely use some tuning, particularly in the case where you are overscrolling at a corner and have overlapping x- and y-overscrolls, but I think this is enough for UX to start playing around with this.

Next step is to put the overscrolling functionality behind a developer pref and land it so UX can play with it.
Attachment #8417724 - Attachment is obsolete: true
Attachment #8425894 - Flags: review?(bugmail.mozilla)
Attachment #8422037 - Attachment is obsolete: true
Attachment #8425897 - Flags: review?(bugmail.mozilla)
Attachment #8421342 - Attachment is obsolete: true
Attachment #8425899 - Flags: review?(bugmail.mozilla)
Attachment #8425785 - Attachment is obsolete: true
Attachment #8425902 - Flags: review?(bugmail.mozilla)
Attachment #8422054 - Attachment is obsolete: true
Attachment #8423285 - Attachment is obsolete: true
Attachment #8425905 - Flags: review?(bugmail.mozilla)
This doesn't actually work right now - setting the developer pref does not actually enable overscroll. I'm not sure why. I will debug it tomorrow and post it for review once I get it working, but I don't want to hold up the review of the other patches.
feature-b2g: --- → 2.0
Whiteboard: [ucid:graphics13]
1. Add user story ID defined in Firefox OS product backlog Google sheet
2. Specify this is Firefox OS 2.0 feature by feature-b2g=2.0
Attachment #8425891 - Flags: review?(bugmail.mozilla) → review+
Attachment #8425894 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8425895 [details] [diff] [review]
Part 3 - Refactor Axis::HasRoomToPan() and Axis::Scrollable()

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1562,5 @@
>  }
>  
>  bool AsyncPanZoomController::IsPannable() const {
>    ReentrantMonitorAutoEnter lock(mMonitor);
> +  return mX.CanScroll() || mY.CanScroll();

At some point we should make this code consistent with respect to using "pan" vs "scroll".
Attachment #8425895 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8425897 [details] [diff] [review]
Part 4 - Support overscrolling during panning

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1270,5 @@
>    }
>  
> +  return // If we consumed the entire displacement as a normal scroll, great.
> +         IsZero(overscroll)
> +

I'd rather break this up into separate if statements with early returns than this long return statement. It's creative with respect to formatting, but also "different and unexpected" which generally means "harder to read"

@@ +1297,5 @@
> +  if (yCanScroll) {
> +    mY.OverscrollBy(aOverscroll.y);
> +  }
> +  if (xCanScroll || yCanScroll) {
> +    ScheduleCompositeAndMaybeRepaint();

In general we shouldn't needing to do a repaint when we are just adjusting overscroll, since it's a compositor-side effect. However, it should also be the case that the dupe-filtering code in RequestContentRepaint should detect that and just early-return. Can you verify that is the case?

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +881,5 @@
> +   * If we are pannable, 'aOverscroll' is added to any existing overscroll,
> +   * and the function returns true.
> +   * Otherwise, nothing happens and the function return false.
> +   */
> +  bool OverscrollBy(const CSSPoint& aOverscroll);

If you're putting this function in the "scroll handoff" section of the .h file then I'd like to see the section comment updated to say "... building the scroll handoff chain and overscroll ..."

Or you could move this function into the uncategorized part of the .h file.

::: gfx/layers/apz/src/Axis.cpp
@@ +76,2 @@
>    // If this displacement will cause an overscroll, throttle it. Can potentially
>    // bring it to 0 even if the velocity is high.

While you're here, can you update this comment? The velocity is irrelevant here. Maybe just replace the entire comment with something like "split the remaining displacement into normal scroll and overscroll"

::: gfx/layers/apz/src/Axis.h
@@ +16,5 @@
>  namespace layers {
>  
>  const float EPSILON = 0.0001f;
>  
> +class FrameMetrics;

Forward declaration shouldn't be needed here. Maybe it belongs in part 2?

@@ +88,5 @@
> +
> +  /**
> +   * Return the amount of overscroll on this axis, in CSS pixels.
> +   */
> +  float GetOverscroll() const;

This function isn't used in this patch, but presumably it will be used later.
Attachment #8425897 - Flags: review?(bugmail.mozilla) → review+
CC'ing mstange as well as an FYI since some of these patches will require him to rebase the OS X work.
Comment on attachment 8425899 [details] [diff] [review]
Part 5 - Support overscrolling during flinging

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

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +279,5 @@
>     * hand off overscroll from a fling.
>     * @param aApzc the APZC that is handing off the fling
>     * @param aVelocity the current velocity of the fling, in |aApzc|'s screen
>     *                  pixels per millisecond
> +   * Returns whether another APZC accepted the handed-off fling. The caller

s/Returns whether/Returns true if/

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +356,5 @@
>  class FlingAnimation: public AsyncPanZoomAnimation {
>  public:
> +  FlingAnimation(AsyncPanZoomController& aApzc,
> +                 bool aApplyAcceleration,
> +                 bool aAllowOverscroll = false)

I would rather make this parameter non-optional, but I don't feel too strongly about it. In general I prefer non-optional parameters unless there are a lot of call sites to change.

@@ +1452,3 @@
>  
> +      // Restore the velocity of the fling, which was zeroed out by
> +      // AdjustDisplacement()p

dangling "p"

@@ +1461,5 @@
> +
> +      // We may have reached the end of the scroll range along one axis but
> +      // not the other. In such a case we only want to hand off the relevant
> +      // component of the fling.
> +      if (FuzzyEqualsMultiplicative(overscroll.x, 0.0f)) {

I suppose this fuzzyequals call will only return true if overscroll.x is exactly zero :( It should probably be a FuzzyEqualsAdditive, but we can deal with it after bug 1009825 is resolved.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +759,4 @@
>  
>    /* ===================================================================
>     * The functions and members in this section are used to manage
>     * fling animations.

... and fling overscroll. (I guess that's implied but I'd rather make it explicit)

@@ +765,5 @@
>    /**
>     * Take over a fling with the given velocity from another APZC. Used for
> +   * during overscroll handoff for a fling. If we are not pannable, calls
> +   * mTreeManager->HandOffFling() to hand the fling off further.
> +   * Returns whether any APZC (whether this one or one further in the handoff

s/Returns whether/Returns true if/

Looking at this code I think it should be possible to rearrange it to reduce stack depth. Right now the call stack looks like APZCTM::HandOffFling -> APZC::TakeOverFling -> APZCTM::HandOffFling -> ... -> APZC::TakeOverFling
Instead we could have a loop in APZCTM::HandOffFling that iterates through the handoff chain, calling TakeOverFling on each one and aborting if one of them returns true.

This could be done as a follow-up mentored bug or something, as it's just refactoring.

@@ +781,5 @@
> +  // Used by FlingAnimation::Sample() to handle the case where a fling gets
> +  // us to the end of our scroll range. Calls mTreeManager->HandOffFling()
> +  // to try to hand off the fling to an APZC later in the handoff chain. If
> +  // none want it, this APZC will continue the fling, entering an overscrolled
> +  // state.

This comment seems to just be reiterating the code. I think a slightly higher-level comment would be more appropriate. For example, "Deal with overscroll resulting from a fling animation. This is only ever called on APZC instances that were actually performaing a fling, but can pass the overscroll up the handoff chain if necessary."
Attachment #8425899 - Flags: review?(bugmail.mozilla) → review+
Attachment #8425901 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8425902 [details] [diff] [review]
Part 7 - Snap-back animation to relieve overscroll

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +251,5 @@
>   * The multiplier we apply to the displayport size if it is not skating (see
>   * documentation for the skate size multipliers above).
> + *
> + * "apz.overscroll.snap_back_accel"
> + * Amount of acceleration applied during the snap-back animation.

Move these up so they are sorted alphabetically by pref name. Also for this acceleration one update the comment to say that the velocity is multiplied by 1+accel every millisecond.

::: gfx/layers/apz/src/Axis.cpp
@@ +114,5 @@
> +
> +bool Axis::SampleSnapBack(const TimeDuration& aDelta) {
> +  // Accelerate the snap-back as time goes on.
> +  mVelocity *= pow(1.0f + gfxPrefs::APZSnapBackAcceleration(), float(aDelta.ToMilliseconds()));
> +  float screenDisplacement = mVelocity * aDelta.ToMilliseconds();

Technically this calculation doesn't result in a smooth curve because you're using assuming a fixed velocity over the delta when in fact it should be a slowly increasing velocity. I don't really care that much as long as it looks smooth but it might be worth a comment here in case somebody complains that the snapback doesn't look right.

@@ +117,5 @@
> +  mVelocity *= pow(1.0f + gfxPrefs::APZSnapBackAcceleration(), float(aDelta.ToMilliseconds()));
> +  float screenDisplacement = mVelocity * aDelta.ToMilliseconds();
> +  float cssDisplacement = screenDisplacement / GetFrameMetrics().GetZoom().scale;
> +  if (mOverscroll > 0) {
> +    mOverscroll = std::max(mOverscroll + cssDisplacement, 0.0f);

I'm a bit concerned that bugs in the code might result in situations where mVelocity ends up increasing the overscroll instead of decreasing it. It would be good to add a warning for that situation (i.e. if cssDisplacement > 0 here) so that we can diagnose it if it happens.

@@ +118,5 @@
> +  float screenDisplacement = mVelocity * aDelta.ToMilliseconds();
> +  float cssDisplacement = screenDisplacement / GetFrameMetrics().GetZoom().scale;
> +  if (mOverscroll > 0) {
> +    mOverscroll = std::max(mOverscroll + cssDisplacement, 0.0f);
> +    // Overscroll relieved, do not continue annimation.

s/annimation/animation/, here and below

::: gfx/thebes/gfxPrefs.h
@@ +130,5 @@
>    DECL_GFX_PREF(Live, "apz.x_stationary_size_multiplier",      APZXStationarySizeMultiplier, float, 3.0f);
>    DECL_GFX_PREF(Live, "apz.y_skate_size_multiplier",           APZYSkateSizeMultiplier, float, 2.5f);
>    DECL_GFX_PREF(Live, "apz.y_stationary_size_multiplier",      APZYStationarySizeMultiplier, float, 3.5f);
> +  DECL_GFX_PREF(Once, "apz.overscroll.snap_back_accel",        APZSnapBackAcceleration, float, 0.002f);
> +  DECL_GFX_PREF(Live, "apz.overscroll.snap_back_init_vel",     APZSnapBackInitialVelocity, float, 1.0f);

These also need to be moved up into sort order
Attachment #8425902 - Flags: review?(bugmail.mozilla) → review+
Attachment #8425904 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8425905 [details] [diff] [review]
Part 9 - Ignore touch/mouse events when in an overscrolled state

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

It seems to me that the changes in APZCTM would be cleaner if you got rid of the out-param and just queried the return apzc instance to see if it's in overscroll. You'd have to do that at every call site, but it could be refactored into a helper function that takes an APZC and returns the return value. Thoughts? Minusing for now (I didn't do a detailed review of the APZCTM changes but I can if you convince me this way is better).

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +418,5 @@
>          MultiTouchInput inputForApzc(multiTouchInput);
>          for (size_t i = 0; i < inputForApzc.mTouches.Length(); i++) {
>            ApplyTransform(&(inputForApzc.mTouches[i].mScreenPoint), transformToApzc);
>          }
>          result = mApzcForInputBlock->ReceiveInputEvent(inputForApzc);

Remove the assignment to result here

::: widget/windows/winrt/MetroInput.cpp
@@ +1286,5 @@
>    nsEventStatus apzcStatus = nsEventStatus_eIgnore;
>  
>    WidgetTouchEvent transformedEvent(*aEvent);
>    DUMP_TOUCH_IDS("APZC(2)", aEvent);
> +  apzcStatus = mWidget->ApzReceiveInputEvent(&transformedEvent, &mTargetAPZCGuid, &ignoreEvent);

&ignoreEvent?

@@ +1422,5 @@
>      return;
>    }
>  
>    DUMP_TOUCH_IDS("APZC(3)", event);
> +  status = mWidget->ApzReceiveInputEvent(event, nullptr, &ignoreEvent);

Ditto

@@ +1475,5 @@
>      return;
>    }
>    if (mContentConsumingTouch) {
>      DUMP_TOUCH_IDS("APZC(4)", &touchEvent);
> +    mWidget->ApzReceiveInputEvent(&touchEvent, nullptr, nullptr);

Extra nullptr
Attachment #8425905 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 8425907 [details] [diff] [review]
Part 10 - Overscroll rendering effect (translation + shrinking)

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

Some of this code will probably change but it's ok for now.
Attachment #8425907 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8425908 [details] [diff] [review]
[WIP] Part 11a - Put overscrolling behind a developer pref (gecko portion)

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

::: gfx/thebes/gfxPrefs.h
@@ +129,5 @@
>    DECL_GFX_PREF(Live, "apz.x_skate_size_multiplier",           APZXSkateSizeMultiplier, float, 1.5f);
>    DECL_GFX_PREF(Live, "apz.x_stationary_size_multiplier",      APZXStationarySizeMultiplier, float, 3.0f);
>    DECL_GFX_PREF(Live, "apz.y_skate_size_multiplier",           APZYSkateSizeMultiplier, float, 2.5f);
>    DECL_GFX_PREF(Live, "apz.y_stationary_size_multiplier",      APZYStationarySizeMultiplier, float, 3.5f);
> +  DECL_GFX_PREF(Live, "apz.overscroll.enabled",                APZOverscrollEnabled, bool, false);

Move this up, alphabetical
(In reply to Botond Ballo [:botond] from comment #46)
> Created attachment 8425908 [details] [diff] [review]
> [WIP] Part 11a - Put overscrolling behind a developer pref (gecko portion)
> 
> This doesn't actually work right now - setting the developer pref does not
> actually enable overscroll. I'm not sure why.

I figured out the problem: I had quotes around "false" in b2g/app/b2g.js. I guess that made it a string pref instead of a bool pref and did not play nicely with gfxPrefs trying to read it as a bool.
Doug, I'd like you to also take a look at these patches and familiarize yourself with how this works. Botond will be away for a good chunk of June and this code may need to be tuned and adjusted so having another person familiar with the code would help with reviews and load balancing. Also this might affect the stuff you're doing with the keyboard overlay.
Flags: needinfo?(drs+bugzilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #50)
> @@ +1297,5 @@
> > +  if (yCanScroll) {
> > +    mY.OverscrollBy(aOverscroll.y);
> > +  }
> > +  if (xCanScroll || yCanScroll) {
> > +    ScheduleCompositeAndMaybeRepaint();
> 
> In general we shouldn't needing to do a repaint when we are just adjusting
> overscroll, since it's a compositor-side effect. However, it should also be
> the case that the dupe-filtering code in RequestContentRepaint should detect
> that and just early-return. Can you verify that is the case?

Good point! RequestContentRepaint does indeed early-return, but it seems unnecessary to call it even so. What do you think about just calling ScheduleComposite() here?

> ::: gfx/layers/apz/src/Axis.h
> @@ +16,5 @@
> >  namespace layers {
> >  
> >  const float EPSILON = 0.0001f;
> >  
> > +class FrameMetrics;
> 
> Forward declaration shouldn't be needed here. Maybe it belongs in part 2?

Yep, thanks for catching it.

> @@ +88,5 @@
> > +
> > +  /**
> > +   * Return the amount of overscroll on this axis, in CSS pixels.
> > +   */
> > +  float GetOverscroll() const;
> 
> This function isn't used in this patch, but presumably it will be used later.

It is used in the "visual overscroll effect" patch, but it seemed strange to put it there, especially as I was at the time playing around with different versions of the effect. Really this Part 4 patch could have been separated into "add overscroll support to Axis" and "support overscrolling while panning" patches, and then this would have more clearly belonged to the "add overscroll support to Axis" patch.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #52)
> @@ +1461,5 @@
> > +
> > +      // We may have reached the end of the scroll range along one axis but
> > +      // not the other. In such a case we only want to hand off the relevant
> > +      // component of the fling.
> > +      if (FuzzyEqualsMultiplicative(overscroll.x, 0.0f)) {
> 
> I suppose this fuzzyequals call will only return true if overscroll.x is
> exactly zero :( It should probably be a FuzzyEqualsAdditive, but we can deal
> with it after bug 1009825 is resolved.

I'll change it to Additive with a 1e-3 additive as per the discussion in that bug.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #53)
> ::: gfx/layers/apz/src/Axis.cpp
> @@ +114,5 @@
> > +
> > +bool Axis::SampleSnapBack(const TimeDuration& aDelta) {
> > +  // Accelerate the snap-back as time goes on.
> > +  mVelocity *= pow(1.0f + gfxPrefs::APZSnapBackAcceleration(), float(aDelta.ToMilliseconds()));
> > +  float screenDisplacement = mVelocity * aDelta.ToMilliseconds();
> 
> Technically this calculation doesn't result in a smooth curve because you're
> using assuming a fixed velocity over the delta when in fact it should be a
> slowly increasing velocity. I don't really care that much as long as it
> looks smooth but it might be worth a comment here in case somebody complains
> that the snapback doesn't look right.

I modelled the calculation after how we apply friction to flings at http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.cpp?from=Axis.cpp#118. Does that also have the same issue of not being smooth, or are the two situations different in some way?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #54)
> ::: widget/windows/winrt/MetroInput.cpp
> @@ +1286,5 @@
> >    nsEventStatus apzcStatus = nsEventStatus_eIgnore;
> >  
> >    WidgetTouchEvent transformedEvent(*aEvent);
> >    DUMP_TOUCH_IDS("APZC(2)", aEvent);
> > +  apzcStatus = mWidget->ApzReceiveInputEvent(&transformedEvent, &mTargetAPZCGuid, &ignoreEvent);
> 
> &ignoreEvent?

Whoops, those are leftovers from an earlier approach where APZCTM::ReceiveInputEvent() would pass out a boolean "ignore this event" out-parameter (before we repurposed the return value for this purpose).
(In reply to Botond Ballo [:botond] from comment #59)
> Good point! RequestContentRepaint does indeed early-return, but it seems
> unnecessary to call it even so. What do you think about just calling
> ScheduleComposite() here?

Sounds good.

> > This function isn't used in this patch, but presumably it will be used later.
> 
> It is used in the "visual overscroll effect" patch, but it seemed strange to
> put it there, especially as I was at the time playing around with different
> versions of the effect. Really this Part 4 patch could have been separated
> into "add overscroll support to Axis" and "support overscrolling while
> panning" patches, and then this would have more clearly belonged to the "add
> overscroll support to Axis" patch.

Either way is fine. The patch queue was very well split up and it was pretty easy to review :)

> > Technically this calculation doesn't result in a smooth curve because you're
> > using assuming a fixed velocity over the delta when in fact it should be a
> > slowly increasing velocity. I don't really care that much as long as it
> > looks smooth but it might be worth a comment here in case somebody complains
> > that the snapback doesn't look right.
> 
> I modelled the calculation after how we apply friction to flings at
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.
> cpp?from=Axis.cpp#118. Does that also have the same issue of not being
> smooth, or are the two situations different in some way?

You're right, that code has the same issue. In both cases I guess it won't really be noticeable to the unaided eye.
Updated to address review comments. Carrying r+.
Attachment #8425894 - Attachment is obsolete: true
Attachment #8426593 - Flags: review+
Updated to address review comments. 

Also revised FuzzyEquals* usage as per bug 1009825 discussion. Re-requesting review for the introduction of COORDINATE_EPSILON.
Attachment #8425897 - Attachment is obsolete: true
Attachment #8426594 - Flags: review?(bugmail.mozilla)
Updated to address review comments. Carrying r+.
Attachment #8425899 - Attachment is obsolete: true
Attachment #8426595 - Flags: review+
Updated to address review comments. Carrying r+.
Attachment #8425902 - Attachment is obsolete: true
Attachment #8426597 - Flags: review+
As discussed with Kats on IRC, we are keeping out parameter in GetTargetAPZC() for now.

The exact behaviour of input events in an overscrolled state is subject to change after UX feedback.
Attachment #8425905 - Attachment is obsolete: true
Attachment #8426602 - Flags: review?(bugmail.mozilla)
Attachment #8425909 - Attachment description: [WIP] Part 11b - Put overscrolling behind a developer pref (gaia portion) → Part 11b - Put overscrolling behind a developer pref (gaia portion)
Attachment #8425909 - Flags: review?(bugmail.mozilla)
Attachment #8425909 - Flags: review?(21)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #49)
> Comment on attachment 8425895 [details] [diff] [review]
> Part 3 - Refactor Axis::HasRoomToPan() and Axis::Scrollable()
> 
> Review of attachment 8425895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +1562,5 @@
> >  }
> >  
> >  bool AsyncPanZoomController::IsPannable() const {
> >    ReentrantMonitorAutoEnter lock(mMonitor);
> > +  return mX.CanScroll() || mY.CanScroll();
> 
> At some point we should make this code consistent with respect to using
> "pan" vs "scroll".

Filed bug 1014276.
(In reply to Botond Ballo [:botond] from comment #35)
> Proper handling of background color still needs to be implemented

Filed bug 1014280 for this as a follow-up, as I intend to land the patches without this, to allow UX to play around with them sooner.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #52)
> Looking at this code I think it should be possible to rearrange it to reduce
> stack depth. Right now the call stack looks like APZCTM::HandOffFling ->
> APZC::TakeOverFling -> APZCTM::HandOffFling -> ... -> APZC::TakeOverFling
> Instead we could have a loop in APZCTM::HandOffFling that iterates through
> the handoff chain, calling TakeOverFling on each one and aborting if one of
> them returns true.
> 
> This could be done as a follow-up mentored bug or something, as it's just
> refactoring.

I'll file a mentored follow-up bug when this lands.
(In reply to Botond Ballo [:botond] from comment #67)
> Try pushes:
> 
> Emulator, build + tests: https://tbpl.mozilla.org/?tree=Try&rev=0654aa42aa0b
> Other platforms, build only:
> https://tbpl.mozilla.org/?tree=Try&rev=e9c82a6514b1

Had -Werror failures for not handling the SNAP_BACK state in OnTouchMove and OnTouchEnd. Fixed in this updated patch. 

Re-requesting review for these added bits. Technically, their correctness depends on code in the Part 9 patch. That possibly suggests that the part 9 patch should come before this one, but I don't know if we care enough.

ReTrying:

https://tbpl.mozilla.org/?tree=Try&rev=203b893bafbc
https://tbpl.mozilla.org/?tree=Try&rev=f562c7a81433
Attachment #8426597 - Attachment is obsolete: true
Attachment #8426672 - Flags: review?(bugmail.mozilla)
Attachment #8425909 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8426594 [details] [diff] [review]
Part 4 - Support overscrolling during panning

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

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +245,5 @@
>     *   represent different displacements in untransformed coordinates.
>     * |aOverscrollHandoffChainIndex| is the next position in the overscroll
>     *   handoff chain that should be scrolled.
>     *
> +   * Returns whether or not some APZC accepted the scroll and scrolled.

Returns true if

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +853,5 @@
>     * |aOverscrollHandoffChainIndex| is used by the tree manager to keep track
>     * of which APZC to hand off the overscroll to; this function increments it
>     * and passes it on to APZCTreeManager::DispatchScroll() in the event of
>     * overscroll.
> +   * Returns whether or not either this APZC, or an APZC further down the

Returns true if

::: gfx/layers/apz/src/Axis.h
@@ +19,5 @@
>  
> +// Epsilon to be used when comparing 'float' coordinate values
> +// with FuzzyEqualsAdditive. The rationale is that 'float' has 7 decimal
> +// digits of precision, and coordinate values are typically no larger than
> +// thousands.

I have seen coordinate values in the 10,000 order of magnitude (for example, in the contacts app with a medium workload). I'd feel more comfortable dropping this to 1e-2. Given that GetOrigin() et al return a CSS pixel length, and the smallest possible legitimate difference in page coordinates is 1 app unit (~0.02 CSS pixels) we should be safe with a 0.01 fuzz.
Attachment #8426594 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8426672 [details] [diff] [review]
Part 7 - Snap-back animation to relieve overscroll

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

Wrong patch
Attachment #8426672 - Flags: review?(bugmail.mozilla) → review-
Attachment #8426603 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8426602 [details] [diff] [review]
Part 9 - Ignore touch/mouse events when in an overscrolled state

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +612,2 @@
>    }
> +  return inOverscrolledApzc ? nsEventStatus_eConsumeNoDefault : nsEventStatus_eIgnore;

I think for consistency we should return nsEventStatus_eConsumeDoDefault in the case where !inOverscrolledApzc && apzc != nullptr. (i.e. basically using the same nested ternary expression you used everywhere else)
Attachment #8426602 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #73)
> Wrong patch

Whoops! Here is the correct patch. The thing I'm requesting re-review for is the added cases in APZC::OnTouchMove() and OnTouchEnd().
Attachment #8426672 - Attachment is obsolete: true
Attachment #8427094 - Flags: review?(bugmail.mozilla)
Updated to address review comments. Carrying r+.
Attachment #8426594 - Attachment is obsolete: true
Attachment #8427106 - Flags: review+
Updated to address review comments. Carrying r+.
Attachment #8426602 - Attachment is obsolete: true
Attachment #8427108 - Flags: review+
Comment on attachment 8427094 [details] [diff] [review]
Part 7 - Snap-back animation to relieve overscroll

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

LGTM
Attachment #8427094 - Flags: review?(bugmail.mozilla) → review+
(In reply to Botond Ballo [:botond] from comment #71)
> ReTrying:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=203b893bafbc
> https://tbpl.mozilla.org/?tree=Try&rev=f562c7a81433

The Try builds had some gtest failures. Investigating them led me to discover three bugs:

 1) The tasks in AsyncPanZoomAnimation::mDeferredTasks were neither posted
    to a message loop (which destroys a Task after it runs it), nor
    explicitly destroyed, and as a result, they - along with anything they
    hold a strong reference to - were being leaked.

    This bug has been around since mDeferredTasks was introduced in
    bug 965871, but it didn't cause any failures until the patches in this
    added additional uses of mDeferredTasks.

    The fix is to delete the tasks after running them in
    SampleContentTransformForFrame(). I'll amend the Part 6 patch to include
    this fix.

    Kudos to gtests for catching this with a "mock object leaked" error!

 2) My modifications to APZCTreeManager::GetAPZCAtPoint() in the Part 9 patch
    had a bug where I changed a 'return' from a loop into an assignment to a 
    return variable, but forgot to add a 'break'. This was causing
    AsyncPanZoomControllerTest.HitTesting1 to fail.
    I'll amend the Part 9 patch to fix this.

 3) ApzcPan() in TestAsyncPanZoomController.cpp was explicitly building the
    overscroll handoff chain, but failing to explicitly clear it. While this
    did not manifest in failures, it has the potential to cause a leak, so
    I will fix it in a Part 12 patch.
Updated patch to delete the deferred tasks after running them, fixing a gtest failure. Carrying r+.
Attachment #8425901 - Attachment is obsolete: true
Attachment #8427330 - Flags: review+
Updated patch to fix a bug it introduced in GetAPZCAtPoint() which was causing AsyncPanZoomControllerTester.HitTesting1 to fail. Carrying r+.
Attachment #8427108 - Attachment is obsolete: true
Attachment #8427333 - Flags: review+
(In reply to Botond Ballo [:botond] from comment #79)
>     The fix is to delete the tasks after running them in
>     SampleContentTransformForFrame(). I'll amend the Part 6 patch to include
>     this fix.

It would be better to file a separate bug and patch for this and land it first, because it might need uplifting. If I understand correctly, the code in 1.4 is going to leak a Task every time we try to hand off a fling, which is going to be pretty frequent.
Attachment #8427335 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #84)
> (In reply to Botond Ballo [:botond] from comment #79)
> >     The fix is to delete the tasks after running them in
> >     SampleContentTransformForFrame(). I'll amend the Part 6 patch to include
> >     this fix.
> 
> It would be better to file a separate bug and patch for this and land it
> first, because it might need uplifting. If I understand correctly, the code
> in 1.4 is going to leak a Task every time we try to hand off a fling, which
> is going to be pretty frequent.

Good point! I filed bug 1015331.
Comment on attachment 8425909 [details] [diff] [review]
Part 11b - Put overscrolling behind a developer pref (gaia portion)

Flagging some other Gaia::System peers for review of developer pref.
Attachment #8425909 - Flags: review?(etienne)
Attachment #8425909 - Flags: review?(alive)
Comment on attachment 8425909 [details] [diff] [review]
Part 11b - Put overscrolling behind a developer pref (gaia portion)

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

r+ if you had the settings into build/config/common-settings.js too.
Attachment #8425909 - Flags: review?(21) → review+
Attachment #8425909 - Flags: review?(etienne)
Attachment #8425909 - Flags: review?(alive)
Updated to address review comments. Carrying r+.
Attachment #8425909 - Attachment is obsolete: true
Attachment #8428839 - Flags: review+
No longer depends on: 1009825
Okay, I read through this for a couple of hours and I think I mostly get it now.
Flags: needinfo?(drs+bugzilla)
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
Depends on: 1025507
Depends on: 1041471
(In reply to Botond Ballo [:botond] from comment #70)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #52)
> > Looking at this code I think it should be possible to rearrange it to reduce
> > stack depth. Right now the call stack looks like APZCTM::HandOffFling ->
> > APZC::TakeOverFling -> APZCTM::HandOffFling -> ... -> APZC::TakeOverFling
> > Instead we could have a loop in APZCTM::HandOffFling that iterates through
> > the handoff chain, calling TakeOverFling on each one and aborting if one of
> > them returns true.
> > 
> > This could be done as a follow-up mentored bug or something, as it's just
> > refactoring.
> 
> I'll file a mentored follow-up bug when this lands.

Filed bug 1056367. Sorry for taking so long.
You need to log in before you can comment on or make changes to this bug.