Closed Bug 824920 Opened 8 years ago Closed 7 years ago

[Browser] double-tap fails to zoom out after double-tap zooming in

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:tef+, blocking-basecamp:-, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
blocking-basecamp -
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: cyu, Assigned: bechen)

References

Details

Attachments

(1 file, 5 obsolete files)

Forked from bug 817127 comment 4. STR:

 1. Visit http://pastebin.mozilla.org/
 2. Double-tap the large-ish footer text ("Mozilla systems and collaborative tools..."
  --> Browser zooms in to that text.
 3. Double-tap it again

Expected result: Browser zooms out.
Actual result: Browser doesn't zoom out.
BB? since it's a breakage of double-tap zooming in the browser app.
blocking-basecamp: --- → ?
I think it looks like a bb+ case.
blocking-basecamp: ? → +
Kinda annoying, but two simple workarounds are pinch-zooming out and double-tapping just outside the text.
blocking-basecamp: + → ?
Assignee: nobody → bechen
1. fix the zoom ratio if double-tapping on a rect closed to boundary.
2. Adjust the offset to prevent over-scrolled
Attachment #696257 - Flags: feedback?(jones.chris.g)
given comment 3, blocking-minus.  Please request uplift if the patch is safe.
blocking-basecamp: ? → -
tracking-b2g18: --- → +
Comment on attachment 696257 [details] [diff] [review]
1.fix zoom ration 2.check the zoom offset

Doug, do you have time to look this over?  If not let me know.
Attachment #696257 - Flags: feedback?(jones.chris.g) → feedback?(bugzilla)
Comment on attachment 696257 [details] [diff] [review]
1.fix zoom ration 2.check the zoom offset

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

Have you tried this with oddly-dimensioned (such as very wide and device-height tall) and very small pages? Something here feels kinda fishy and I can't put a finger on it. I think it's the fact that we're intersecting the zoomToRect with the CSS page rect earlier. It could be perfectly valid, though.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1256,5 @@
>                               y + dh/2,
>                               cssPageRect.width,
>                               y + dh/2 + newHeight);
> +      zoomToRect = zoomToRect.Intersect(cssPageRect);
> +      mEndZoomToMetrics.mZoom = gfxSize(1, 1);

Does this actually work? I can't remember off the top of my head, but I'm pretty sure a zoom of 1 doesn't correspond to the page size (smaller of the height or width with regards to the composition bounds).

@@ +1264,5 @@
> +
> +      mAnimationStartTime = TimeStamp::Now();
> +
> +      ScheduleComposite();
> +      return;

Is there a good reason to short-circuit the code here instead of using if blocks? Normally I'd be fine with it but there's 8 lines (or 6 if you don't count whitespace) of code that is now duplicated here and at the end.

@@ -1270,5 @@
> -    zoomToRect = zoomToRect.Intersect(cssPageRect);
> -
> -    // Do one final recalculation to get the resolution.
> -    targetResolution = NS_MAX(compositionBounds.width / zoomToRect.width,
> -                              compositionBounds.height / zoomToRect.height);

I'm wary of removing this stuff. This was supposed to make sure that we didn't go over bounds on one dimension, but at the same time, let us zoom in as much as possible. With this patch, do we still zoom in the proper amount?

@@ +1276,4 @@
>      float targetZoom = float(targetResolution / resolution.width) * mFrameMetrics.mZoom.width;
>  
>      // If current zoom is equal to mMaxZoom,
>      // user still double-tapping it, just zoom-out to the full page size

Why was this code ever duplicated like this? (not relevant to this patch)

@@ +1293,5 @@
>                               cssPageRect.width,
>                               y + dh/2 + newHeight);
>  
>        zoomToRect = zoomToRect.Intersect(cssPageRect);
>        // assign 1 to targetZoom is a shortcut

Comment is out of date

@@ +1300,5 @@
> +      gfxFloat targetFinalZoom = clamped(targetZoom, mMinZoom, mMaxZoom);
> +      mEndZoomToMetrics.mZoom = gfxSize(targetFinalZoom, targetFinalZoom);
> +      // we are trying to adjust the zoomToRect.x zoomToRect.y into
> +      // sensible position to prevent over-scrolled.
> +      FrameMetrics temp = mFrameMetrics;

Don't name variables "temp"

@@ +1308,5 @@
> +      // if (zoomToRect.y + rectAfterZoom.height > cssPageRect.heigh)
> +      // that means the offset will over-scrolled after zoomed.
> +      if(zoomToRect.y + rectAfterZoom.height > cssPageRect.height) {
> +        zoomToRect.y = cssPageRect.height - rectAfterZoom.height;
> +        zoomToRect.y > 0 ? zoomToRect.y : 0;

Meaningless statement. Maybe you meant:
zoomToRect.y = zoomToRect.y > 0 ? zoomToRect.y : 0;
?

@@ +1312,5 @@
> +        zoomToRect.y > 0 ? zoomToRect.y : 0;
> +      }
> +      if(zoomToRect.x + rectAfterZoom.width > cssPageRect.width) {
> +        zoomToRect.x = cssPageRect.width - rectAfterZoom.width;
> +        zoomToRect.x > 0 ? zoomToRect.x : 0;

Meaningless statement.
Attachment #696257 - Flags: feedback?(bugzilla)
> Have you tried this with oddly-dimensioned (such as very wide and
> device-height  tall) and very small pages?
Do you know where I can easily find the oddly-dimensioned pages? Especially for very small pages.

>> +      zoomToRect = zoomToRect.Intersect(cssPageRect);
>> +      mEndZoomToMetrics.mZoom = gfxSize(1, 1);

> Does this actually work? I can't remember off the top of my head, but I'm 
> pretty sure a zoom of 1 doesn't correspond to the page size (smaller of the 
> height or width with regards to the composition bounds).
You are right. zoom 1 is not the original page size.
So I modify this in this patch.

>> -    // Recalculate the zoom to rect using the new dimensions.
>> -    zoomToRect.width = compositionBounds.width / targetResolution;
>> -    zoomToRect.height = compositionBounds.height / targetResolution;
>> -    zoomToRect = zoomToRect.Intersect(cssPageRect);
>> -    // Do one final recalculation to get the resolution.
>> -    targetResolution = NS_MAX(compositionBounds.width / zoomToRect.width,
>> -                              compositionBounds.height / zoomToRect.height);

> I'm wary of removing this stuff. This was supposed to make sure that we 
> didn't go over bounds on one dimension, but at the same time, let us zoom in > as much as possible. With this patch, do we still zoom in the proper amount?

The code must be removed because it's root cause of the bug. bug 817127 comment 4 .
And the zoom factor is ok that we can only calculate once.
Attachment #696257 - Attachment is obsolete: true
Attachment #697381 - Flags: feedback?(bugzilla)
version 3: 
I add |float localZoomMin| to prevent the over-zoom-out.
The value is computed by |CalculateCompositedRectInCssPixels|
|mFrameMetrics.mZoom| |cssPageRect|.
If the zoom ratio is lower than localZoomMin, there will be white band on the edge after zoomed.

The over-zoom-out occurs when user double-tapping on a rect which width/height ratio is small then device. And also the rect size is very large.

example: zoom factor is 1, device w/h(300/400), content w/h(300/600), content rect w/h(300/500)
1. double-tapping on the rect
2. the zoom ratio after computed is 0.8 (400/500)
3. content width is not enough so there is white on the edge

STR:
1. nytimes.com
2. double-tapping on the narrow rect which has no content(white) and below the rect("COMPANY INFO about NYT Co. About IHT Advertise") on the most left side.
Attachment #697381 - Attachment is obsolete: true
Attachment #697381 - Flags: feedback?(bugzilla)
Attachment #698550 - Flags: feedback?(bugzilla)
(In reply to Benjamin Chen from comment #8)
> Do you know where I can easily find the oddly-dimensioned pages? Especially
> for very small pages.

I don't know of any test pages for this. When I was working on this, I just created simple pages and messed around with the meta viewport tag.
Comment on attachment 698550 [details] [diff] [review]
1.fix zoom ratio 2.check the zoom offset

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

>The over-zoom-out occurs when user double-tapping on a rect which width/height ratio is small then device. And also the rect size is very large.

Yeah, this is what I was worried about before when removing that big block.

Looks fine to me now, I'd give it another read-through for review though.
Attachment #698550 - Flags: feedback?(bugzilla) → feedback+
re-based version, no logic change.
Attachment #698550 - Attachment is obsolete: true
Attachment #702678 - Flags: review?(bugzilla)
Comment on attachment 702678 [details] [diff] [review]
1.fix zoom ration 2.check the zoom offset

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

Cool, mostly comment nits but a couple of functionality problems. I think it's important that comments are easy to parse in people's heads so I reworded some things slightly.

I'd still like some tests for this, even if they're just pages you can point your browser to and check with until we have proper automated testing for APZC.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1322,5 @@
>      nsIntRect compositionBounds = mFrameMetrics.mCompositionBounds;
>      gfx::Rect cssPageRect = mFrameMetrics.mScrollableRect;
>      gfx::Point scrollOffset = mFrameMetrics.mScrollOffset;
>      gfxSize resolution = CalculateResolution(mFrameMetrics);
> +    gfxSize userZoom = mFrameMetrics.mZoom;

I'd prefer "currentZoom" or something like that.

@@ +1329,3 @@
>  
> +    // localMinZoom: a minimal zoom ratio to prevent over-zoom-out.
> +    // If the zoom factor is lower than it, there is white band on the edge.

// The minimum zoom to prevent over-zoom-out.
// If the zoom is lower than this, the CSS content rect layers
// pixels will be smaller than the composition bounds.

(Note that the "white band" comment isn't necessarily true)

@@ +1333,5 @@
> +    gfx::Rect compositedRect = CalculateCompositedRectInCssPixels(mFrameMetrics);
> +    localMinZoom =
> +      std::max(userZoom.width / (cssPageRect.width / compositedRect.width),
> +               userZoom.height / (cssPageRect.height / compositedRect.height));
> +    localMinZoom = std::max(localMinZoom, mMinZoom);

My understanding is that, in general, we should use our own implementations of functions instead of std's or the STL's. AFAIK, there's nothing wrong with it, but it should be avoided if possible. In this case, I'd prefer using NS_MAX().

@@ +1336,5 @@
> +               userZoom.height / (cssPageRect.height / compositedRect.height));
> +    localMinZoom = std::max(localMinZoom, mMinZoom);
> +
> +    if (!zoomToRect.IsEmpty()) {
> +      // Clamp the zoom to rect to the CSS rect to make sure it fits.

This wording is really confusing, but it's actually my fault.

// Intersect the zoom-to-rect to the CSS rect to make sure it fits.

@@ +1339,5 @@
> +    if (!zoomToRect.IsEmpty()) {
> +      // Clamp the zoom to rect to the CSS rect to make sure it fits.
> +      zoomToRect = zoomToRect.Intersect(cssPageRect);
> +      targetResolution =
> +        std::min(compositionBounds.width / zoomToRect.width,

Same comment as above.

@@ +1343,5 @@
> +        std::min(compositionBounds.width / zoomToRect.width,
> +                 compositionBounds.height / zoomToRect.height);
> +      targetZoom = float(targetResolution / resolution.width) * userZoom.width;
> +    }
> +    // 1. If the rect is empty, request received from browserElementScrolling.js

I don't understand this comment. Unless the functionality has changed since I last touched it, a zoom-to-rect that is empty signals a request to zoom out as much as possible without over-zooming (if you want to call it that).

@@ +1366,5 @@
> +                             newHeight);
> +      zoomToRect = zoomToRect.Intersect(cssPageRect);
> +      targetResolution =
> +        std::min(compositionBounds.width / zoomToRect.width,
> +                 compositionBounds.height / zoomToRect.height);

Same comment as above.

@@ +1376,3 @@
>  
> +    // we are trying to adjust the zoomToRect.x zoomToRect.y into
> +    // sensible position to prevent over-scrolled.

// Adjust the zoomToRect to a sensible position to prevent overscrolling.

@@ +1376,5 @@
>  
> +    // we are trying to adjust the zoomToRect.x zoomToRect.y into
> +    // sensible position to prevent over-scrolled.
> +    // if (zoomToRect.y + rectAfterZoom.height > cssPageRect.heigh)
> +    // that means the offset will over-scrolled after zoomed.

Move this comment down to the if blocks and make it:

// If either of these conditions are met, the page will be
// overscrolled after zoomed.
Attachment #702678 - Flags: review?(bugzilla)
(In reply to Doug Sherk (:drs) (:dRdR) from comment #13)

> @@ +1333,5 @@
> > +    gfx::Rect compositedRect = CalculateCompositedRectInCssPixels(mFrameMetrics);
> > +    localMinZoom =
> > +      std::max(userZoom.width / (cssPageRect.width / compositedRect.width),
> > +               userZoom.height / (cssPageRect.height / compositedRect.height));
> > +    localMinZoom = std::max(localMinZoom, mMinZoom);
> 
> My understanding is that, in general, we should use our own implementations
> of functions instead of std's or the STL's. AFAIK, there's nothing wrong
> with it, but it should be avoided if possible. In this case, I'd prefer
> using NS_MAX().
> 
> @@ +1336,5 @@

bug 786533 replace the NS_MIN NS_MAX to std::min std::max,
looks like there are some compatibility issues for float and double,  please see bug 785542 comment 3.
(In reply to Doug Sherk (:drs) (:dRdR) from comment #13)
 
> @@ +1343,5 @@
> > +        std::min(compositionBounds.width / zoomToRect.width,
> > +                 compositionBounds.height / zoomToRect.height);
> > +      targetZoom = float(targetResolution / resolution.width) * userZoom.width;
> > +    }
> > +    // 1. If the rect is empty, request received from browserElementScrolling.js
> 
> I don't understand this comment. Unless the functionality has changed since
> I last touched it, a zoom-to-rect that is empty signals a request to zoom
> out as much as possible without over-zooming (if you want to call it that).
> 

The functionality for empty rect does not change. APZC still handles this case as a zoom out request.
I just want to comment the empty rect request is sent from browserElementScrolling.js _zoomOut method.
Severity: normal → blocker
Priority: -- → P1
Can someone please update the latest status on this.
- modify some comments, no logic change
Attachment #702678 - Attachment is obsolete: true
Attachment #721642 - Flags: review?(bugzilla)
QC requested this to block.
blocking-b2g: --- → tef+
Comment on attachment 721642 [details] [diff] [review]
1.fix zoom ratio 2.check the zoom offset

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

Cool, just one nit:

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1354,4 @@
>  
> +    // The minimum zoom to prevent over-zoom-out.
> +    // If the zoom is lower than this, the CSS content rect layers
> +    // pixels will be smaller than the composition bounds.

This is actually my fault, but I want to reword it again:

// If the zoom factor is lower than this (i.e. we are zoomed more into the page),
// then the CSS content rect, in layers pixels, will be smaller than the
// composition bounds. If this happens, we can't fill the target composited area
// with this frame.
Attachment #721642 - Flags: review?(bugzilla) → review+
Attachment #721642 - Attachment is obsolete: true
Attachment #723391 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e0366a64dfc
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
This issue no longer reproduces on Unagi device.
Build ID: 20130313070202
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e74dafa6b2d9
Gaia: b34e726147f8e671ad8c538b50900ccfbffcb084

Browser zooms in and zooms out, when user double-taps the text.
Verified fixed.
Status: RESOLVED → VERIFIED
Is this fixed in 1.1? It seems browser tap to zoom in zoom out is broken in 1.1. Is there any existing for that\has someone verified recently.
blocking-b2g: tef+ → leo?
Flags: needinfo?
moving back to tef+
tef+ means leo will also get the fix.
blocking-b2g: leo? → tef+
Flags: needinfo?
You need to log in before you can comment on or make changes to this bug.