697701 - Support double-tap gesture to zoom

Status: VERIFIED FIXED

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

Description

[Aaron Train [:aaronmt]]

Support double-tap gesture to zoom inwards and outwards.

Comment 1

[Wesley Johnston (:wesj)]

I think we should actively talk about whether or not we want to support this. We take a hit in "tap detecting performance" to implement it, and gain a feature that only makes sense to users who have been trained to expect it.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I think Wes can take this one

Comment 4

[Alex Limi (:limi) — Firefox UX Team]

This is one of the most common UI patterns on touch devices, we can't seriously consider not supporting this. :)

Comment 5

[Alex Limi (:limi) — Firefox UX Team]

Also, while we're discussing it — there's a subtle way it is broken even in current XUL Fennec:

1. Go to a web site, zoomed out
2. Double tap to zoom in on a paragraph
3. Double tap on a different part of the page

Expected result:
4. The new region selected gets brought into the viewport at an appropriate zoomed-in level

Actual result:
4. We zoom out instead

…so if we're re-implementing this for Native Fennec, we should attempt to fix that in the process.

Let me know if my explanation makes no sense, possibly easier to show than to explain. :)

Comment 6

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

WIP. This mostly works, but is based on a slightly modified version of the birch-pan-zoom patch queue. I'll rebase it when that lands (soon?). And then we can review I guess.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Patch?

Comment 8

[Patrick Walton (:pcwalton)]

Comment on attachment 573851 [details] [diff] [review]
Patch v1

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

Also, could animatedZoomTo() maybe go into the PanZoomController?

r+ with changes.

::: embedding/android/GeckoApp.java
@@ +47,4 @@
>  import org.mozilla.gecko.gfx.LayerView;
>  import org.mozilla.gecko.gfx.PlaceholderLayerClient;
>  import org.mozilla.gecko.Tab.HistoryEntry;
> +import org.mozilla.gecko.gfx.FloatRect;

This will need to be android.graphics.RectF soon.

@@ +768,5 @@
>                      }
>                  });
> +            } else if (event.equals("Browser:ZoomToRect")) {
> +                if (mLayerController != null) {
> +                    float x = (float)message.getInt("x");

Why not getFloat()?

@@ +775,5 @@
> +                    float height = (float)message.getInt("h");
> +                    mLayerController.animatedZoomTo(x, y, width, height, 200);
> +                }
> +            } else if (event.equals("Browser:ZoomToPageWidth")) {
> +                Log.i(LOG_NAME, "Got zoomtopagewidth");

Should be removed.

::: embedding/android/gfx/FloatRect.java
@@ +94,5 @@
>      public FloatRect scaleAll(float factor) {
>          return new FloatRect(x * factor, y * factor, width * factor, height * factor);
>      }
> +
> +    public FloatRect blend(FloatRect aRect, float aBlendAmount) {

Will need to be moved to RectUtils.

::: embedding/android/gfx/LayerController.java
@@ +248,5 @@
> +        // if the requested rect would not fill the screen, shift it to be centered
> +        if (height < newHeight) {
> +            y -= (newHeight - height)/2;
> +        }
> +        final FloatRect finalRect = clampToScreenSize(new FloatRect(x,y,width,newHeight));

nit: new FloatRect(x, y, width, newHeight)

::: embedding/android/ui/PanZoomController.java
@@ +579,5 @@
> +    @Override
> +    public boolean onDoubleTap(MotionEvent motionEvent) {
> +        JSONObject ret = new JSONObject();
> +        try {
> +            FloatPoint point = new FloatPoint(motionEvent.getX(), motionEvent.getY());

Will need to be android.graphics.PointF.

@@ +584,5 @@
> +            point = mController.convertViewPointToLayerPoint(point);
> +            ret.put("x", (int)Math.round(point.x));
> +            ret.put("y", (int)Math.round(point.y));
> +        } catch(Exception ex) {
> +            Log.w(LOG_NAME, "Error building return: " + ex);

This might mask errors. I usually use:

try {
   ...
} catch (JSONException e) {
   throw new RuntimeException(e);
}

The "wrapper" constructor of RuntimeException is really useful for getting around annoying checked exceptions :)

@@ +586,5 @@
> +            ret.put("y", (int)Math.round(point.y));
> +        } catch(Exception ex) {
> +            Log.w(LOG_NAME, "Error building return: " + ex);
> +        }
> +        Log.w(LOG_NAME, "Send doubletap " + ret.toString());

Remove debug code.

::: mobile/chrome/content/browser.js
@@ +594,5 @@
>        this.panZoom(aData);
>      } else if (aTopic == "FullScreen:Exit") {
>        browser.contentDocument.mozCancelFullScreen();
> +    } else if (aTopic == "Gesture:DoubleTap") {
> +      dump("got double tap");

Remove.

@@ +613,5 @@
> +    if (!element || element == this._zoomedToElement) {
> +      this._zoomedToElement = null;
> +      // zoom out, try to keep the center in the center of the page?
> +      setTimeout(function() {
> +        dump("ZoomToPageWidth");

Remove.

@@ +1854,5 @@
> +    if (!aElement)
> +      return {x: 0, y: 0, w: 0, h: 0};
> +  
> +    let document = aElement.ownerDocument;
> +    while(document.defaultView.frameElement)

nit: while (document.defaultView.frameElement)

Comment 9

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/projects/birch/rev/8f3eacc9ceb6

Comment 10

[Wesley Johnston (:wesj)]

Need to test this a bit more. Backed out.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch v2

I had to add some Runnable stuff to make this not crash now that the Flash stuff landed. It works, but I wanted to have someone look at it and tell me if its wrong.

Comment 12

[Patrick Walton (:pcwalton)]

Comment on attachment 575548 [details] [diff] [review]
Patch v2

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

Mostly there, except for the logic in the pan/zoom controller. Thanks for this work!

::: embedding/android/GeckoApp.java
@@ +807,5 @@
> +                    float x = (float)message.getDouble("x");
> +                    float y = (float)message.getDouble("y");
> +                    float width = (float)message.getDouble("w");
> +                    float height = (float)message.getDouble("h");
> +                    mLayerController.getPanZoomController().animatedZoomTo(x, y, width, height, 200);

I'd make the time a constant.

@@ +818,5 @@
> +                    float y = viewableRect.top;
> +                    float dh = (viewableRect.height() * pageSize.width/viewableRect.width()) - viewableRect.height(); // increase in the height
> +                    y -= dh/2;
> +                    mLayerController.getPanZoomController().animatedZoomTo(0, y, pageSize.width,
> +                                                    pageSize.width * viewableRect.height()/viewableRect.width(), 200);

Here too.

::: embedding/android/gfx/LayerController.java
@@ +173,1 @@
>      public void setVisibleRect(float x, float y, float width, float height) {

Maybe we should just remove the setVisibleRect(float,float,float,float) function in favor of the RectF version.

::: embedding/android/gfx/RectUtils.java
@@ +88,5 @@
>                           x + (rect.width() * scale),
>                           y + (rect.height() * scale));
>      }
> +
> +    public static RectF clamp(RectF rect, RectF dest) {

I'd add a comment above saying what this function does.

@@ +96,5 @@
> +        float y = Math.max(dest.top, Math.min(dest.bottom-rect.height(), rect.top));
> +        return new RectF(x, y, x+width, y+height);
> +    }
> +
> +    public static RectF blend(RectF aRect1, RectF aRect2, float aBlendAmount) {

For this function too.

nit: Also we don't use aArgument style in this file.

::: embedding/android/ui/PanZoomController.java
@@ +598,5 @@
> +        IntSize pageSize = mController.getPageSize();
> +        RectF pageRect = new RectF(0,0, pageSize.width, pageSize.height);
> +        if (!pageRect.contains(visible)) {
> +            RectF rect = mController.clampToScreenSize(visible);
> +            animatedZoomTo(rect.left, rect.top, rect.width(), rect.height(), 200);

Use the constant here.

@@ +658,5 @@
> +
> +        mZoomTimer = new Timer();
> +        final RectF startRect = mController.getVisibleRect();
> +        final long startTime = new Date().getTime();
> +        mZoomTimer.scheduleAtFixedRate(new TimerTask() {

Make sure this isn't racy. In particular I could foresee races when setting the visible rect on the layer controller. It may make sense to post this on the UI thread.

Most importantly, I think that animated zoom should probably be one of the states. I don't think it makes sense to be able to fling at the same time you're zooming, for example. The pan/zoom controller is really built around the idea of doing one thing at a time, because getting all the corner cases right is a lot easier that way. You'll probably want to add an ANIMATED_ZOOM state, and switch back into NOTHING at the end of the zoom animation. Ignore all pinch and pan attempts while the state is set to ANIMATED_ZOOM.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch v3

This is growing...

I think I've got your changes all included patrick. Hope they're right.

mfinkle, I wound up adding some code because doubletaps were also triggering clicks. So I moved the "click" detection code to use native android versions and send messages to Gecko. Because that also introduces a delay (to detect single vs double tap), I also brought back our old friend tap highlighting. 

Then, just for fun, apparently firing touchdown doesn't remove focus from textboxes (contrary to bugs 701706 and 701947?) so I also added a getFocusManager.clear() call to clear focus if you tap on something non-clickable. Maybe that has something to do with the way we're dispatching mouse events?

We're now really not hitting any of the mouse event dispatching code in widget, I may just remove it when touchevents land.

Comment 14

[Wesley Johnston (:wesj)]

On the train home had a thought about why clicking isn't changing focus. Forgot that we're intercepting all those mousedown/mouseup/click events and killing them. Hopefully we don't need to do that anymore since they're only firing during clicks (we'll have touch events if pages want to get more detailed information about what a person is doing).

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch v4

This removes all of our click handling code from browser.js which let me get rid of the FocusManager stuff as well. Also fixed a bug with tap highlight and context menus.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 576175 [details] [diff] [review]
Patch v4

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

> XPCOMUtils.defineLazyServiceGetter(this, "URIFixup",
>   "@mozilla.org/docshell/urifixup;1", "nsIURIFixup");
>+XPCOMUtils.defineLazyServiceGetter(this, "gDOMUtils",
>+  "@mozilla.org/inspector/dom-utils;1", "inIDOMUtils");

gDOMUtils -> DOMUtils

>+    Services.obs.addObserver(this, "Gesture:DoubleTap", false);
>+    Services.obs.addObserver(this, "Gesture:SingleTap", false);
>+    Services.obs.addObserver(this, "Gesture:ShowPress", false);
>+    Services.obs.addObserver(this, "Gesture:CancelTouch", false);

Can we move these to ElementTouchHelper or a new GestureHandler or even BrowserEventHandler? Too much stuff is being added to BrowserApp, making it look like a kitchen sink.

>+    } else if (aTopic == "Gesture:ShowPress") {
>+      let data = JSON.parse(aData);
>+      let closest = ElementTouchHelper.elementFromPoint(BrowserApp.selectedBrowser.contentWindow, data.x, data.y);
>+      if (!closest) {
>+        closest = ElementTouchHelper.anyElementFromPoint(BrowserApp.selectedBrowser.contentWindow, data.x, data.y);
>+      }

No {}

>+  onDoubleTap: function(aData) {

>+    win = element.ownerDocument.defaultView;
>+    while (element && win.getComputedStyle(element,null).display == "inline")
>+      element = element.parentNode;
>+    if (!element || element == this._zoomedToElement) {

Add line break before the "if"

>+      setTimeout(function() {
>+        rect.type = "Browser:ZoomToRect";
>+        rect.x -= 2*margin;
>+        rect.w += 2*margin;

2 * margin  (add spaces)

>     observe: function(aSubject, aTopic, aData) {
>       let data = JSON.parse(aData);
>+      BrowserApp.cancelTapHighlight();

Move BrowserApp.... to the top of the method, it looks jammed in between the context menu code

>     return result;
>+  },
>+  getBoundingContentRect: function(aElement) {

Add line break

You removed all the mouse/click handling from BrowserEventHandler. Is this functionality in Java now or is this dependent on another bug/patch?

browser part r+ with nits fixed and the mouse/click question answered.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch v5

Sorry for the churn. Just fixing mfinkle's comments.
> gDOMUtils -> DOMUtils
DONE

> Can we move these to ElementTouchHelper or a new GestureHandler or even
> BrowserEventHandler? Too much stuff is being added to BrowserApp, making it
> look like a kitchen sink.
Moved to BrowserEventHandler, since it was previously dealing with these things anyway.

> No {}
Fixed
> Add line break before the "if"
Fixed

> 2 * margin  (add spaces)
Fixed
> Move BrowserApp.... to the top of the method, it looks jammed in between the
> context menu code
Fixed

> You removed all the mouse/click handling from BrowserEventHandler. Is this
> functionality in Java now or is this dependent on another bug/patch?

This is all being done in Java now. Basically java is telling browser.js when to dispatch a click. browser.js looks for a clickable element and clicks it if it can. Otherwise it just fires mousemove/mouseup/mousedown on whatever was directly under your finger (EventStateManager handles firing a click from that).

My grand plan is to implement touchevents and remove our support for dispatching mouse events when we receive MotionEvents in widget (maybe I can add a pref for that?). If we need to work with individual mousedowns or mousemoves in the frontend, we'll have to write code that watches for touch events (which won't give us selection problems) just like we expect pages to do.

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 576196 [details] [diff] [review]
Patch v5

Moving review to Chris Lord just to take some pressure off Patrick. He won't get to this till tomorrow though, so if you want it pcwalton (or are mad that I moved it?) feel free to hollar.

Comment 19

[Chris Lord [:cwiiis]]

Comment on attachment 576196 [details] [diff] [review]
Patch v5

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

Some naming quibbles, but overall good. Unfortunately, this has bit-rotted quite a bit. While I'm r+'ing this, the rebase will be substantial and ought to be reviewed as well.

::: mobile/android/base/GeckoApp.java
@@ +849,5 @@
> +            } else if (event.equals("Browser:ZoomToPageWidth")) {
> +                if (mLayerController != null) {
> +                    IntSize pageSize = mLayerController.getPageSize();
> +                    RectF viewableRect = mLayerController.getVisibleRect();
> +                    // attempt to keep the middle of the screen in the middle of the screen

'zoom focused on the center of the viewport' ?

@@ +1119,5 @@
>              lp.repositionFromVisibleRect(mLayerController.getVisibleRect(), mLayerController.getZoomFactor(), resize);
>  
>              if (setVisible) {
> +                view.post(new Runnable() {
> +                    public void run() {

Unrelated change?

::: mobile/android/base/GeckoAppShell.java
@@ -471,5 @@
> -
> -                GeckoAppShell.sendEventToGecko(new GeckoEvent(event));
> -
> -                /* Restore the view coordinates in case the caller further processes this event */
> -                event.setLocation(origX, origY);

Why is this block removed? I guess this would mean that we don't queue up events to send to Gecko, but perhaps we don't need to/this is the wrong way?

::: mobile/android/base/gfx/LayerView.java
@@ +69,5 @@
>          mController = controller;
>          mRenderer = new LayerRenderer(this);
>          setRenderer(mRenderer);
>          mGestureDetector = new GestureDetector(context, controller.getGestureListener());
> +        mGestureDetector.setOnDoubleTapListener(controller.getDoubleTapListener());

Not really sure why the GestureDetector code lives in LayerView and not LayerController really, but this patch doesn't change that...

::: mobile/android/base/gfx/RectUtils.java
@@ +93,5 @@
> +     Returns a new RectF which restricts a source rect to the area inside a second destination rect.
> +     If the source rect is wider/taller than the destination rect, it's width/height will be shortened
> +     (and its aspect ratio will NOT be maintained).
> +    */
> +    public static RectF clamp(RectF rect, RectF dest) {

I think I would call this crop, rather than clamp.

@@ +105,5 @@
> +    /*
> +     Returns a new rect that is a mixture of a start and end rect. blendAmount conrols the weighting
> +     of each of the original rects (blendAmount = 1 returns endRect, blendAmount = 0 returns startRect)
> +    */
> +    public static RectF blend(RectF startRect, RectF endRect, float blendAmount) {

Similarly, I'd call this interpolate rather than blend.

::: mobile/android/base/ui/PanZoomController.java
@@ +613,5 @@
>      @Override
>      public boolean onScale(ScaleGestureDetector detector) {
> +        cancelTouch();
> +        if (mState == PanZoomState.ANIMATED_ZOOM)
> +            return false;

Should this cancelTouch be within the if block below it?

@@ +690,5 @@
> +        return true;
> +    }
> +
> +    private Timer mZoomTimer;
> +    public boolean animatedZoomTo(float x, float y, float width, float height, final float duration) {

Maybe the animation duration constant should just live in PanZoomController, and this parameter could be removed?

@@ +702,5 @@
> +        float newHeight = width * screenSize.height / screenSize.width;
> +        // if the requested rect would not fill the screen, shift it to be centered
> +        if (height < newHeight) {
> +            y -= (newHeight - height)/2;
> +        }

Unnecessary braces.

@@ +736,5 @@
> +                    mState = PanZoomState.NOTHING;
> +                    GeckoApp.mAppContext.showPluginViews();
> +               }
> +            }
> +        }, 0, 1000L/60L);

Just a note more as a reminder to myself, we really need to have drawing callbacks instead of scheduling things to run at 60Hz...

::: mobile/android/chrome/content/browser.js
@@ +1501,5 @@
>                                                 false); /* don't flush layout */
>  
>      // return null if the click is over a clickable element
>      if (this._isElementClickable(target))
> +      return target;

Should change the comment too.

@@ +1714,5 @@
> +    while (aTarget) {
> +      if (this._isSelectElement(aTarget)) {
> +        let list = this.getListForElement(aTarget);
> +        this.show(list, aTarget);
> +        aTarget = null;

Is this change necessary?

Comment 20

[Wesley Johnston (:wesj)]

Attachment: Patch v6

Rebased on the new viewport stuff. I have a feeling that I'm doing some coordinate transforms I don't need to do here. Happy to remove them now or in a follow up.

I added the 'snap-back' when you pinch zoom, but this will try to snap-back so that the scroll position is maintained on the page. That feels a bit awkward, but is livable (we should probably aim to keep the "center" of the visible page in the center of the screen?). The more annoying part is, if you pinch zoom past the bottom of the page it will snap back to a strange position.

I'd rather address those things in a follow up (or patrick said he had a different approach for this we could switch to?), but happy to debate. A build is available at: http://dl.dropbox.com/u/72157/doubletap.apk

Comment 21

[Chris Lord [:cwiiis]]

Comment on attachment 578523 [details] [diff] [review]
Patch v6

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

Once the comments below are addressed, this is good to go.

::: mobile/android/base/GeckoApp.java
@@ +886,5 @@
>  
>                  tab.setAgentMode(agentMode);
>                  if (tab == Tabs.getInstance().getSelectedTab())
>                      updateAgentModeMenuItem(tab, agentMode);
> +            } else if (event.equals("Browser:ZoomToRect")) {

I think these event handlers should be moved into PanZoomController, they make more sense there.

@@ +1220,5 @@
> +            view.post(new Runnable() {
> +                public void run() {
> +                    view.setVisibility(View.GONE);
> +                }
> +            });

Is this necessary? It seems unrelated - I'd rather this was done in a separate patch on a different bug if it fixes something.

@@ +1241,4 @@
>              lp.reposition(hostViewport, targetViewport);
>  
> +            view.post(new Runnable() {
> +                public void run() {

Same here.

::: mobile/android/base/gfx/LayerController.java
@@ +234,5 @@
>          mView.requestRender();
>      }
>  
> +    public void zoomTo(PointF origin, float zoomFactor) {
> +    	  mViewportMetrics.zoomTo(origin, zoomFactor);

Bad indentation (tabs instead of spaces?)

@@ +273,5 @@
>          return new RectF(x, y, x + TILE_WIDTH, y + TILE_HEIGHT);
>      }
>  
> +    public RectF clampToPageSize(RectF aRect) {
> +    	  FloatSize pageSize = getPageSize().scale(1/getZoomFactor());

Ditto. Also, this pageSize should not be scaled (see comments later).

::: mobile/android/base/gfx/PointUtils.java
@@ +38,5 @@
>  package org.mozilla.gecko.gfx;
>  
>  import android.graphics.Point;
>  import android.graphics.PointF;
> +import android.graphics.RectF;

Unnecessary.

@@ +60,5 @@
>          return new Point(Math.round(point.x), Math.round(point.y));
>      }
> +
> +   /*
> +    Returns a new pointthat is a mixture of a start and end points. blendAmount conrols the weighting

Missing space (s/pointthat/point that/)

Also, comment format isn't quite right; we do this:

/* Multiple
 * Line
 * Comment
 */

(missing stars at the start of each line)

@@ +61,5 @@
>      }
> +
> +   /*
> +    Returns a new pointthat is a mixture of a start and end points. blendAmount conrols the weighting
> +    of each of the original points (blendAmount = 1 returns endRect, blendAmount = 0 returns startRect)

s/Rect/Point

::: mobile/android/base/gfx/RectUtils.java
@@ +40,5 @@
>  import android.graphics.Rect;
>  import android.graphics.RectF;
>  import org.json.JSONException;
>  import org.json.JSONObject;
> +import android.util.Log;

This should go above JSONException.

@@ +45,3 @@
>  
>  public final class RectUtils {
> +    static private String LOGTAG = "GeckoRectUtils";

There's no logging here, don't need to add this.

@@ +112,5 @@
> +    /*
> +     Returns a new RectF which restricts a source rect to the area inside a second destination rect.
> +     If the source rect is wider/taller than the destination rect, it's width/height will be shortened
> +     (and its aspect ratio will NOT be maintained).
> +     XXX - A better solution should clip so that we scale towards the center of rect rather than to its top

I'm not sure I understand this XXX comment in the context of a crop function - maybe this ought to go where you use it, instead of here? Also, bad comment format (see above).

@@ +118,5 @@
> +    public static RectF crop(RectF rect, RectF dest) {
> +        float width = Math.min(rect.width(), dest.width());
> +        float height = Math.min(rect.height(), dest.height());
> +        float x = Math.max(dest.left, Math.min(dest.right-width, rect.left));
> +        float y = Math.max(dest.top, Math.min(dest.bottom-height, rect.top));

This isn't quite right I don't think? I can't quite follow it, I think it'd be clearer as:


float left = Math.max(rect.left, dest.left);
float top = Math.max(rect.top, dest.top);
float right = Math.min(rect.right, dest.right);
float bottom = Math.min (rect.bottom, dest.bottom);
return new RectF(left, top, right, bottom);

Note that this would possibly return an empty rect (android docs define empty as top/left being >= bottom/right, so negative rectangles like the one this would return would class as empty). Perhaps put an intersects check at the top, and return a 0,0,0,0 rectangle if they don't intersect, to stop other code from going screwy.

::: mobile/android/base/gfx/ViewportMetrics.java
@@ +215,5 @@
>  
>          mZoomFactor = newZoomFactor;
>      }
>  
> +    public void zoomTo(PointF origin, float newZoomFactor) {

This is misleadingly named given the differences between scaleTo and setZoomFactor. I'd rather we just had a scale function that calls scaleTo with a 0,0 focus, and calls to zoomTo can be replaced by a call to setOrigin followed by a call to scale.

Alternatively, this file can go unchanged and you can replace zoomTo with a call to setOrigin, followed by a call to scaleTo with a 0,0 focus.

::: mobile/android/base/ui/PanZoomController.java
@@ +44,5 @@
>  import org.mozilla.gecko.FloatUtils;
>  import org.mozilla.gecko.GeckoApp;
>  import org.mozilla.gecko.GeckoAppShell;
>  import org.mozilla.gecko.GeckoEvent;
> +import org.mozilla.gecko.gfx.RectUtils;

This should go below PointUtils.

@@ +54,5 @@
>  import android.view.ScaleGestureDetector;
>  import java.lang.Math;
>  import java.util.Timer;
>  import java.util.TimerTask;
> +import java.util.Date;

Likewise, this should go above Timer.

@@ +738,5 @@
> +        RectF viewport = mController.getViewport();
> +        float scale = mController.getZoomFactor();
> +        viewport = RectUtils.scale(viewport, 1/scale);
> + 
> +        FloatSize pageSize = mController.getPageSize().scale(1/scale);

Both of these scales should be removed, the Java compositor shouldn't need to do transformations like these - they should be kept in browser.js and done when sending coordinates over.

@@ +839,5 @@
> +            mZoomTimer.cancel();
> +        }
> +
> +        mState = PanZoomState.ANIMATED_ZOOM;
> +        final float startScale = mController.getZoomFactor();

I would call this startZoom (elsewhere, zoom usually refers to an absolute value and scale refers to a relative value).

@@ +852,5 @@
> +            zoomToRect.bottom = zoomToRect.top + newHeight;
> +        }
> +
> +        zoomToRect = mController.clampToPageSize(zoomToRect);
> +        final float finalScale = viewport.width() / zoomToRect.width();

with the other suggested changes, finalZoom would be (viewport.width() / startZoom) / zoomToRect.width().

@@ +853,5 @@
> +        }
> +
> +        zoomToRect = mController.clampToPageSize(zoomToRect);
> +        final float finalScale = viewport.width() / zoomToRect.width();
> +        zoomToRect = RectUtils.scale(zoomToRect, finalScale);

Along with the other scale removals, there's no need for this one.

@@ +866,5 @@
> +                long now = new Date().getTime();
> +                final float dt = (float)(now - startTime)/ZOOM_DURATION;
> +
> +                if (dt < 1) {
> +                    controller.getView().post(new Runnable() {

You can just do controller.post() here.

@@ +869,5 @@
> +                if (dt < 1) {
> +                    controller.getView().post(new Runnable() {
> +                        public void run() {
> +                        	  PointF currentPoint = PointUtils.interpolate(finalPoint, startPoint, dt);
> +                        	  float  currentScale = startScale + (finalScale-startScale)*dt;

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 might be wrong about this though, maybe it ends up simplifying down to this - I'd try it out and make sure though.

::: mobile/android/chrome/content/browser.js
@@ +1476,5 @@
> +      this._zoomOut();
> +    } else if (element) {
> +      const margin = 15;
> +      this._zoomedToElement = element;
> +      rect = ElementTouchHelper.getBoundingContentRect(element);

With the other suggested changes, this rect needs to be multiplied by the current viewport zoom factor (it should be multiplied *before* adding the margin, rather than after).

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch v7

(In reply to Chris Lord [:cwiiis] from comment #21)
> I think these event handlers should be moved into PanZoomController, they
> make more sense there.

Good idea! Done.

> Is this necessary? It seems unrelated - I'd rather this was done in a
> separate patch on a different bug if it fixes something.

I was crashing on animated zooms for nytimes.com without this. I've left it in. We can get snorp or someone to look it over maybe.

> This isn't quite right I don't think? I can't quite follow it, I think it'd
> be clearer as...

I played with this for a bit. The sample you give is probably better for the generic definition of "crop", but doesn't give us what we want. For instance, if the page is pinched beyond its bottom, it can leave empty space below the page.  I left my code in here because my brain is not coming up with a better solution right now.

> This is misleadingly named given the differences between scaleTo and
> setZoomFactor. I'd rather we just had a scale function that calls scaleTo
> with a 0,0 focus, and calls to zoomTo can be replaced by a call to setOrigin
> followed by a call to scale.

I implemented this.

> Both of these scales should be removed, the Java compositor shouldn't need
> to do transformations like these - they should be kept in browser.js and
> done when sending coordinates over.

I'm scaling the coordinates sent from browser.js now, which puts them in the same coordinate system as getPageSize(). Viewport() is actually in a different coordinate system, and so I need to scale it sometimes?

> with the other suggested changes, finalZoom would be (viewport.width() /
> startZoom) / zoomToRect.width().

I used viewport.width() * startZoom / zoomToRect.width(), but works!

> 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 might be wrong about this though, maybe it ends up simplifying down to
> this - I'd try it out and make sure though.

When we set the viewport, we need to be in viewport coordinates, so I left in the scaling to finalPoint done earlier (startPoint is already scaled since it IS the viewport). Scaling currentPoint as well results in a strange looking zoom animation, so I left it out.

Comment 23

[Chris Lord [:cwiiis]]

Comment on attachment 578802 [details] [diff] [review]
Patch v7

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

Almost, just some small things to address and it's good to go.

Apologies for being quite hard on this, I just want to make sure that this code remains reasonably easy to understand :)

::: mobile/android/base/gfx/LayerController.java
@@ +223,5 @@
>          GeckoApp.mAppContext.repositionPluginViews(false);
>          mView.requestRender();
>      }
>  
> +    public void scale(float zoomFactor) {

I'd rather this was called scaleTo than scale, as it's not relative.

@@ +237,5 @@
>          GeckoApp.mAppContext.repositionPluginViews(false);
>          mView.requestRender();
>      }
>  
> +    public void zoomTo(PointF origin, float zoomFactor) {

This really does the same thing as scaleTo, but with different parameters - either we should use setOrigin+scale, or rename this to scaleWithFocus/scaleWithOrigin (see comments in ViewportMetrics.java).

::: mobile/android/base/gfx/PointUtils.java
@@ +60,5 @@
> +
> +   /* Returns a new point that is a mixture of a start and end points. blendAmount conrols the weighting
> +    * of each of the original points (blendAmount = 1 returns endPoint, blendAmount = 0 returns startPoint)
> +    */
> +   public static PointF interpolate(PointF startPoint, PointF endPoint, float blendAmount) {

And to be pedantic again, could we change blendAmount to weight?

::: mobile/android/base/gfx/RectUtils.java
@@ +110,5 @@
> +    /* Returns a new RectF which restricts a source rect to the area inside a second destination rect.
> +     * If the source rect is wider/taller than the destination rect, it's width/height will be shortened
> +     * (and its aspect ratio will NOT be maintained).
> +    */
> +    public static RectF crop(RectF rect, RectF dest) {

I see what this is doing a bit better now, and think this would be better named 'restrict'.

::: mobile/android/base/gfx/ViewportMetrics.java
@@ +215,5 @@
>  
>          mZoomFactor = newZoomFactor;
>      }
>  
> +    public void zoomTo(PointF origin, float newZoomFactor) {

Still don't like this - either we should figure the maths and use scaleTo on its own, or call setOrigin, followed by scaleTo with a PointF(0,0) origin (which will do the same thing).

If this absolutely must be a separate function, I'd rename scaleTo->scaleWithFocus, call this scaleWithOrigin, and I'd still implement it as a call to setOrigin, followed by a call to scaleWithFocus - it's not necessary to have this scaling code more than once in this file.

::: mobile/android/base/ui/PanZoomController.java
@@ +145,5 @@
> +                    float y = (float)message.getDouble("y");
> +                    RectF zoomRect = new RectF(x, y,
> +                                         x + (float)message.getDouble("w"),
> +                                         y + (float)message.getDouble("h"));
> +                    animatedZoomTo(zoomRect);

This will be called in the Gecko thread, should probably use mController.post? This may have been the root-cause of the crash you saw with the plugins - if it is, I'd suggest removing the post added in GeckoApp.hidePluginsView.

@@ +160,5 @@
> +                    RectF r = new RectF(0.0f,
> +                                        y + dh/2,
> +                                        pageSize.width,
> +                                        (y + pageSize.width * viewableRect.height()/viewableRect.width()));
> +                    animatedZoomTo(r);

Same here.

@@ +920,5 @@
> +                    });
> +                    populatePositionAndLength();
> +                } else {
> +                    mController.setForceRedraw();
> +                    GeckoApp.mAppContext.showPluginViews();

These two lines should probably come below the zoomTo in the Runnable below?

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch v8

Thanks for the comments. I think this looks a lot nicer now.

> Still don't like this - either we should figure the maths and use scaleTo on
> its own, or call setOrigin, followed by scaleTo with a PointF(0,0) origin
> (which will do the same thing).

Oops. I'm not even using this. Just removed it instead.

> This will be called in the Gecko thread, should probably use
> mController.post? This may have been the root-cause of the crash you saw
> with the plugins - if it is, I'd suggest removing the post added in
> GeckoApp.hidePluginsView.

I'm posting to the other thread from within AnimatedZoomTo. Is it would be better to do this earlier? I moved the hide/showPluginViews calls to be inside the runnables and am not seeing crashes any more so I removed the plugin view.postRunnable stuff.

Comment 25

[Chris Lord [:cwiiis]]

Comment on attachment 579045 [details] [diff] [review]
Patch v8

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

Very close now, so marking as r+ with the assumption that the below changes will be made (feel free to r? again if you want further review).

We need to post the animatedZoomTo calls to the main thread, as the function is called by both the main thread (in onScaleEnd) and the Gecko thread (in the message handler). The function alters variables without any kind of lock, so while things may work ok, animatedZoomTo is definitely not a thread-safe call.

::: mobile/android/base/ui/PanZoomController.java
@@ +144,5 @@
> +                    float y = (float)message.getDouble("y");
> +                    RectF zoomRect = new RectF(x, y,
> +                                         x + (float)message.getDouble("w"),
> +                                         y + (float)message.getDouble("h"));
> +                    animatedZoomTo(zoomRect);

This should be posted as a Runnable to the main thread.

@@ +149,5 @@
> +                }
> +            } else if (event.equals("Browser:ZoomToPageWidth")) {
> +                if (mController != null) {
> +                    float scale = mController.getZoomFactor();
> +                    FloatSize pageSize = mController.getPageSize();//.scale(1/scale);

Old comment?

@@ +152,5 @@
> +                    float scale = mController.getZoomFactor();
> +                    FloatSize pageSize = mController.getPageSize();//.scale(1/scale);
> +
> +                    RectF viewableRect = mController.getViewport();
> +                    float y = viewableRect.top;// * mController.getZoomFactor();

Same here?

@@ +159,5 @@
> +                    RectF r = new RectF(0.0f,
> +                                        y + dh/2,
> +                                        pageSize.width,
> +                                        (y + pageSize.width * viewableRect.height()/viewableRect.width()));
> +                    animatedZoomTo(r);

Same as above call to animatedZoomTo.

@@ +875,5 @@
> +    }
> +
> +    private Timer mZoomTimer;
> +    public boolean animatedZoomTo(RectF zoomToRect) {
> +        mController.post(new Runnable() {

Once the animatedZoomTo calls are posted to the main thread, this won't be necessary.

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Patch for checkin

Comment 27

[Chris Lord [:cwiiis]]

http://hg.mozilla.org/projects/birch/rev/f8c174b95c40

Comment 28

[Carla Nadastean]

Retested with:
Mozilla/5.0 (Android; Linux armv7l; rv:11.0a1) Gecko/20111212 Firefox/11.0a1 Fennec/11.0a1
HTC Desire Z (Android 2.3)

Zoom can be performed using double-tap gesture.
Verifying.

Comment 29

[Catalin Suciu [:csuciu]]

Testcase created:
https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=45316