Closed Bug 761721 Opened 10 years ago Closed 10 years ago

double-tap zoom/unzoom completely changes the focal point, sometimes

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox14 verified, firefox15 verified, firefox16 verified, blocking-fennec1.0 .N+, fennec15+)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox14 --- verified
firefox15 --- verified
firefox16 --- verified
blocking-fennec1.0 --- .N+
fennec 15+ ---

People

(Reporter: luke, Assigned: kats)

References

()

Details

Attachments

(2 files)

I've been seeing this for a few weeks on nightlies.  Unfortunately, I wasn't able to get an exact STR, but I do have a relatively simple site where I see it pretty easily and reproducibly:
 1. go to http://www.groklaw.net/article.php?story=20120604161143147
 2. scroll down a ways (say to Sixth Claim for Relief)
 3. repeatedly double tap in the middle of the body text, but on different positions each time
Mostly, it will zoom in and out of the main body div exactly like you'd expect.  But every one to four times, it will scroll almost all the way to the top of the page.

This behavior makes me think this isn't just a UI heuristic going wrong but a more fundamental bug.
A specific place you can double-tap to see this is on the paragraph numbered 22 ("Oracle realleges and..."). The reason this happens is because the page is using table-based layouts and the closest block ancestor of the text in question is a <td> that basically encompasses the entire article text.
blocking-fennec1.0: --- → ?
Yeah. We've had this discussion before and decided not to block on it. But mostly because its been pretty odd to run into. If people feel like its happening on real sites frequently enough to be annoying, I can look into a fix.

We probably need to adjust the heuristic so that, if the nearest block level element is far away we just zoom to fit what's visible instead.... or something like that.
tracking-fennec: --- → 15+
blocking-fennec1.0: ? → .N+
(In reply to Kartikaya Gupta (:kats) from comment #1)
Ah, that makes sense, thanks for explaining.

(In reply to Wesley Johnston (:wesj) from comment #2)
> Yeah. We've had this discussion before and decided not to block on it. But
> mostly because its been pretty odd to run into. If people feel like its
> happening on real sites frequently enough to be annoying, I can look into a
> fix.

If it helps, in my experience, this isn't rare: I mostly use Fennec to browse news sites linked from Hacker News and see this several times a day, not all from the same site.
Assignee: nobody → bugmail.mozilla
Comment on attachment 633626 [details] [diff] [review]
(1/2) - Cleanup for readability, no functional changes

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

::: mobile/android/chrome/content/browser.js
@@ +2660,5 @@
>    _zoomOut: function() {
>      sendMessageToJava({ gecko: { type: "Browser:ZoomToPageWidth"} });
>    },
>  
> +  _rectAlreadyZoomedIn: function(aRect, aViewport) {

Nit. Can we name this something like _isRectVisible

@@ +2668,5 @@
> +    let vRect = new Rect(aViewport.cssX, aViewport.cssY, aViewport.cssWidth, aViewport.cssHeight);
> +    let overlap = vRect.intersect(aRect);
> +    let overlapArea = overlap.width * overlap.height;
> +    let availHeight = Math.min(aRect.width * vRect.height / vRect.width, aRect.height);
> +    let showing = overlapArea / (aRect.width * availHeight);

We've moved the (not so great) comments we had explaining this code to where its called instead. Can you add something here to briefly explain what this  math does. Namely, we're worried that the amount of the rect visible on screen (overlapArea) is ~= the max area we could show on screen (the rect's width * min of the viewport height or the rect's height), and that the rect is actually on screen (i.e. x is near the left and x+w is near the right).
Attachment #633626 - Flags: review?(wjohnston) → review+
Attachment #633627 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #6)
> Nit. Can we name this something like _isRectVisible
> 

As per IRC discussion I renamed this to _isRectZoomedIn.

> We've moved the (not so great) comments we had explaining this code to where
> its called instead. Can you add something here to briefly explain what this 
> math does. Namely, we're worried that the amount of the rect visible on
> screen (overlapArea) is ~= the max area we could show on screen (the rect's
> width * min of the viewport height or the rect's height), and that the rect
> is actually on screen (i.e. x is near the left and x+w is near the right).

Added a bunch of comments to the function explaining more in detail what it does.

Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8d922913e30
https://hg.mozilla.org/integration/mozilla-inbound/rev/47859f893a99
Target Milestone: --- → Firefox 16
Comment on attachment 633626 [details] [diff] [review]
(1/2) - Cleanup for readability, no functional changes

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: double-tapping to zoom out on some pages unexpectedly takes you back to the top of the page (or some other part of the page farther up)
Testing completed (on m-c, etc.): landed on inbound june 18, nom'ing early to get it on the radar
Risk to taking this patch (and alternatives if risky): mobile-only, low risk. the second patch alone should be sufficient to fix the issue; the first one is refactoring only.
String or UUID changes made by this patch: none
Attachment #633626 - Flags: approval-mozilla-beta?
Attachment #633626 - Flags: approval-mozilla-aurora?
Comment on attachment 633627 [details] [diff] [review]
(2/2) - Adjust y-coord of zoom rect so we don't fly to top of page

[Approval Request Comment]
see above
Attachment #633627 - Flags: approval-mozilla-beta?
Attachment #633627 - Flags: approval-mozilla-aurora?
Comment on attachment 633626 [details] [diff] [review]
(1/2) - Cleanup for readability, no functional changes

Please land on mozilla-aurora and the default branch of mozilla-beta but not the beta 7 release branch of mozilla-beta
Attachment #633626 - Flags: approval-mozilla-beta?
Attachment #633626 - Flags: approval-mozilla-beta+
Attachment #633626 - Flags: approval-mozilla-aurora?
Attachment #633626 - Flags: approval-mozilla-aurora+
Comment on attachment 633627 [details] [diff] [review]
(2/2) - Adjust y-coord of zoom rect so we don't fly to top of page

Please land on mozilla-aurora and the default branch of mozilla-beta but not the beta 7 release branch of mozilla-beta
Attachment #633627 - Flags: approval-mozilla-beta?
Attachment #633627 - Flags: approval-mozilla-beta+
Attachment #633627 - Flags: approval-mozilla-aurora?
Attachment #633627 - Flags: approval-mozilla-aurora+
Unable to reproduce the issue as described on Aurora 15.0a2 2012-06-19. The fix has not made it into Nightly 2012-06-19 yet. Leaving open for retest on a later build.

Tested using:
HTC Desire
Android 2.2
No longer blocks: 741405
Duplicate of this bug: 741405
No longer blocks: 765703
Duplicate of this bug: 765703
Please also verify the two bugs I duped to this one to ensure they are fixed as well. Thanks!
Bug 741405 and Bug 765703 are both fixed on Aurora 15.0a2 2012-06-19 but are reproducible on Nightly 16.0a1 2012-06-19.
The two bugs Bug 741405 and Bug 765703 are not reproducible anymore on Nightly 14.0a1 2012-06-25 but they are still reproducible on Firefox Beta 8.
Not reproducible anymore on Firefox 14.0b10
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.