Closed Bug 720538 Opened 12 years ago Closed 12 years ago

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

Categories

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

ARM
Android
defect

Tracking

(firefox11 verified, firefox12 verified, firefox13 verified, fennec11+)

VERIFIED FIXED
Firefox 13
Tracking Status
firefox11 --- verified
firefox12 --- verified
firefox13 --- verified
fennec 11+ ---

People

(Reporter: martijn.martijn, Assigned: kats)

References

()

Details

(Keywords: testcase)

Attachments

(6 files)

Attached file 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
Attached image Screenshot of bug
This was tested on the LG Optimus Black, Android 2.2.2.
Assignee: nobody → bugmail.mozilla
tracking-fennec: --- → 11+
Priority: -- → P3
This is the main part of the fix. Running the viewport through getValidViewportMetrics ensures that the overscroll is taken out.
Attachment #592990 - Flags: review?(wjohnston)
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.
Attachment #592988 - Flags: review?(wjohnston) → review+
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.
Attachment #592990 - Flags: review?(wjohnston) → review+
Attachment #592991 - Flags: review?(wjohnston) → review+
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!
Attachment #593904 - Flags: review?(jmaher)
Attachment #593904 - Flags: feedback?(wjohnston)
Attachment #593904 - Flags: feedback?(gbrown)
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?
Attachment #593904 - Flags: review?(jmaher) → review+
(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 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.
Attachment #593904 - Flags: feedback?(gbrown) → feedback+
Attachment #593904 - Flags: feedback?(wjohnston)
(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.
No longer blocks: 720902
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
Attachment #592988 - Flags: approval-mozilla-beta?
Attachment #592988 - Flags: approval-mozilla-aurora?
Attachment #592990 - Flags: approval-mozilla-beta?
Attachment #592990 - Flags: approval-mozilla-aurora?
Attachment #592991 - Flags: approval-mozilla-beta?
Attachment #592991 - Flags: approval-mozilla-aurora?
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 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.
Attachment #592988 - Flags: approval-mozilla-beta?
Attachment #592988 - Flags: approval-mozilla-beta+
Attachment #592988 - Flags: approval-mozilla-aurora?
Attachment #592988 - Flags: approval-mozilla-aurora+
Attachment #592990 - Flags: approval-mozilla-beta?
Attachment #592990 - Flags: approval-mozilla-beta+
Attachment #592990 - Flags: approval-mozilla-aurora?
Attachment #592990 - Flags: approval-mozilla-aurora+
Attachment #592991 - Flags: approval-mozilla-beta?
Attachment #592991 - Flags: approval-mozilla-beta+
Attachment #592991 - Flags: approval-mozilla-aurora?
Attachment #592991 - Flags: approval-mozilla-aurora+
(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
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)
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.