804985 - [browser] don't try to zoom out wider than the page width

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P4)

Description

[Tim Taubert [:ttaubert] (inactive)]

If you navigate to http://timtaubert.de/ (sorry, I didn't find any other example) the page fits the viewport. Double-tapping does not zoom in or out. Now pinch-zoom into the page to make it bigger. Now double-tap to get back to your original size:

Actual results:
 + We're zooming out of the page and it gets really small.
 + At least, double-tapping it now again should get the page back to the viewport size and not to the zoomed-in state from before.

Expected results:
 + Don't zoom out if we didn't allow it before.

Comment 1

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

For the site: <meta content="width=device-width, initial-scale=1.0" name="viewport">
Don't you have to have a maximum-scale=1 or  user-scalable = no set to not allow for scaling?  I'm not sure if this is a bug in the expected result.  

I would think that the double tapping not scaling when the device-width being set would be the bug in this case.

Comment 2

[Andrew Overholt [:overholt]]

This seems like polish (important polish, mind you) but not something we'd block on fixing.  Please re-nom with further justification if you'd like us to re-evaluate during triage.  Thanks.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Naoki Hirata :nhirata from comment #1)
> For the site: <meta content="width=device-width, initial-scale=1.0"
> name="viewport">
> Don't you have to have a maximum-scale=1 or  user-scalable = no set to not
> allow for scaling?  I'm not sure if this is a bug in the expected result.  
> 
> I would think that the double tapping not scaling when the device-width
> being set would be the bug in this case.

I don't really know, tbh. I checked with Firefox on Android and it doesn't allow you to zoom out either. Unlike the B2G browser, double-tapping brings you back to the original viewport you started with.

Comment 4

[Lawrence Mandel [:lmandel] (use needinfo)]

Tim, I agree that this behaviour is not ideal. We do want to fix this issue but it will not block basecamp.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Lawrence Mandel [:lmandel] from comment #4)
> Tim, I agree that this behaviour is not ideal. We do want to fix this issue
> but it will not block basecamp.

Oops, I'm sorry. I re-requested this by accident. I agree that this shouldn't block.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: take current zoom level into account when zooming to an element

The problem here is that when you make an element so big that it exceeds the screen bounds and then double-tap to zoom to it, we don't take the current zoom level into account. Pretty simple fix.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 679421 [details] [diff] [review]
take current zoom level into account when zooming to an element

Hi Tim,

It looks like the site sets the viewport to "width=device-width".  Since the page isn't horizontally pannable, then the "CSS page rect" (overflow of the viewport frame) should have the same width as the device width.  We don't allow zooming out wider than the "page rect".  So I don't think this patch is correct.

The value computed here is supposed to be in CSS pixels, so applying a zoom to it is not valid.

I think we need to ensure here that the double-tap zoom-out is clamped to the CSS page rect appropriately.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Just to clarify the issue, I made this:

http://i.imgur.com/QpJZZ.png

1) Go to nytimes.com.
2) Double-click to zoom into one of the smaller paragraphs.
3) Pinch-zoom in even further.
4) Double-click the paragraph again to get back to the state after (2)

nytimes.com doesn't specify a viewport. With my patch applied this issue is fixed. Is the way I fixed still wrong no matter if the page specifies a viewport or not?

To be clear: isn't the issue that we should make the paragraph fit the viewport width again after double-clicking in step (4)? If we clamp the zoom-out to the css page rect we'll just zoom out completely (but still more correctly than we do now).

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note, the viewport isn't the same as the page rect.  I don't understand what the underlying bug is here; we need to figure out why the computation is wrong.  Just applying a scale to values that have particular units isn't necessarily valid.

Comment 11

[Benjamin Chen [:bechen]]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> Created attachment 679421 [details] [diff] [review]
> take current zoom level into account when zooming to an element
> 
> The problem here is that when you make an element so big that it exceeds the
> screen bounds and then double-tap to zoom to it, we don't take the current
> zoom level into account. Pretty simple fix.

Hi Tim:
Now I am looking bug 805535 and just find out these 2 bugs are relative or duplicate.

I agree that we may need to take current zoom level into BrowserElementScrolling.js. 
But maybe it should be used for     
this._viewport = new Rect(metrics.x, metrics.y,
metrics.viewport.width / zoom,
metrics.viewport.height / zoom);

In my test environment, the metrics.viewport is always the same even though I pinch zoom in it. That cause the method _isRectZoomedIn is not work.

And now I still not figure out why the metrics.viewport is not changed with the zoom level. (TabChild::RecvUpdateFrame(const FrameMetrics& aFrameMetrics))

Comment 12

[Benjamin Chen [:bechen]]

Attachment: 1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect

1. bring the zoom factor into BrowserElementScroll.js
Due to the method _isRectZoomedIn need the viewport info with zoom factor

2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect
duplicate Bug 805535
The zoom is relative scale number

Comment 13

[Benjamin Chen [:bechen]]

Attachment: 1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect 3. fix the relation between _isRectZoomedIn and AsyncPanZoomController::ZoomToRect

In this patch, I remove http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1214 to L1230. And modify the _isRectZoomedIn in BrowserElementScrolling.js.

It's weird that we need to increase/decrease the rect width/height at AsyncPanZoomController.cpp#1214 to L1230 since the code doesn't consider all . the cases. 
float targetRatio = compositionBounds.width / compositionBounds.height;
the targetRatio may be smaller than 1 or bigger than 1.

For example(apply attachment 682972 [details] [diff] [review]): open browser, go to www.nytimes.com in landscape mode.
Double tapping a text area, it will zoom in. Then double tapping again, it won't zoom out. 
Because AsyncPanZoomController.cpp#1214 to L1230 enlarge the zoomRect so the _isRectZoomedIn method always return false => can't zoom out.

So does the same reason for _isRectZoomedIn.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

I'd like to give this a try but this patch does unfortunately not apply cleanly.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Oh, wait. This doesn't apply on Aurora (soon Beta), right?

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Sorry, it looks like you attached the wrong patch?

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 683031 [details] [diff] [review]
1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect 3. fix the relation between _isRectZoomedIn and AsyncPanZoomController::ZoomToRect

Yeah, wrong patch :).

Comment 18

[Benjamin Chen [:bechen]]

Attachment: 1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect 3. fix the relation between _isRectZoomedIn and AsyncPanZoomController::ZoomToRect

Sorry for the wrong patch.

Comment 19

[Benjamin Chen [:bechen]]

Attachment: 1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect

Modify
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementScrolling.js#260

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 683487 [details] [diff] [review]
1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect

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

The patch works great for me and fixes the problem described here.

Comment 21

[Benjamin Chen [:bechen]]

Attachment: 1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect 3. fix the relation between _isRectZoomedIn and AsyncPanZoomController::ZoomToRect

1. bring the zoom factor into BrowserElementScrolling.js
The this._viewport is used for this._isRectZoomedIn method which computes the 
area ratio of targetRect and this._viewport. So the _viewport is changed by zoom if we do some zoom operations.

2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect
The final zoom "targetZoom" is computed by targetResolution and resolution.width.
Futhermore the resolution is computed by AsyncPanZoomController::CalculateResolution().
The CalculateResolution() had zoom factor inside. Therefore the targetZoom is a relative ratio to current zoom.

3. fix the relation between _isRectZoomedIn and AsyncPanZoomController::ZoomToRect
We can't increase/decrease the Rect width/height if we don't consider the relation between _isRectZoomedIn 
and AsyncPanZoomController::ZoomToRect. Such as narrow/fat rect, narrow/fat screen, portrait/landscape mode. So I remove the code that can't work at all situations.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 683852 [details] [diff] [review]
1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect 3. fix the relation between _isRectZoomedIn and AsyncPanZoomController::ZoomToRect

>diff --git a/dom/browser-element/BrowserElementScrolling.js b/dom/browser-element/BrowserElementScrolling.js
>--- a/dom/browser-element/BrowserElementScrolling.js
>+++ b/dom/browser-element/BrowserElementScrolling.js
>@@ -289,35 +289,27 @@ const ContentPanning = {
>   _zoomOut: function() {
>     let rect = new Rect(0, 0, 0, 0);
>     var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>     os.notifyObservers(docShell, 'browser-zoom-to-rect', JSON.stringify(rect));
>   },
> 
>   _isRectZoomedIn: function(aRect, aViewport) {
>     // This function checks to see if the area of the rect visible in the
>-    // viewport (i.e. the "overlapArea" variable below) is approximately
>-    // the max area of the rect we can show. It also checks that the rect
>-    // is actually on-screen by testing the left and right edges of the rect.
>-    // In effect, this tells us whether or not zooming in to this rect
>-    // will significantly change what the user is seeing.
>-    const minDifference = -20;
>-    const maxDifference = 20;

I understand what this change is intended to do, but what bug is it
fixing?  This code is originally from Firefox on Android, so it would
be better if we kept these heuristics the same for platform
consistency.  Unless this is fixing a known bug ...

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
>--- a/dom/ipc/TabChild.cpp
>+++ b/dom/ipc/TabChild.cpp
>@@ -1153,18 +1153,18 @@ TabChild::RecvUpdateFrame(const FrameMet
>     }
>-        data += nsPrintfCString("{ \"width\" : %f", aFrameMetrics.mViewport.width);
>-        data += nsPrintfCString(", \"height\" : %f", aFrameMetrics.mViewport.height);
>+        data += nsPrintfCString("{ \"width\" : %f", (aFrameMetrics.mViewport.width / aFrameMetrics.mZoom.width));
>+        data += nsPrintfCString(", \"height\" : %f", (aFrameMetrics.mViewport.height / aFrameMetrics.mZoom.height));

This change isn't correct AFAICT.  mViewport is in CSS pixels, and all
the BrowserElementChild code expects to work in CSS pixels.

I'm not reviewing the rest of the patch because I'm not sure what this
change is trying to, and I think the rest of the patch depends on it.

Comment 24

[Benjamin Chen [:bechen]]

Attachment: 1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect 3. fix the relation between _isRectZoomedIn and AsyncPanZoomController::ZoomToRect

>This change isn't correct AFAICT.  mViewport is in CSS pixels, and all
>the BrowserElementChild code expects to work in CSS pixels.
Is it OK to pass the cssCompositedRect.width/height into BrowserElementScrolling.js ?
+ gfx::Rect cssCompositedRect =
+ AsyncPanZoomController::CalculateCompositedRectInCssPixels(aFrameMetrics);
+ data += nsPrintfCString(", \"cssCompositedRect\" : ");
+            data += nsPrintfCString("{ \"width\" : %f", cssCompositedRect.width);

- if (this._isRectZoomedIn(bRect, viewport))
+ if (this._isRectZoomedIn(bRect, this._cssCompositedRect))

The cssCompositedRect.width is exactly equal to (aFrameMetrics.mViewport.width/aFrameMetrics.mZoom.width).

If we don't bring the zoom information into method _isRectZoomedIn, it doesn't work. The symptom is that we can't zoom out if we double-tapping on an already zoom-in rect.

=====
>I understand what this change is intended to do, but what bug is it
>fixing?  This code is originally from Firefox on Android, so it would
>be better if we kept these heuristics the same for platform
>consistency.  Unless this is fixing a known bug ...

The original code logic in _isRectZoomedIn :

return (showing > 0.9 &&
dx > minDifference && dx < maxDifference &&
dw > minDifference && dw < maxDifference);

=> if the rect's width/height is very close to "viewport" (fab(dx)<20 fab(dw)<20), the _isRectZoomedIn return TRUE.

As I mentioned before, bring zoom information to _isRectZoomedIn make the "viewport" correct. But the method _isRectZoomedIn still doesn't work.
Because the code flow in AsyncPanZoomController::ZoomToRect L1242 1243 will try to
make the rect enlarge to fit the target aspect ratio.
But the problem is that when we double-tapping the same rect again (the expected action is zoom-out to original size)
Due to AsyncPanZoomController::ZoomToRect L1242~1243 enlarge the rect, but the _isRectZoomedIn doesn't know it since _isRectZoomedIn always get the original rect size by "let rect = ElementTouchHelper.getBoundingContentRect(element);"
So _isRectZoomedIn return false => AsyncPanZoomController::ZoomToRect again with the same rect.

Symptom: It looks like the target doesn't react to double-tapping.

For example: if the device width/height is 4/3 , target rect is 1/3.
AsyncPanZoomController::ZoomToRect L1242~1243 make the rect to 4/3.
But _isRectZoomedIn still uses 1/3 and 4/3 as method input and then return false. Another way to solve this is that before calling _isRectZoomedIn method, we add the same logic code corresponding to AsyncPanZoomController::ZoomToRect L1242~1243.  
One more reason I modify _isRectZoomedIn is that there is a constant inside regardless of the target resolution (cssCompositedRect)
"const minDifference = -20" 

=====
And also I found bug in AsyncPanZoomController::ZoomToRect L1214~1230.
The code may decrease the rect width/height temporarily and then adjust rect (x,y). Decrease the width/height make the (x,y) is incorrect.

Symptom : zoom content is cropped at first line.

=====
finally the targetZoom is a relative number to current zoom.

-    mEndZoomToMetrics.mZoom = gfxSize(targetZoom, targetZoom);
+    mEndZoomToMetrics.mZoom = gfxSize(targetZoom * mFrameMetrics.mZoom.width, targetZoom * mFrameMetrics.mZoom.height);

Symptom : zoom content is too large/small.

STR:
If the ideal zoom is 5, at beginning we pinch-zoom it to 10x
then
double-tapping: too small 0.5x
double-tapping: too large 10x
double-tapping: too small 0.5x
double-tapping: too large 10x
...

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Benjamin Chen from comment #24)
> Created attachment 684564 [details] [diff] [review]
> 1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio
> in AsyncPanZoomController::ZoomToRect 3. fix the relation between
> _isRectZoomedIn and AsyncPanZoomController::ZoomToRect
> 
> >This change isn't correct AFAICT.  mViewport is in CSS pixels, and all
> >the BrowserElementChild code expects to work in CSS pixels.
> Is it OK to pass the cssCompositedRect.width/height into
> BrowserElementScrolling.js ?

> The cssCompositedRect.width is exactly equal to
> (aFrameMetrics.mViewport.width/aFrameMetrics.mZoom.width).

That would be fine but the "CSS composited rect" is not the same size as the viewport in general.  For example, if a page uses

 <meta name="viewport" value="width=800">

and the user zooms in a factor of 4x, then the CSS viewport should be 1000 CSS pixels, and the CSS composited rect should be 200 CSS pixels.

> If we don't bring the zoom information into method _isRectZoomedIn, it
> doesn't work. The symptom is that we can't zoom out if we double-tapping on
> an already zoom-in rect.

OK, I suspect there's a unit or value being confused here.

We need to be very very careful about keeping our terminology and units precise.  It's very hard to read this kind of code after it's been written, and mismatched terminology makes it even harder ;).

I'm trying to follow the rest of your comment but it's making my head spin a little, sorry :(.  I'll try to re-read it a few more times in a little bit, but what might help is working through an example where you show the data flow for the values being changed here.  That should make it obvious which values are getting the wrong units, or not being adjusted correctly.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #25)
> (In reply to Benjamin Chen from comment #24)
> > Created attachment 684564 [details] [diff] [review]
> > 1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio
> > in AsyncPanZoomController::ZoomToRect 3. fix the relation between
> > _isRectZoomedIn and AsyncPanZoomController::ZoomToRect
> > 
> > >This change isn't correct AFAICT.  mViewport is in CSS pixels, and all
> > >the BrowserElementChild code expects to work in CSS pixels.
> > Is it OK to pass the cssCompositedRect.width/height into
> > BrowserElementScrolling.js ?
> 
> > The cssCompositedRect.width is exactly equal to
> > (aFrameMetrics.mViewport.width/aFrameMetrics.mZoom.width).
> 
> That would be fine but the "CSS composited rect" is not the same size as the
> viewport in general.  For example, if a page uses
> 
>  <meta name="viewport" value="width=800">
> 
> and the user zooms in a factor of 4x, then the CSS viewport should be 1000
> CSS pixels, and the CSS composited rect should be 200 CSS pixels.

Sorry!  "then the CSS viewport should be *800* CSS pixels"

Comment 27

[Benjamin Chen [:bechen]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #25)

> > Is it OK to pass the cssCompositedRect.width/height into
> > BrowserElementScrolling.js ?
> 
> > The cssCompositedRect.width is exactly equal to
> > (aFrameMetrics.mViewport.width/aFrameMetrics.mZoom.width).
> 
> That would be fine but the "CSS composited rect" is not the same size as the
> viewport in general.  For example, if a page uses
> 
>  <meta name="viewport" value="width=800">
> 
> and the user zooms in a factor of 4x, then the CSS viewport should be 800
> CSS pixels, and the CSS composited rect should be 200 CSS pixels.

cssCompositedRect.width is exactly equal to
(aFrameMetrics.mViewport.width /aFrameMetrics.mZoom.width).
200 = (800/4)

> > If we don't bring the zoom information into method _isRectZoomedIn, it
> > doesn't work. The symptom is that we can't zoom out if we double-tapping on
> > an already zoom-in rect.
> 
> OK, I suspect there's a unit or value being confused here.
> 
> We need to be very very careful about keeping our terminology and units
> precise.  It's very hard to read this kind of code after it's been written,
> and mismatched terminology makes it even harder ;).
> 
> I'm trying to follow the rest of your comment but it's making my head spin a
> little, sorry :(.  I'll try to re-read it a few more times in a little bit,
> but what might help is working through an example where you show the data
> flow for the values being changed here.  That should make it obvious which
> values are getting the wrong units, or not being adjusted correctly.

For example:
go to www.nytimes.com and double-tapping any text area
in BrowserElementScrolling.js::_recvDoubleTap
we dump the "this._viewport" (aFrameMetrics.mViewport.width)  {"left":0,"top":0,"right":980,"bottom":1270.93335}
The value "this._viewport doesn't changed by any zoom operation(gesture zoom in/out).
So the value "this._viewport" can't not be used for the input B
_isRectZoomedIn(A, B) 
The _isRectZoomedIn return TRUE if A is already zoom-in in B

That's why I said we need the zoom information "aFrameMetrics.mViewport.width/aFrameMetrics.mZoom.width" or _cssCompositedRect
pass it to input argument B.

===
After bring the zoom information into _isRectZoomedIn, we expected the _isRectZoomedIn will return TRUE if we doeble-tapping an already zoom-in rect.
And then trigger zoom-out. 
But the _isRectZoomedIn still returns FALSE in some trivial cases.

The point is that 

AsyncPanZoomController::ZoomToRect L1234 ~L1236
gfxFloat targetResolution =	
NS_MIN(compositionBounds.width / zoomToRect.width,
compositionBounds.height / zoomToRect.height);

and 

_isRectZoomedIn
    return (showing > 0.9 &&
            dx > minDifference && dx < maxDifference &&
            dw > minDifference && dw < maxDifference);

The AsyncPanZoomController::ZoomToRect considers width or height but the
_isRectZoomedIn only considers width!!

For example: if the device width/height is 40/30 , target rect is 1/3.
First double-tapping: zoom in is ok(10x) and now the _cssCompositedRect is (4,3)
Second double-tapping: It won't zoom out because the dx dw in 
_isRectZoomedIn can't match the condition check.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Benjamin Chen from comment #27)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #25)
> 
> > > Is it OK to pass the cssCompositedRect.width/height into
> > > BrowserElementScrolling.js ?
> > 
> > > The cssCompositedRect.width is exactly equal to
> > > (aFrameMetrics.mViewport.width/aFrameMetrics.mZoom.width).
> > 
> > That would be fine but the "CSS composited rect" is not the same size as the
> > viewport in general.  For example, if a page uses
> > 
> >  <meta name="viewport" value="width=800">
> > 
> > and the user zooms in a factor of 4x, then the CSS viewport should be 800
> > CSS pixels, and the CSS composited rect should be 200 CSS pixels.
> 
> cssCompositedRect.width is exactly equal to
> (aFrameMetrics.mViewport.width /aFrameMetrics.mZoom.width).
> 200 = (800/4)
> 

Ah, yes!  I totally missed the "/aFrameMetrics.mZoom.width" in your comment, sorry about that.  (I even fixed FrameMetrics so that everything worked out like that! ;) )

I prefer that we pass the zoom value into BrowserElementScrolling and let it compute what it needs based on the existing data, instead of passing cssCompositedRect.

> ===
> After bring the zoom information into _isRectZoomedIn, we expected the
> _isRectZoomedIn will return TRUE if we doeble-tapping an already zoom-in
> rect.
> And then trigger zoom-out. 
> But the _isRectZoomedIn still returns FALSE in some trivial cases.
> 
> The point is that 
> 
> AsyncPanZoomController::ZoomToRect L1234 ~L1236
> gfxFloat targetResolution =	
> NS_MIN(compositionBounds.width / zoomToRect.width,
> compositionBounds.height / zoomToRect.height);
> 
> and 
> 
> _isRectZoomedIn
>     return (showing > 0.9 &&
>             dx > minDifference && dx < maxDifference &&
>             dw > minDifference && dw < maxDifference);
> 
> The AsyncPanZoomController::ZoomToRect considers width or height but the
> _isRectZoomedIn only considers width!!
> 
> For example: if the device width/height is 40/30 , target rect is 1/3.
> First double-tapping: zoom in is ok(10x) and now the _cssCompositedRect is
> (4,3)
> Second double-tapping: It won't zoom out because the dx dw in 
> _isRectZoomedIn can't match the condition check.

Thanks for the example.  I think I understand what's going on here now.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 684564 [details] [diff] [review]
1. bring the zoom factor into BrowserElementScroll.js 2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect 3. fix the relation between _isRectZoomedIn and AsyncPanZoomController::ZoomToRect

Actually, I like this patch better than passing in FrameMetrics.mZoom.  Let's do it.

Comment 30

[Benjamin Chen [:bechen]]

When doing the final review, I found there's a mistake in my patch.

gfxFloat targetZoom = clamped(float(targetResolution / resolution.width),mMinZoom, mMaxZoom);
mEndZoomToMetrics.mZoom = gfxSize(targetZoom * mFrameMetrics.mZoom.width, targetZoom * mFrameMetrics.mZoom.height);

The clamped macro guarantee the targetZoom value is between mMinZoom and mMaxZoom. It should be called at last.

Hence the correct code:

gfxFloat targetZoom = clamped(float(targetResolution / resolution.width) * mFrameMetrics.mZoom.width
,mMinZoom, mMaxZoom);

mEndZoomToMetrics.mZoom = gfxSize(targetZoom , targetZoom);

After modify the code, I found a buggy scenario about mMaxZoom mMinZoom.
If there's a zoom limitation, there should be some code to do that. I can't find it neither BrowserElementScrolling.js nor AsyncPanZoomController::ZoomToRect.

Assume the mMaxZoom is 10x. The target rect's ideal zoom is 20x
1. First double-tapping: zoom in to 10x
2. Second double-tapping: can't zoom-out
The expected result of step 2 is zoom-out if we reach the maxZoom.
Because the limitation by mMaxZoom in AsyncPanZoomController, _isRectZoomedIn still tries to zoom the rect since the target rect is not large enough(20x).

There are 2 ways in my thought that may resolve this:
1. BrowserElementScrolling.js need to know the mMaxZoom and mMinZoom to decide zoom-in or zoom-out
1.1 ipc code to get the  AsyncPanZoomController::mMaxZoom? 
1.2 TabChild::RecvUpdateFrame(const FrameMetrics& aFrameMetrics) add fields in FrameMetrics?

2. do something in AsyncPanZoomController::ZoomToRect
2.1 use the "final targetZoom" and current zoom "mFrameMetrics.mZoom.width" and
mFrameMetrics.mScrollOffset.x mFrameMetrics.mScrollOffset.y

Apparently the second way is more feasible and easier?

Note that we need to consider the corner cases such as some rect's ideal zoom ratio is just exactly equal to mMaxZoom.

I'll try to fix the scenario on my next patch.

Comment 31

[Benjamin Chen [:bechen]]

Attachment: Fix browser zoom issues.

1. bring the zoom factor into BrowserElementScrollng.js
2. fix the zoom ratio in AsyncPanZoomController::ZoomToRect
3. fix the relation between _isRectZoomedIn and AsyncPanZoomController::ZoomToRect
4. zoom it out if the rect reach the device maximal zoom ratio

carry on r+ after addressing comment 30.

Comment 32

[Benjamin Chen [:bechen]]

try server:
https://tbpl.mozilla.org/?tree=Try&rev=b24c17ddfad2

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 686030 [details] [diff] [review]
Fix browser zoom issues.

[Approval Request Comment]
User impact if declined: Weird zooming behavior in the B2G browser
Risk to taking this patch (and alternatives if risky): Low to medium.
String or UUID changes made by this patch: None.

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83bf684a81e9

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83bf684a81e9

Comment 36

[Alex Keybl [:akeybl]]

(In reply to Tim Taubert [:ttaubert] from comment #33)
> Comment on attachment 686030 [details] [diff] [review]
> Fix browser zoom issues.
> 
> [Approval Request Comment]
> User impact if declined: Weird zooming behavior in the B2G browser
> Risk to taking this patch (and alternatives if risky): Low to medium.
> String or UUID changes made by this patch: None.

Hi Tim - can you be more explicit with the risk to taking this patch, specifically with the risk of regression to B2G versus Desktop/Android? Any guidance for QA on how to test for regressions would also be very helpful.

Comment 37

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

There's zero (well \epsilon) risk for all other products.  None of this code runs there.

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 686030 [details] [diff] [review]
Fix browser zoom issues.

Approving for branches based on very small likelihood of effect on other products as stated by cjones.

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/269946c84e11
https://hg.mozilla.org/releases/mozilla-beta/rev/a82e1a6acbb6