701594 - Unify the "bouncing back" overscroll animation and the "zoom" animation when zoomed out too far

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Wesley Johnston (:wesj)]

birch-pan-zoom should not let you zoom our further than the page width.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

When the page renders at the correct size, this feels and looks good to me, but it builds on my patch in Bug 697701.

I get the feeling there's more in panZoomController to do this animation stuff that I'm ignoring at the moment.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

This just animates us back to the page size after a pinchzoom ends.

Comment 3

[Patrick Walton (:pcwalton)]

Comment on attachment 573852 [details] [diff] [review]
Patch

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

r+ with changes

::: embedding/android/ui/PanZoomController.java
@@ +560,5 @@
>          mY.touchPos = detector.getFocusY();
> +
> +        FloatRect visible = mController.getVisibleRect();
> +        IntSize pageSize = mController.getPageSize();
> +        FloatRect pageRect = new FloatRect(0,0, pageSize.width, pageSize.height);

Needs to be RectF.

@@ +563,5 @@
> +        IntSize pageSize = mController.getPageSize();
> +        FloatRect pageRect = new FloatRect(0,0, pageSize.width, pageSize.height);
> +        if (!pageRect.contains(visible)) {
> +            FloatRect rect = mController.clampToScreenSize(visible);
> +            mController.animatedZoomTo(rect.x, rect.y, rect.width, rect.height, 200);

animatedZoomTo() should be in the PanZoomController.

Comment 4

[Patrick Walton (:pcwalton)]

Forget to mention: I think we'll probably want to fold this logic into the "snapping" logic that used when you overscroll somehow, but we should land any solution we can, because this is a showstopper bug.

Comment 5

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/projects/birch/rev/6b1414e0a6e8

Comment 6

[Wesley Johnston (:wesj)]

Need to test this a bit more. Backed out.

Comment 9

[Cristian Nicolae (:xti)]

Is this bug related for zooming in as well? Or should I file a new bug?

Comment 10

[Aaron Train [:aaronmt]]

(In reply to Cristian Nicolae (:xti) from comment #9)
> Is this bug related for zooming in as well? Or should I file a new bug?

This bug is what is written in the summary: "Should not be able to stay zoomed out further than the pages width".

Comment 11

[Steffen Wilberg]

Please update the patch. Besides the move of the file, the lines in the context don't exist anymore...

Comment 12

[Patrick Walton (:pcwalton)]

Mind if I take this?

I think what needs to be done is to tie this to the snapping animation. Take the ease-in animation curve that's plotted for the snap animation and use it to zoom back in when overscroll is BOTH.

Comment 14

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

QA verifiers : Please test bug Bug 706694 if this gets resolved.  that bug involves panning not zooming.

Comment 15

[Patrick Walton (:pcwalton)]

Attachment: WIP patch.

This patch significantly redoes how "snapping" or "bouncing" works. Instead of being an action that is triggered on a per-axis basis, it's now a global action that determines a suitable set of "valid" viewport metrics and animates to that viewport. The end result is that one code path handles all kinds of invalid viewport correction (bouncing behavior on the edges, zooming out too far from the page, or some combination of these). This action runs after every fling.

Along the way, I refactored the axes to avoid duplicating layer controller data in them. I also hard-coded the ease-out animation frames, for speed and simplicity. Finally, I fixed the edge resistance, which I believe regressed with bug 705114.

The end result is a browser that's much more usable, IMO.

I can't land this patch as is, because it makes the first page the user visits after Fennec Start unviewable, for some reason. However, it's somewhat sizable, so I figured I'd get feedback early.

Comment 16

[Chris Lord [:cwiiis]]

Comment on attachment 578499 [details] [diff] [review]
WIP patch.

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

I like this new approach, much simpler... Some issues below I've noted.

::: mobile/android/base/FloatUtils.java
@@ +50,5 @@
> +     * returns `to`; with t = 0.5f, this returns the value halfway from `from` to `to`.
> +     */
> +    public static float interpolate(float from, float to, float t) {
> +        return from + (to - from) * t;
> +    }

Not sure I like something as simple as this being a function, but I see that you've used it a lot and it does make things neater... Your call.

::: mobile/android/base/gfx/LayerController.java
@@ +211,5 @@
>  
> +        if (notifyLayerClient)
> +            notifyLayerClientOfGeometryChange();
> +        else
> +            mPanZoomController.geometryChanged(true);

This either/or situation isn't really an obvious side-effect given the parameter name. Maybe notification should just be split away from these viewport-changing conveniences, and that notification function should take a flags argument to decide what to notify? That would be good to reduce the amount of calls to repositionPluginViews too, which can be expensive.

::: mobile/android/base/gfx/ViewportMetrics.java
@@ +226,5 @@
> +        ViewportMetrics result = new ViewportMetrics();
> +        result.mPageSize = mPageSize.interpolate(to.mPageSize, t);
> +        result.mViewportRect = RectUtils.interpolate(mViewportRect, to.mViewportRect, t);
> +        result.mViewportOffset = PointUtils.interpolate(mViewportOffset, to.mViewportOffset, t);
> +        result.mZoomFactor = FloatUtils.interpolate(mZoomFactor, to.mZoomFactor, t);

The interpolation of the viewport rect isn't correct here, with respect to its origin. The same can probably be said about the viewport offset, although I'm not sure we want to be interpolating the viewport offset at all. I pointed out the same problem in a review of bug #697701, so I'll copy-paste:

"Doing a linear interpolation of the position will probably not yield the animation we want - the transformation is a little more complex than that. Really, you want to do a linear interpolation of the unscaled values, then re-scale them.

This would look something like;

PointF currentPoint = PointUtils.interpolate(finalPoint / finalZoom, startPoint / startZoom, dt);
PointUtils.scale(currentPoint, currentScale);"

::: mobile/android/base/ui/PanZoomController.java
@@ +433,5 @@
>      }
>  
> +    /* Instructs the controller to move the origin to the new origin. */
> +    private void updateOrigin() {
> +        mController.scrollTo(mNewOrigin);

Is there a point in having this function and not just calling scrollTo?

Comment 17

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

Comment on attachment 578499 [details] [diff] [review]
WIP patch.

>+        populateOrigin();
>+        mX.displaceTrack(); mY.displaceTrack();
>+        updateOrigin();

This seems clunky to me. Can we do this with a single call to getLayerController().scrollBy(...) instead? In generally I definitely like keeping less viewport information in PanZoomController. The rest of the patch seems pretty good, although I didn't go over it in too much detail.

Comment 18

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Proposed patch attached.

Comment 19

[Patrick Walton (:pcwalton)]

(In reply to Chris Lord [:cwiiis] from comment #16)
> This either/or situation isn't really an obvious side-effect given the
> parameter name. Maybe notification should just be split away from these
> viewport-changing conveniences, and that notification function should take a
> flags argument to decide what to notify? That would be good to reduce the
> amount of calls to repositionPluginViews too, which can be expensive.

I changed it so that there are two notification functions, one for the layer client and one for the pan-zoom controller, and the caller decides which one to call.

> The interpolation of the viewport rect isn't correct here, with respect to
> its origin. The same can probably be said about the viewport offset,
> although I'm not sure we want to be interpolating the viewport offset at
> all. I pointed out the same problem in a review of bug #697701, so I'll
> copy-paste:
> 
> "Doing a linear interpolation of the position will probably not yield the
> animation we want - the transformation is a little more complex than that.
> Really, you want to do a linear interpolation of the unscaled values, then
> re-scale them.
> 
> This would look something like;
> 
> PointF currentPoint = PointUtils.interpolate(finalPoint / finalZoom,
> startPoint / startZoom, dt);
> PointUtils.scale(currentPoint, currentScale);"

I tried that and it didn't seem to work -- the origin would jump around during the animation. Maybe I was doing something wrong. Here's the code I used:

/* Unscale; interpolate; scale up. */
RectF fromRectUnscaled = RectUtils.scale(mViewportRect, 1.0f / mZoomFactor);
RectF toRectUnscaled = RectUtils.scale(to.mViewportRect, 1.0f / to.mZoomFactor);
RectF resultRectUnscaled = RectUtils.interpolate(fromRectUnscaled, toRectUnscaled, t);
result.mViewportRect = RectUtils.scale(resultRectUnscaled, result.mZoomFactor);

/* Ditto. */
PointF fromOffUnscaled = PointUtils.scale(mViewportOffset, 1.0f / mZoomFactor);
PointF toOffUnscaled = PointUtils.scale(to.mViewportOffset, 1.0f / to.mZoomFactor);
PointF resultOffUnscaled = PointUtils.interpolate(fromOffUnscaled, toOffUnscaled, t);
result.mViewportOffset = PointUtils.scale(resultOffUnscaled, result.mZoomFactor);

> Is there a point in having this function and not just calling scrollTo?

Changed to just scrollTo.

> This seems clunky to me. Can we do this with a single call to
> getLayerController().scrollBy(...) instead? In generally I definitely like
> keeping less viewport information in PanZoomController. The rest of the
> patch seems pretty good, although I didn't go over it in too much detail.

Done.

Comment 20

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

Comment on attachment 578741 [details] [diff] [review]
Proposed patch.

Overall, I like this a lot as it makes things much cleaner and simpler than they were before. But r- because I believe it introduces a regression by not aborting bounce/fling animations when Gecko updates the scroll position. If you address that and the few other comments below then I'll happily r+ it :)


>diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
>     public void setLayerController(LayerController layerController) {
>         super.setLayerController(layerController);
> 
>         layerController.setRoot(mTileLayer);
>-        if (mGeckoViewport != null)
>+        if (mGeckoViewport != null) {
>             layerController.setViewportMetrics(mGeckoViewport);
>+            layerController.notifyPanZoomControllerOfGeometryChange(false);
>+        }
>+

I was actually wondering about this code - is it ever even executed? I'm not sure if mGeckoViewport is ever assigned at this point, unless the point of this is to robustify against future changes where the controller might be unset and reset or something. It's more of a general question, not specific to this patch though.

>                         if (onlyUpdatePageSize) {
> [...]
>                         } else {
>                             controller.setViewportMetrics(mGeckoViewport);
>+                            controller.notifyPanZoomControllerOfGeometryChange(false);

Why is this false instead of true? Don't we want to abort the fling/bounce in cases where the viewport is updated by Gecko? The specific scenario I was running into was as follows: the PanZoomController is performing some sort of animation, like a fling or snap-back (or with this patch, a fling or bounce). While it's doing this, it pre-computes the coordinates at which the viewport should be at (with this patch, this is effectively still happening because you cache mBounce(Start|End)Metrics and interpolate using those). While the animation is happening, Gecko changes the scroll coordinates, and it propagates up through this code. It changes the viewport in the controller, but then on the next animation frame that gets clobbered by the PanZoomController with the pre-computed animation value.

The specific test this was causing me grief was on http://people.mozilla.org/~nhirata/html_tp/jumptag.html when you click on the link in such a way that you also push the page into overscroll or do a fling. When that happens, both the PanZoomController animation and Gecko's jump-to-anchor code try to set the viewport, and whoever does it last (usually PanZoomController) wins. And so it looks like the anchor jump never happens.

I think changing the argument here to true, and also updating PanZoomController#geometryChanged to abort bounce animations would prevent this regression from coming back.

>diff --git a/mobile/android/base/ui/PanZoomController.java
>+    /* The new origin we're moving to. */
>+    private PointF mNewOrigin;

This is never used in this revision of the patch (yay!) and can be removed.

>+        mX.setFlingState(FlingState.PANNING); mY.setFlingState(FlingState.PANNING);
>+
>+        mController.scrollTo(new PointF(mX.displaceTrack(), mY.displaceTrack()));

This would be cleaner with a call to scrollBy instead of scrollTo - see my other comment on displaceTrack() below.

>     private void fling() {
>         if (mState != PanZoomState.FLING)
>             mX.velocity = mY.velocity = 0.0f;
> 
>-        mX.displace(); mY.displace();
>-        updatePosition();
>+        mController.scrollTo(new PointF(mX.displaceFling(), mY.displaceFling()));

This would be cleaner with a call to scrollBy instead of scrollTo - see my other comments below.

>+            /* If we're still flinging, update the origin and finish here. */
>+            if (xFlinging || yFlinging) {
>+                mController.scrollTo(new PointF(mX.displaceFling(), mY.displaceFling()));

This would be cleaner with a call to scrollBy instead of scrollTo - see my other comments below.

>+    /* Returns the nearest viewport metrics with no overscroll visible. */
>+    private ViewportMetrics getValidViewportMetrics() {
> [...]
>+        if (minZoomFactor != 0.0f) {

I'd feel more comfortable if this used FloatUtils.fuzzyEquals, or had a comment explaining why it's not needed in this instance.

>+            PointF center = new PointF(viewport.width() / 2.0f, viewport.height() / 2.0f);

This had me confused until I looked at ViewportMetrics.scaleTo and realized the focus point is relative to the viewport origin, not to the page origin. But not a problem with your patch.

>+        public boolean stopped() { return FloatUtils.fuzzyEquals(velocity, 0.0f); }

This function is never called and can be removed.

>-        private void scroll() {
>-            // If we aren't overscrolled, just apply friction.
>+            /* If we aren't overscrolled, just apply friction. */
>             float excess = getExcess();
>             if (FloatUtils.fuzzyEquals(excess, 0.0f)) {
>                 velocity *= FRICTION;
>-                if (Math.abs(velocity) < 0.1f) {
>+                if (Math.abs(velocity) < STOPPED_THRESHOLD) {

STOPPED_THRESHOLD is 4.0f, is this change intentional?

>+            velocity = Math.copySign(magnitude, velocity);
> 
>-            if (Math.abs(velocity) < 0.3f) {
>+            if (magnitude < STOPPED_THRESHOLD) {

This one should be STOPPED_THRESHOLD_OVERSCROLL, not STOPPED_THRESHOLD. I think. Otherwise STOPPED_THRESHOLD_OVERSCROLL is never used.

>+        public float displaceTrack() {
>+            float origin = getOrigin();
>+            return locked ? origin : (origin + applyEdgeResistance(lastTouchPos - touchPos));
>         }

Here you can return the actual displacement (locked ? 0 : applyEdgeResistance(lastTouchPos - touchPos)) instead of the new position, and call mController.scrollBy instead of mController.scrollTo. It feels like that would be cleaner and easier to understand, since using scrollBy is more state-free than using scrollTo.

>+        public float displaceFling() {
>+            float origin = getOrigin();
>+            return locked ? origin : (origin + velocity);

Ditto - you can return "locked ? 0 : velocity" here.

Comment 21

[Chris Lord [:cwiiis]]

This has actually been fixed by bug #697701 now - do we want to rename this bug to reflect the other issues it fixes?

Comment 22

[Aaron Train [:aaronmt]]

fwiw, bug 706146 was filed for limitations on zooming inwards

Comment 23

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

Patch version 2 addresses review comments.

Comment 24

[Patrick Walton (:pcwalton)]

(In reply to Kartikaya Gupta (:kats) from comment #20)
> I was actually wondering about this code - is it ever even executed? I'm not
> sure if mGeckoViewport is ever assigned at this point, unless the point of
> this is to robustify against future changes where the controller might be
> unset and reset or something. It's more of a general question, not specific
> to this patch though.

Removed.

> I think changing the argument here to true, and also updating
> PanZoomController#geometryChanged to abort bounce animations would prevent
> this regression from coming back.

Done.

> 
> >diff --git a/mobile/android/base/ui/PanZoomController.java
> >+    /* The new origin we're moving to. */
> >+    private PointF mNewOrigin;
> 
> This is never used in this revision of the patch (yay!) and can be removed.

Done.

> 
> >+        mX.setFlingState(FlingState.PANNING); mY.setFlingState(FlingState.PANNING);
> >+
> >+        mController.scrollTo(new PointF(mX.displaceTrack(), mY.displaceTrack()));
> 
> This would be cleaner with a call to scrollBy instead of scrollTo - see my
> other comment on displaceTrack() below.

Done (along with all the others).

> STOPPED_THRESHOLD is 4.0f, is this change intentional?

Yeah, I felt it would be more consistent to have one stopped threshold.

> I'd feel more comfortable if this used FloatUtils.fuzzyEquals, or had a
> comment explaining why it's not needed in this instance.

Uses fuzzyEquals now.

> This function is never called and can be removed.

Done.

> This one should be STOPPED_THRESHOLD_OVERSCROLL, not STOPPED_THRESHOLD. I
> think. Otherwise STOPPED_THRESHOLD_OVERSCROLL is never used.

Should be STOPPED_THRESHOLD. Removed STOPPED_THRESHOLD_OVERSCROLL.

Comment 25

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

Comment on attachment 579150 [details] [diff] [review]
Proposed patch, version 2.

Looks good.

Comment 26

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 3.

The patches that have landed on birch in the meantime have caused the patches in this bug to bitrot massively. Redoing them. Here's part 1, which contains the ease-out animation reform.

Comment 27

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 3.

Patch part 2 version 3 adds the code that avoids maintaining parallel state in the axes that's already in the layer controller.

Comment 28

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 3.

Patch part 3 adds the viewport interpolation functions.

Comment 29

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 3.

Patch part 4 version 3 reforms LayerController.setViewportMetrics() to not automatically assume it was called by the layer client and updates callers accordingly.

Comment 30

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 5, version 3.

Patch part 5 adds back the code that uses valid viewport metrics to handle bouncing back after overscroll.

Comment 31

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 4.

Patch part 1, version 4 removes the now-useless constant that specifies the number of subdivisions to compute.

Comment 32

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 5, version 4.

Patch part 5, version 4 factors out the fling timer setting functions into startFlingTimer() and stopFlingTimer(), in preparation for moving bounce to a separate class.

Comment 33

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 6, version 3.

Patch part 6 separates out the fling and bounce animations and makes them use the valid viewport metrics.

Comment 34

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 7, version 3.

Patch part 7, version 3 removes the old per-axis bounce/snap stuff.

Comment 35

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 6, version 4.

Patch part 6 version 4 makes the bounce animation not depend on overscroll.

Comment 36

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 8, version 3.

Patch part 8, version 3 refactors the animatedZoomTo() function to use the new bounce animation internally.

At the end of this patch series, there is only one way to correct viewports or zoom from one viewport to another: the bounce animation. It uses the ease-out animation and viewport interpolation to do its work.

Comment 37

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

Comment on attachment 579550 [details] [diff] [review]
Proposed patch, part 4, version 3.

>diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
>@@ -161,16 +164,17 @@ public class GeckoSoftwareLayerClient ex
>                     public void run() {
>                         if (onlyUpdatePageSize) {
>                             // Don't adjust page size when zooming unless zoom levels are
>                             // approximately equal.
>                             if (FloatUtils.fuzzyEquals(controller.getZoomFactor(), mGeckoViewport.getZoomFactor()))
>                                 controller.setPageSize(mGeckoViewport.getPageSize());
>                         } else {
>                             controller.setViewportMetrics(mGeckoViewport);
>+                            controller.notifyPanZoomControllerOfGeometryChange(false);
>                         }

s/false/true/ as per previous discussion, unless you have an argument against. I realize this causes some unnecessary jumping around but that issue can be tracked with bug 707869 and bug 707965.

r+ with above change.

Comment 38

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

Comment on attachment 579582 [details] [diff] [review]
Proposed patch, part 5, version 4.

>diff --git a/mobile/android/base/ui/PanZoomController.java b/mobile/android/base/ui/PanZoomController.java
> 
>+    /* The timer that handles flings or bounces. */
>     private Timer mFlingTimer;

Might make sense to rename this to mAnimationTimer. Same for the other hunks in this patch - startAnimationTimer and stopAnimationTimer make more sense. I'll also be renaming aAbortFling to abortAnimation when I land a fixed up version of the patch in bug 704738.

Comment 39

[Patrick Walton (:pcwalton)]

(In reply to Kartikaya Gupta (:kats) from comment #37)
> s/false/true/ as per previous discussion, unless you have an argument
> against. I realize this causes some unnecessary jumping around but that
> issue can be tracked with bug 707869 and bug 707965.

Oops, just an oversight. No arguments against this change.

Comment 40

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

Comment on attachment 579591 [details] [diff] [review]
Proposed patch, part 6, version 4.

I'm a little concerned that this change will break opening new tabs. I'm not sure why though, but that regression appeared with my patch on bug 704738, and this patch seems to involve similar behaviour. It would be good if you could test opening new tabs before landing this - just start Fennec, let about:home load, and then open a new tab with some site and make sure it renders ok.

Comment 41

[Patrick Walton (:pcwalton)]

Landed on inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ffd93ca68fd7
http://hg.mozilla.org/integration/mozilla-inbound/rev/953247b411fa
http://hg.mozilla.org/integration/mozilla-inbound/rev/fe9f200f7d28
http://hg.mozilla.org/integration/mozilla-inbound/rev/31b6da96f5e0
http://hg.mozilla.org/integration/mozilla-inbound/rev/e5b56fcb36af
http://hg.mozilla.org/integration/mozilla-inbound/rev/d1466b26c891
http://hg.mozilla.org/integration/mozilla-inbound/rev/1a233691d283
http://hg.mozilla.org/integration/mozilla-inbound/rev/3366b6a387e6

Comment 42

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/ffd93ca68fd7
http://hg.mozilla.org/mozilla-central/rev/953247b411fa
http://hg.mozilla.org/mozilla-central/rev/fe9f200f7d28
http://hg.mozilla.org/mozilla-central/rev/31b6da96f5e0
http://hg.mozilla.org/mozilla-central/rev/e5b56fcb36af
http://hg.mozilla.org/mozilla-central/rev/d1466b26c891
http://hg.mozilla.org/mozilla-central/rev/1a233691d283
http://hg.mozilla.org/mozilla-central/rev/3366b6a387e6

Comment 43

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-02-17)
20120217031227
http://hg.mozilla.org/mozilla-central/rev/2271cb92cc05

--
Device: Motorola Droid PRO
OS: Android 2.3.3

Comment 44

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 12.0a2 (2012-02-16)
20120216042010
http://hg.mozilla.org/releases/mozilla-aurora/rev/c6fcc091279c

Firefox 11.0 (2012-02-15)
20120215185359
http://hg.mozilla.org/releases/mozilla-beta/rev/f21c6aa0f8c2

--
Device: Motorola Droid PRO
OS: Android 2.3.3