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

VERIFIED FIXED in Firefox 11

Status

()

P3
normal
VERIFIED FIXED
7 years ago
2 years ago

People

(Reporter: martijn.martijn, Assigned: kats)

Tracking

({testcase})

Trunk
Firefox 13
ARM
Android
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

(URL)

Attachments

(6 attachments)

(Reporter)

Description

7 years ago
Created attachment 590899 [details]
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
(Reporter)

Comment 1

7 years ago
Created attachment 590900 [details]
Screenshot of bug

This was tested on the LG Optimus Black, Android 2.2.2.
Assignee: nobody → bugmail.mozilla
tracking-fennec: --- → 11+
Priority: -- → P3
Created attachment 592988 [details] [diff] [review]
(1/3) Clarify and fix zoom rect when zooming to page width
Attachment #592988 - Flags: review?(wjohnston)
Created attachment 592990 [details] [diff] [review]
(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.
Attachment #592990 - Flags: review?(wjohnston)
Created attachment 592991 [details] [diff] [review]
(3/3) Cleanup - remove unused things
Attachment #592991 - 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.
Blocks: 720902
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+
Depends on: 723619
Created attachment 593904 [details] [diff] [review]
(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!
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
Duplicate of this bug: 720902
Duplicate of this bug: 716096
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

Comment 23

7 years ago
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
status-firefox11: fixed → verified
status-firefox12: fixed → verified
status-firefox13: fixed → verified
Blocks: 724948
You need to log in before you can comment on or make changes to this bug.