Last Comment Bug 840593 - In content UI cut off on small screens
: In content UI cut off on small screens
Status: VERIFIED FIXED
[QVGA]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 21
Assigned To: Wesley Johnston (:wesj)
:
Mentors:
: 769485 772286 (view as bug list)
Depends on: 858111
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-12 10:09 PST by Ian Barlow (:ibarlow)
Modified: 2013-04-05 16:58 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
verified
verified
verified
21+
20+


Attachments
Patch (5.30 KB, patch)
2013-03-01 15:16 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Review
Bigger patch (6.32 KB, patch)
2013-03-06 14:27 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Review
Bigger Patch (26.57 KB, patch)
2013-03-06 14:37 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Review
Screenshot (88.52 KB, image/png)
2013-03-06 14:49 PST, Wesley Johnston (:wesj)
no flags Details
Small patch v2 (4.48 KB, patch)
2013-03-08 13:06 PST, Wesley Johnston (:wesj)
mbrubeck: review+
lukasblakk+bugs: approval‑mozilla‑aurora-
lukasblakk+bugs: approval‑mozilla‑beta-
Details | Diff | Review
Big patch v2 (27.03 KB, patch)
2013-03-08 14:45 PST, Wesley Johnston (:wesj)
mbrubeck: review+
Details | Diff | Review
Screen shot at 1.0 zoom (95.94 KB, image/png)
2013-03-08 14:46 PST, Wesley Johnston (:wesj)
no flags Details
Viewport fix (1.65 KB, patch)
2013-03-22 16:39 PDT, Wesley Johnston (:wesj)
mbrubeck: review+
bajaj.bhavana: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta-
Details | Diff | Review

Description Ian Barlow (:ibarlow) 2013-02-12 10:09:05 PST
Testing on a QVGA phone today, we saw that pages like Downloads, Addons, about:ocnfig etc are cut off on the right and scrolling horizontally is necessary. 

We should do what we can to ensure these pages are responsive enough where they never scroll horizontally.
Comment 1 Aaron Train [:aaronmt] 2013-02-26 12:19:43 PST
We supposedly are supporting QVGA for Fennec 20, so requesting tracking.
Comment 2 Wesley Johnston (:wesj) 2013-03-01 15:16:37 PST
Created attachment 720158 [details] [diff] [review]
Patch

Hmm.. We have a viewport on these pages of:

<meta name="viewport" content="width=480; initial-scale=.6667; user-scalable=0" />

which basically force them to a min-width of 320px. This forces that to zero (maybe thats too small...).

If we're going to keep them non-native (I think we should) I think we generally need to do some more in-depth work to make these pages responsive. about:addons should probably switch to something more like desktop on 7-10 in tablets (we should probably just make the desktop about:addons responsive, it already is partly there).

Downloads could also show a two pane view, although our current view with a max-width on the list might be fine too?

Apps is likely our best fit right now since its essentially a grid of icons.
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2013-03-02 07:12:17 PST
Comment on attachment 720158 [details] [diff] [review]
Patch

Does | width=device-wdith | work? Maybe without the "initial-scale" ?
Comment 4 Wesley Johnston (:wesj) 2013-03-06 10:45:41 PST
No.

We currently set a specific width an initial-scale and a width. The viewport code has to choose between the two and basically takes the width as a minimum-width, but forces the scale to be used. i.e. we'll display at .667 scale with a width that's at least 480px wide.

When you use device-width for the width, we essentially force the page to be real-device-width*some-scale-factor wide (on phones that usually comes out to a 320px viewport). i.e. we display at a different scale than .667 which makes these pages look a bit nuts.

We can fix that, but will have to adjust all of our sizes in aboutAddons.css to be 320/480 smaller. Since they wanted to uplift this patch, I thought this was an easier solution. I can put up a second patch with the scaled aboutAddons.css stuff as well though.

I'm adding a need info for mbrubeck to make sure I haven't missed something else.
Comment 5 Matt Brubeck (:mbrubeck) 2013-03-06 10:51:42 PST
Comment on attachment 720158 [details] [diff] [review]
Patch

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

::: mobile/android/chrome/content/about.xhtml
@@ +14,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <html xmlns="http://www.w3.org/1999/xhtml">
>  <head>
> +  <meta name="viewport" content="width=0; initial-scale=.6667; user-scalable=no"/>

I think you can just omit "width" (instead of using "width=0").
Comment 6 Wesley Johnston (:wesj) 2013-03-06 14:27:22 PST
Created attachment 721894 [details] [diff] [review]
Bigger patch

Thanks matt. I don't have a device to test this on and was using 0 to be safe.

This does the bigger job of moving us to use device-width and a reduced font-size. I moved most of our measurements to em units to make page wide changes easier. I also flipped (some) things to use flex-box and created a base css file for all our in-content pages to inherit from.
Comment 7 Wesley Johnston (:wesj) 2013-03-06 14:37:05 PST
Created attachment 721899 [details] [diff] [review]
Bigger Patch

Wrong patch.
Comment 8 Matt Brubeck (:mbrubeck) 2013-03-06 14:42:46 PST
Comment on attachment 721899 [details] [diff] [review]
Bigger Patch

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

::: mobile/android/chrome/content/aboutAddons.xhtml
@@ +16,5 @@
>  
>  <html xmlns="http://www.w3.org/1999/xhtml">
>  <head>
>    <title>&aboutAddons.title2;</title>
> +  <meta name="viewport" content="width=device-width; user-scalable=0" />

In addition to the text, this change will also affect the scale of any images on the page...

@@ +60,5 @@
>    </menu>
>  
> +  <div id="addons-header" class="header">
> +    <div>&aboutAddons.header2;</div>
> +    <img src="chrome://browser/skin/images/addons-amo-hdpi.png" pref="extensions.getAddons.browseAddons" onclick="openLink(this);"/>

...like this one, which I think will now be larger (and blurry on HDPI screens where it wasn't before).
Comment 9 Wesley Johnston (:wesj) 2013-03-06 14:49:43 PST
Created attachment 721906 [details]
Screenshot

Yes. That image in larger now. Its the only difference between the two modes that I can see, and because we included it as an image tag, its a pain to fix.

After pushing at it for awhile, I decided it wasn't worth the effort. It looks larger, but fine on my XHDPI S3. The header area expands slightly to make sure there is room for it.
Comment 10 Matt Brubeck (:mbrubeck) 2013-03-06 15:02:09 PST
(In reply to Wesley Johnston (:wesj) from comment #9)
> Yes. That image in larger now. Its the only difference between the two modes
> that I can see, and because we included it as an image tag, its a pain to
> fix.

You can fix it by giving it an ID or a class and then setting its width/height explicitly in CSS, something like:

#my-image-id {
  width: 33.33px;
  height: 33.33px;
}
Comment 11 Wesley Johnston (:wesj) 2013-03-08 13:06:30 PST
Created attachment 722915 [details] [diff] [review]
Small patch v2

Throwing this to mbrubeck since he's the viewport wizard.
Comment 12 Matt Brubeck (:mbrubeck) 2013-03-08 13:14:19 PST
Comment on attachment 722915 [details] [diff] [review]
Small patch v2

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

Looks good to me.  Have you done any testing to see how this looks on QVGA screens?

If you don't have a QVGA device, you can temporarily change the initial-scale from 0.667 to 1 (just for testing) to get a similar layout on a more typical phone screen.
Comment 13 Wesley Johnston (:wesj) 2013-03-08 14:45:35 PST
Created attachment 722969 [details] [diff] [review]
Big patch v2

Not sure if you want this or not mbrubeck, but you looked before! Your

#id { width: 33px; height: 33px; }

trick does not work. Someone somewhere is refusing to let me set the width of this image element. No number of !important or max-width rules seemed to help either. I really want to get the DOMInspector remoted.

I took a simpler approach since I'm changing things and just moved it to a background-image on a div[role="button"].
Comment 14 Wesley Johnston (:wesj) 2013-03-08 14:46:31 PST
Created attachment 722970 [details]
Screen shot at 1.0 zoom

Here's this at 1.0 zoom. Big, but not overflowing.
Comment 15 Matt Brubeck (:mbrubeck) 2013-03-08 17:57:58 PST
Comment on attachment 722969 [details] [diff] [review]
Big patch v2

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

Nice cleanup!

::: mobile/android/chrome/content/browser.js
@@ +3915,4 @@
>          viewportW = Math.max(viewportW, screenW / maxInitialZoom);
> +        console.log("Use max: " + viewportW + " or " + (screenW / maxInitialZoom));
> +      }
> +      console.log("Use max: " + viewportW + " or " + (screenW / maxInitialZoom));

Remember to drop the logging code before checkin.

::: mobile/android/themes/core/aboutBase.css
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +html {
> +  font-family: Roboto,"Droid Sans",helvetica,arial,clean,sans-serif;

Should we change this to just "sans-serif" so it uses our shiny new default fonts?

We can probably at least drop helvetica,arial,clean...

@@ +5,5 @@
> +html {
> +  font-family: Roboto,"Droid Sans",helvetica,arial,clean,sans-serif;
> +  font-size: 12px;
> +  background-color: #ced7de;
> +  -moz-text-size-adjust: none;

-moz-text-size-adjust shouldn't be necessary now that you are using width=device-width.  I'm pretty sure automatically disable font inflation in that case (bug 706198).
Comment 16 Aaron Train [:aaronmt] 2013-03-12 09:24:23 PDT
*** Bug 769485 has been marked as a duplicate of this bug. ***
Comment 17 Aaron Train [:aaronmt] 2013-03-12 09:25:11 PDT
*** Bug 772286 has been marked as a duplicate of this bug. ***
Comment 18 Wesley Johnston (:wesj) 2013-03-12 09:48:41 PDT
Comment on attachment 722915 [details] [diff] [review]
Small patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Since the beginning of (Fennec) time
User impact if declined: Some about pages require horizontal scrolling
Testing completed (on m-c, etc.): Landing on mc now
Risk to taking this patch (and alternatives if risky): Very low risk. Just removing a min-width.
String or UUID changes made by this patch: None.
Comment 19 Wesley Johnston (:wesj) 2013-03-12 09:49:18 PDT
Comment on attachment 722915 [details] [diff] [review]
Small patch v2

See aurora approval request
Comment 20 Wesley Johnston (:wesj) 2013-03-12 09:52:11 PDT
Landed the simple patch (first) on inbound for testing. I'll land the more complex patch once its confirmed.
Comment 21 Aaron Train [:aaronmt] 2013-03-12 11:19:21 PDT
I am not seeing any changes on the builds produced (https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d31d517c7f7c) on my Galaxy Chat 320x240 (QVGA); no new changes.
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-03-12 11:43:28 PDT
Backed out because this apparently caused the reanimated corpse of bug 817440 to come back from the grave and feed on every robocop-2 run thereafter.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb7e29036271

https://tbpl.mozilla.org/php/getParsedLog.php?id=20572457&tree=Mozilla-Inbound
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-14 10:35:03 PDT
Triage drive-by - can't approve this for uplift unless it can land without issue on trunk - will revisit and watch for an update here.
Comment 24 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-18 14:48:54 PDT
Comment on attachment 722915 [details] [diff] [review]
Small patch v2

Not going to take this for FF20 beta at this point. If this doesn't make it into FF21 before merge to beta you can flip the flag again (or on an updated patch if needed due to the landing issues with this one).
Comment 25 Wesley Johnston (:wesj) 2013-03-22 16:37:37 PDT
Landed the larger patch on Center:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c81af6c72726

Found a fix for the small one. Patch coming...
Comment 26 Wesley Johnston (:wesj) 2013-03-22 16:39:09 PDT
Created attachment 728511 [details] [diff] [review]
Viewport fix

We are not using a default min size of zero if one isn't specified. In fact, we just use NaN which causes failures all over in here.
Comment 27 Joe Drew (not getting mail) 2013-03-23 15:58:40 PDT
https://hg.mozilla.org/mozilla-central/rev/c81af6c72726
Comment 28 Matt Brubeck (:mbrubeck) 2013-03-24 11:57:47 PDT
Comment on attachment 728511 [details] [diff] [review]
Viewport fix

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

::: mobile/android/chrome/content/browser.js
@@ +5026,5 @@
>      let minScale = parseFloat(windowUtils.getDocumentMetadata("viewport-minimum-scale"));
>      let maxScale = parseFloat(windowUtils.getDocumentMetadata("viewport-maximum-scale"));
>  
> +    let widthStr = windowUtils.getDocumentMetadata("viewport-width") || 0;
> +    let heightStr = windowUtils.getDocumentMetadata("viewport-height") || 0;

This will affect the statements below where we check if (widthStr == "").

Well, actually it won't, but it depends on the fact that (0 == "") in JavaScript which in confusing and will break if someone changes == to === below.

@@ +5031,2 @@
>      let width = this.clamp(parseInt(widthStr), kViewportMinWidth, kViewportMaxWidth);
>      let height = this.clamp(parseInt(heightStr), kViewportMinHeight, kViewportMaxHeight);

You should probably put the "|| 0" on these lines instead, for clarity.

Other than that, I think this is good and safe, but I wish we still had working tests for this code. :(
Comment 29 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2013-03-25 05:21:16 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #15)
> Remember to drop the logging code before checkin.

The logging code was checked in!

http://hg.mozilla.org/mozilla-central/file/c81af6c72726/mobile/android/chrome/content/browser.js#l3378
Comment 30 Wesley Johnston (:wesj) 2013-03-25 08:53:10 PDT
What logging code?

https://hg.mozilla.org/integration/mozilla-inbound/rev/943c43de7329
Comment 31 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2013-03-25 09:02:08 PDT
You missed a couple :)

https://hg.mozilla.org/integration/mozilla-inbound/file/943c43de7329/mobile/android/chrome/content/browser.js#l3386
Comment 32 Wesley Johnston (:wesj) 2013-03-25 09:10:37 PDT
Gah. Serves me right for thinking I could wait between writing/landing this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bff25b01fb0d
Comment 34 Wesley Johnston (:wesj) 2013-03-25 16:01:41 PDT
Comment on attachment 728511 [details] [diff] [review]
Viewport fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Forever
User impact if declined: Sites that have no width in their viewport will incorrectly scale themselves to 800px wide.
Testing completed (on m-c, etc.): Landed on mc now.
Risk to taking this patch (and alternatives if risky): Low. Sites with this type of viewport seem to be fairly odd. That means this mostly affects internet dark-matter which is really hard to test for.
String or UUID changes made by this patch: None.
Comment 35 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-25 16:13:16 PDT
Comment on attachment 728511 [details] [diff] [review]
Viewport fix

Too late to take this for our final beta without a proper testing/verification having taken place and since it sounds like this affects a small portion of sites and that zooming out is possible to see the page (after the default size is loaded) we think this can wait and land to FF21 once it makes it to trunk and gets testing.  Will mark this for a relnote to say it's fixed in 21.
Comment 36 Ryan VanderMeulen [:RyanVM] 2013-03-26 07:23:07 PDT
https://hg.mozilla.org/mozilla-central/rev/67cb3a9a4ad1
Comment 37 bhavana bajaj [:bajaj] 2013-03-26 09:06:13 PDT
Comment on attachment 728511 [details] [diff] [review]
Viewport fix

Helps scale correctly on devices with QVGA screen which we are supporting in Fx20.

Requesting QA verification once this lands with help in exploratory testing on those devices .will be of great help if you can cover the duplicate bugs as a part of this verification. Thanks !
Comment 38 Ryan VanderMeulen [:RyanVM] 2013-03-26 12:58:06 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/34f00081ec61

Note You need to log in before you can comment on or make changes to this bug.