852565 - Lock toolbar on pages smaller than the screen size

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Chris Lord [:cwiiis]]

So we have the issue with Google Maps that it always sizes itself as the viewport plus 2 vertical pixels. This triggers the viewport enlargement and gives it a dynamic toolbar. Instead of doing this, for a page that's smaller than the screen size, we could not enlarge the viewport and instead lock the toolbar and allow it to scroll normally.

This 'fixes' the issue on Google Maps, and any other site that manages to get page sizing wrong in the same way but still manages to disable scrolling.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Lock toolbar on small pages

Do as Brad suggested, this pretty much fixes our interaction with Google Maps and other sites with similar issues.

Comment 2

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

Comment on attachment 726734 [details] [diff] [review]
Lock toolbar on small pages

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

I'd rather leave google maps broken than hard-code in a magic "5" like this...

::: mobile/android/base/BrowserApp.java
@@ +223,5 @@
> +        // Don't let the toolbar scroll at all if the page is shorter than
> +        // the screen height.
> +        ImmutableViewportMetrics metrics =
> +            mLayerView.getLayerClient().getViewportMetrics();
> +        if (metrics.getPageHeight() < metrics.getHeight()) {

How does this deal with the page size changing after page load? e.g. the page starts off long, the user scrolls off the header, and then the page shrinks itself to less than metrics.getHeight(). (possibly due to rounding error). Seems likek the header should come back in that scenario but I don't think it will.

::: mobile/android/chrome/content/browser.js
@@ +3507,3 @@
>      if (!this.updatingViewportForPageSizeChange) {
>        this.updatingViewportForPageSizeChange = true;
> +      if (((viewport.pageBottom - viewport.pageTop <= gScreenHeight + 5) !=

Just say NO to magic numbers.

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Comment on attachment 726734 [details] [diff] [review]
> Lock toolbar on small pages
> 
> Review of attachment 726734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd rather leave google maps broken than hard-code in a magic "5" like
> this...
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +223,5 @@
> > +        // Don't let the toolbar scroll at all if the page is shorter than
> > +        // the screen height.
> > +        ImmutableViewportMetrics metrics =
> > +            mLayerView.getLayerClient().getViewportMetrics();
> > +        if (metrics.getPageHeight() < metrics.getHeight()) {
> 
> How does this deal with the page size changing after page load? e.g. the
> page starts off long, the user scrolls off the header, and then the page
> shrinks itself to less than metrics.getHeight(). (possibly due to rounding
> error). Seems likek the header should come back in that scenario but I don't
> think it will.

You're right, though I think this is an existing problem - this is a symptom of the header driving margin size instead of the other way round. I think this is best fixed in another bug.

> ::: mobile/android/chrome/content/browser.js
> @@ +3507,3 @@
> >      if (!this.updatingViewportForPageSizeChange) {
> >        this.updatingViewportForPageSizeChange = true;
> > +      if (((viewport.pageBottom - viewport.pageTop <= gScreenHeight + 5) !=
> 
> Just say NO to magic numbers.

Ok, so although 5 is a bit magic here, I'd say it's not an entirely magic number - it's the fuzz within which we decide we need to remeasure the page. This patch has changed the logic so that only screen-sized and above pages get the full viewport, but if the page is relative to the viewport, we need some heuristic to decide when to re-measure. We can't do this on every page size change, that'd be way too expensive...

That said, I think this would read nicer and make more sense if the fuzz was the toolbar height. I'll make that change along with a better comment.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Lock toolbar on small pages v2

This is less magic than the last patch and has the benefit of providing symmetric behaviour (the previous patch, a page could size itself bigger than the screen + toolbar size + x, then resize itself to screen + toolbar size - x, and not get the smaller viewport again, where x is a number between 5 and toolbar size minus 5)

Comment 5

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

Comment on attachment 727052 [details] [diff] [review]
Lock toolbar on small pages v2

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

::: mobile/android/chrome/content/browser.js
@@ +4006,5 @@
>  
>        minScale = screenW / pageWidth;
>  
> +      // In the situation the page size equals or exceeds the screen size
> +      // lengthen the viewport on the corresponding axis to include the margins.

Need a comma before "lengthen"

Comment 6

[Chris Lord [:cwiiis]]

Pushed to inbound with a small test fix, confirmed still r+ on IRC:

https://hg.mozilla.org/integration/mozilla-inbound/rev/74c06a7308d1

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74c06a7308d1

Comment 8

[Mathieu Pellerin]

This might have caused a regression, documented in bug 854289.

Comment 9

[William Lachance (:wlach)]

It appears like something in this patch is causing bug 855019 (CNN.com in eideticker refused to load on lg g2x)