672411 - Cannot completely scroll downwards at about:memory in portrait mode

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

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.

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: 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.

Comment 2

[Benjamin Stover (:stechz)]

Wes said he'd take a look.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch?

This fixes about:memory for me. Unfortunately, I'm going to have to endure a big ol' headache trying to understand why.

Comment 4

[Wesley Johnston (:wesj)]

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.

Comment 5

[Matt Brubeck (: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>

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2 (backed out)

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.

Comment 7

[Wesley Johnston (:wesj)]

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?

Comment 8

[Matt Brubeck (:mbrubeck)]

(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);

Comment 9

[Matt Brubeck (:mbrubeck)]

Passed Try: http://tbpl.mozilla.org/?tree=Try&rev=88b5137c9c26

http://hg.mozilla.org/integration/mozilla-inbound/rev/10cb6aeef385

Comment 10

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/10cb6aeef385

Comment 11

[Matt Brubeck (:mbrubeck)]

Backed out for causing Talos failures (bug 675750):
http://hg.mozilla.org/integration/mozilla-inbound/rev/ac9bc4e2b2dd

Comment 12

[Matt Brubeck (:mbrubeck)]

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.

Comment 13

[Marco Bonardo [:mak]]

backed out from central as well, http://hg.mozilla.org/mozilla-central/rev/ac9bc4e2b2dd

Comment 14

[Matt Brubeck (:mbrubeck)]

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.

Comment 15

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

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.

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: minor refactoring

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.

Comment 17

[Matt Brubeck (:mbrubeck)]

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

Comment 18

[Wesley Johnston (:wesj)]

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?

Comment 19

[Matt Brubeck (:mbrubeck)]

(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

Comment 20

[Ed Morley [:emorley]]

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

Comment 21

[Matt Brubeck (:mbrubeck)]

Pushed part 2 to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/4362a0bf69e0

Comment 22

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/4362a0bf69e0

Comment 23

[Cristian Nicolae (:xti)]

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

Comment 24

[Cristian Nicolae (:xti)]

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