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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Luke Wagner [:luke]]

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.

Comment 1

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

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.

Comment 2

[Wesley Johnston (:wesj)]

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.

Comment 3

[Luke Wagner [:luke]]

(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.

Comment 4

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

Attachment: (1/2) - Cleanup for readability, no functional changes

Comment 5

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

Attachment: (2/2) - Adjust y-coord of zoom rect so we don't fly to top of page

Comment 6

[Wesley Johnston (:wesj)]

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).

Comment 7

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

(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

Comment 8

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

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

Comment 9

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

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

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 12

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

Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/78bd8749ed97
https://hg.mozilla.org/releases/mozilla-aurora/rev/04e8736653ff

Beta (default branch, had a small rebase because bug 754637 hasn't been uplifted):
https://hg.mozilla.org/releases/mozilla-beta/rev/3faba8c0856b
https://hg.mozilla.org/releases/mozilla-beta/rev/cbcca3a61368

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c8d922913e30
https://hg.mozilla.org/mozilla-central/rev/47859f893a99

Comment 14

[Adrian Tamas (:AdrianT)]

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

Comment 17

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

Please also verify the two bugs I duped to this one to ensure they are fixed as well. Thanks!

Comment 18

[Adrian Tamas (:AdrianT)]

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.

Comment 19

[Paul Feher]

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.

Comment 20

[Paul Feher]

Not reproducible anymore on Firefox 14.0b10