Closed Bug 672411 Opened 8 years ago Closed 8 years ago

Cannot completely scroll downwards at about:memory in portrait mode

Categories

(Firefox for Android Graveyard :: Panning/Zooming, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 9

People

(Reporter: martijn.martijn, Assigned: mbrubeck)

References

()

Details

Attachments

(3 files, 2 obsolete files)

At about:memory, I can't completely scroll downwards, also the scroll indicators show that you can't completely scroll downwards.

About:memory is adding elements on page load, fyi.
Attached image screenshot
Last 10 % of the page is not viewable in portrait mode; work around is to look at it in landscape mode.  See screenshot of trying to scroll to the bottom.
Priority: -- → P3
Summary: Cannot completely scroll downwards at about:memory → Cannot completely scroll downwards at about:memory in portrait mode
Wes said he'd take a look.
Assignee: nobody → wjohnston
Attached patch Patch? (obsolete) — Splinter Review
This fixes about:memory for me. Unfortunately, I'm going to have to endure a big ol' headache trying to understand why.
Comment on attachment 549217 [details] [diff] [review]
Patch?

I think this makes sense. We're trying to get the scale ratio of the window, so we're comparing the screenW and contentWindowWidth (not the document). For these local pages that are overflowing the screen size, contentWindowWidth != contentDocumentWidth.
Attachment #549217 - Flags: review?(mbrubeck)
Comment on attachment 549217 [details] [diff] [review]
Patch?

The minimum zoom level shows the entire document width, not just the viewport width.  With this patch we show checkerboard at the bottom of any page whose document is wider than the viewport.  For example, try loading this URL in Fennec with this patch:

data:text/html,<body%20style=width:2000px>test</body>
Attachment #549217 - Flags: review?(mbrubeck) → review-
Attached patch patch v2 (backed out) (obsolete) — Splinter Review
This fixes clampZoomLevel to work correctly on non-zoomable pages, and then uses it to calculate the actual minimum zoom level for the page.  Thanks to Wes on IRC for tracking down the actual cause of the bug.
Assignee: wjohnston → mbrubeck
Status: NEW → ASSIGNED
Attachment #549229 - Flags: review?(wjohnston)
Comment on attachment 549229 [details] [diff] [review]
patch v2 (backed out)

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

::: mobile/chrome/content/browser.js
@@ -2730,5 @@
>      }
>  
>      // Make sure the viewport height is not shorter than the window when
>      // the page is zoomed out to show its full width.
> -    if (viewportH * this.clampZoomLevel(this.getPageZoomLevel()) < screenH)

Blame says this check was put in to make sure some tests passed on device (bug 650694). Sure we can remove it?
Attachment #549229 - Flags: review?(wjohnston) → review+
(In reply to comment #7)
> > -    if (viewportH * this.clampZoomLevel(this.getPageZoomLevel()) < screenH)
> 
> Blame says this check was put in to make sure some tests passed on device
> (bug 650694). Sure we can remove it?

The if statement is now redundant with the Math.max() call on the following line.  To see why, re-write the expression as follows, with no change in meaning:

>     let minScale = this.clampZoomLevel(this.getPageZoomLevel());
>-    if (viewportH < screenH / minScale)
>       viewportH = Math.max(viewportH, screenH / minScale);
http://hg.mozilla.org/mozilla-central/rev/10cb6aeef385
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
Depends on: 675750
Backed out for causing Talos failures (bug 675750):
http://hg.mozilla.org/integration/mozilla-inbound/rev/ac9bc4e2b2dd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [inbound]
Target Milestone: Firefox 8 → ---
Possibly related to the tp4m failures, this patch changes the viewport height of http://people.mozilla.com/~mbrubeck/mobile_tp4/m.baidu.com/www.baidu.com/ at some screen sizes.  I'll take a look to see why.
Status: REOPENED → ASSIGNED
Attachment #549217 - Attachment is obsolete: true
Attachment #549229 - Attachment description: patch → patch v2 (backed out)
Since bug 610834 was fixed, this no longer causes failures in tp4m.  But it does seem to cause perf regressions there.  I pushed this patch to try and ran tp4m three times:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=25ea992965f7

The times were 915, 2435, and 871 ms.  The normal time range is around 790-840 ms.  From the logs, the time increase was spread out across most of the pages in the set, although in the 2435 case it was concentrated in the yandex.ru pages.
Depends on: 610834
Attached patch patch v3Splinter Review
I think the original patch may have been slow because of additional calls to getBoundingClientRect, in getPageZoomLevel.  I have an updated patch in progress that optimizes away those calls.  I'll push this to Try to check performance.
Attachment #549229 - Attachment is obsolete: true
This is not directly related to this bug; it's just a cleanup of some nearby code.  This is purely a refactoring; it should have no change in behavior.
Attachment #555570 - Flags: review?
Attachment #555570 - Flags: review? → review?(lucasr.at.mozilla)
OS: Windows 7 → All
Hardware: x86 → All
Whiteboard: [has wip]
Comment on attachment 555568 [details] [diff] [review]
patch v3

The data's a bit too noisy to tell for sure yet, but this appears to have fixed the tp4m regression.  I've triggered another couple runs and I'll try to make sure that the regression is really fixed before I land this:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=b497cf013da1
Attachment #555568 - Flags: review+
Attachment #555568 - Flags: review+ → review?(wjohnston)
Attachment #555570 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 555568 [details] [diff] [review]
patch v3

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

Nice!

::: mobile/chrome/content/browser.js
@@ +3030,5 @@
>      return this.clampZoomLevel(pageZoom);
>    },
>  
> +  /**
> +   * Load a URI in the current tab, or a new tab if necessary.

Is this comment in the right place?
Attachment #555568 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #18)
> Is this comment in the right place?

Oops, copy-paste error.  Fixed, thanks.  Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f755ec6586
Whiteboard: [has wip] → [inbound]
http://hg.mozilla.org/mozilla-central/rev/f3f755ec6586

Not closing, since I don't believe both patches have landed. If this is not the case, please close - thanks :-)
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
Pushed part 2 to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/4362a0bf69e0
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/4362a0bf69e0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110901
Firefox/9.0a1 Fennec/9.0a1
Device: Acer ICONIA TAB A500
OS: Android 3.1
Status: RESOLVED → VERIFIED
Also verified on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110901
Firefox/9.0a1 Fennec/9.0a1
Device: Samsung Galaxy S
OS: Android 2.2
You need to log in before you can comment on or make changes to this bug.