604765 - Use an 800px viewport by default except for pages with >800px content width

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

For bug 588881 we changed our default viewport width from 800px to 980px, because some sites like facebook.com and mozilla.com do not work well at 800px.  However, this causes many sites to be scaled down unnecessarily when zoomed to fit.

Instead, we could try laying out the site with an 800px viewport, then change the viewport size to 980 only if the document width exceeds 800.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

Just a snapshot of my current development.  The hard part is finding the right moment to reset the viewport when the page starts loading, and making sure it has had enough time to layout at 800px after the viewport is reset.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Passing this to Wes

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This is pretty simple, but works fairly well on desktop and device with some basic test pages I wrote. I've also been testing by switching back and forth between www.google.com and www.facebook.com as well, and that seems happy on desktop. On my device Google doesn't seem to like 800px.

Sounds like you had thought about this a bit Matt, so I'd be happy to hear if I'm missing something.

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 501532 [details] [diff] [review]
Patch v1

Looks good to me!  I wasn't able to trigger any of the problems I think I saw before.

>       if (this._tab == Browser.selectedTab) {
>         // We're about to have new page content, so scroll the content area
>         // to the top so the new paints will draw correctly.
>         // (background tabs are delayed scrolled to top in _documentStop)
>         Browser.scrollContentToTop({ x: 0 });
>       }
>     }
>+    this._tab.useFallbackWidth = false;
>+    this._tab.updateViewportSize();

Move these two lines inside the "if (locationHasChanged)" block above, so we don't reset the width when clicking an in-page (URI fragment) link.

Please update the browser-chrome viewport tests as needed, and add a new test that triggers the fallback behavior.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch with tests

Oops. Didn't see that there was an r+ before and rebuilt this on top of the old patch.

This is the fixed patch with tests/test fixes. New tests were tacked onto the viewport ones because... that made things easy. Putting up for review again to make sure that's what we all want.

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 501770 [details] [diff] [review]
Patch with tests

>+  { metadata: "stylewidth:400px;margin:0px;", width: DEFAULT_WIDTH, scale: 1 },

Just for readability, can we use "style=" instead of just "style" for the prefix?

>+  { metadata: "stylewidth:1000px;margin:0px;", width: 980, scale: DEFAULT_WIDTH/1000 },

I think this should be window.innerWidth/1000 instead.  (They happen to be the same, but this changes with the window size, not the default scale.)

Comment 7

[Wesley Johnston (:wesj)]

Pushed:

http://hg.mozilla.org/mobile-browser/rev/a1ccd0b9c52e

Comment 8

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20100107 Namoroka/4.0b9pre Fennec/4.0b4pre