720538 - Double tap on iframe causes overflow area at bottom to appear permanently

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, steps to reproduce:
- Double tap on the iframe on the top left (dashed blue box)
- Double tap again on the iframe on the top left

Expected result:
- Whole page is shown, no overflow scroll area shown at the bottom of the page

Actual result:
- Overflow scroll area shown at the bottom of the page

Comment 1

[Martijn Wargers (dead)]

Attachment: Screenshot of bug

This was tested on the LG Optimus Black, Android 2.2.2.

Comment 2

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

Attachment: (1/3) Clarify and fix zoom rect when zooming to page width

Comment 3

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

Attachment: (2/3) Ensure the aspect ratio of the viewport is maintained, and it gets validated

This is the main part of the fix. Running the viewport through getValidViewportMetrics ensures that the overscroll is taken out.

Comment 4

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

Attachment: (3/3) Cleanup - remove unused things

Comment 5

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

Note that with these patches and the STR in comment #0, when we zoom out after the second double-tap, we zoom out to the center of the page horizontally, rather than the top-left. This is intentional and I expect it to behave better than zooming to top-left for most pages on which this problem occurs.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 592990 [details] [diff] [review]
(2/3) Ensure the aspect ratio of the viewport is maintained, and it gets validated

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

::: mobile/android/base/ui/PanZoomController.java
@@ +855,5 @@
>      private boolean animatedZoomTo(RectF zoomToRect) {
>          GeckoApp.mAppContext.hidePluginViews();
>          GeckoApp.mAppContext.mAutoCompletePopup.hide();
>  
> +        Log.i(LOGTAG, "Doing animated zoom to rect " + zoomToRect + " in state " + mState);

I don't really want more logging, but feel free to keep this if you think we need it for some reaason.

Comment 7

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

Attachment: (4/3) Add a regression test in robocop

Not really sure who should review this but I figure joel/geoff are more familiar with the functions I'm using to replicate the STR. First actual regression test in robocop!

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 593904 [details] [diff] [review]
(4/3) Add a regression test in robocop

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

The robocop bits look great.  I assume this tests the bug as it appears to.

::: mobile/android/base/tests/test_bug720538.java.in
@@ +27,5 @@
> +        painted = mDriver.getPaintedSurface();
> +        mAsserter.ispixel(painted[100][0], 0, 0x80, 0, "Checking page background to the left of the iframe");
> +        mAsserter.ispixel(painted[100][50], 0, 0, 0xFF, "Checking for iframe a few pixels from the left edge");
> +        mAsserter.ispixel(painted[100][mDriver.getGeckoWidth() - 51], 0, 0, 0xFF, "Checking for iframe a few pixels from the right edge");
> +        mAsserter.ispixel(painted[100][mDriver.getGeckoWidth() - 1], 0, 0x80, 0, "Checking page background the right of the iframe");

how do you know the values at each of these locations?  Can you add a comment to help figure it out if we need to debug in the future?

Comment 9

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

(In reply to Joel Maher (:jmaher) from comment #8)
> 
> how do you know the values at each of these locations?  Can you add a
> comment to help figure it out if we need to debug in the future?

The page we're rendering is simple (has only two colors) and well-known (a slightly modified version of the testcase :mw22 provided), so we can know what it's going to look like when it renders, and whether particular pixels will be green or blue. But yeah, I'll add some comments indicating how I got those values into the test.

Comment 10

[Geoff Brown [:gbrown]]

Comment on attachment 593904 [details] [diff] [review]
(4/3) Add a regression test in robocop

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

Nice to see you putting robocop to good use!

::: mobile/android/base/tests/test_bug720538.java.in
@@ +8,5 @@
> +    private static final String URL = "http://mochi.test:8888/tests/robocop/test_bug720538.html";
> +
> +    public void test_bug720538() {
> +        Actions.RepeatedEventExpecter paintExpecter = mActions.expectPaint();
> +        loadUrl(URL);

We'll want to use the new getAbsoluteUrl() introduced in bug 717057 here.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c30546ac97f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a3c03e50dd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e413c6c3da40
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b30c1c883f2

Comment 12

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

(In reply to Wesley Johnston (:wesj) from comment #6)
> I don't really want more logging, but feel free to keep this if you think we
> need it for some reaason.

I took out the logging on landing; it was left in from when I was debugging.

(In reply to Joel Maher (:jmaher) from comment #8)
> how do you know the values at each of these locations?  Can you add a
> comment to help figure it out if we need to debug in the future?

Add a comment explaining the values in the patch I landed.

(In reply to Geoff Brown [:gbrown] from comment #10)
> 
> We'll want to use the new getAbsoluteUrl() introduced in bug 717057 here.

Yeah, thanks for the reminder :) I updated the patch to set the test type and use the absolute url prior to landing.

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1c30546ac97f
https://hg.mozilla.org/mozilla-central/rev/4a3c03e50dd1
https://hg.mozilla.org/mozilla-central/rev/e413c6c3da40
https://hg.mozilla.org/mozilla-central/rev/3b30c1c883f2

Comment 16

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

Comment on attachment 592988 [details] [diff] [review]
(1/3) Clarify and fix zoom rect when zooming to page width

[Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: double-tapping on blocks can sometimes leave the page with a bad viewport (zoomed in beyond limits, showing overscroll, etc). it's mostly a polish issue.
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): double-tap zoom behaviour may be changed to be less effective. mobile-only change
String changes made by this patch: none

Comment 17

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

Not requesting aurora/beta approval for the regression test patch because that depends on all sorts of robocop patches that haven't been uplifted.

Comment 18

[Alex Keybl [:akeybl]]

Comment on attachment 592988 [details] [diff] [review]
(1/3) Clarify and fix zoom rect when zooming to page width

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.

Comment 19

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ed47bdb36762
https://hg.mozilla.org/releases/mozilla-aurora/rev/1b5272d0974c
https://hg.mozilla.org/releases/mozilla-aurora/rev/a21629312590
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb8140c09c30

Comment 20

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

(In reply to Mark Finkle (:mfinkle) from comment #19)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/cb8140c09c30

The incorrect regression test landing on aurora was backed out in:
https://hg.mozilla.org/releases/mozilla-aurora/rev/1e51e4d8d70b

Comment 21

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

https://hg.mozilla.org/releases/mozilla-beta/rev/92ab01832190

Comment 22

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

https://hg.mozilla.org/releases/mozilla-beta/rev/92ab01832190
https://hg.mozilla.org/releases/mozilla-beta/rev/b90d93b8ff98
https://hg.mozilla.org/releases/mozilla-beta/rev/3ae1c247bfa8

Comment 23

[Andreea Pod]

Verified fixed on builds:
Firefox 11 (tinderbox build): 	1328588649/	06-Feb-2012 21:51 		
Firefox 12: Firefox 12.0a2 (2012-02-07)
Firefox 13: Firefox 13.0a1 (2012-02-07)

Device: LG Optimus 2X (Android 2.2.2)