Closed Bug 604765 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0b4+)

VERIFIED FIXED
Tracking Status
fennec 2.0b4+ ---

People

(Reporter: mbrubeck, Assigned: wesj)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 603194
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b3+
Attached patch WIP (obsolete) — Splinter Review
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.
tracking-fennec: 2.0b3+ → 2.0b4+
Passing this to Wes
Assignee: mbrubeck → wjohnston
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #490269 - Attachment is obsolete: true
Attachment #501532 - Flags: review?(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.
Attachment #501532 - Flags: review?(mbrubeck) → review+
Blocks: 623675
Attached patch Patch with testsSplinter Review
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.
Attachment #501532 - Attachment is obsolete: true
Attachment #501770 - Flags: review?(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.)
Attachment #501770 - Flags: review?(mbrubeck) → review+
Pushed:

http://hg.mozilla.org/mobile-browser/rev/a1ccd0b9c52e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
verified FIXED on builds:

Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20100107 Namoroka/4.0b9pre Fennec/4.0b4pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.