In content UI cut off on small screens

VERIFIED FIXED in Firefox 21

Status

()

Firefox for Android
General
VERIFIED FIXED
4 years ago
10 months ago

People

(Reporter: ibarlow, Assigned: wesj)

Tracking

unspecified
Firefox 21
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox19 wontfix, firefox20 wontfix, firefox21 verified, firefox22 verified, firefox23 verified, relnote-firefox 21+, fennec20+)

Details

(Whiteboard: [QVGA])

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.

Updated

4 years ago
OS: Mac OS X → Android
Hardware: x86 → ARM
Whiteboard: [QVGA]
We supposedly are supporting QVGA for Fennec 20, so requesting tracking.
tracking-fennec: --- → ?
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
(Assignee)

Updated

4 years ago
Assignee: nobody → wjohnston

Updated

4 years ago
tracking-fennec: ? → 20+
(Assignee)

Comment 2

4 years ago
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.
Attachment #720158 - Flags: review?(mark.finkle)
Comment on attachment 720158 [details] [diff] [review]
Patch

Does | width=device-wdith | work? Maybe without the "initial-scale" ?
(Assignee)

Comment 4

4 years ago
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.
Flags: needinfo?(mbrubeck)
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").
Flags: needinfo?(mbrubeck)
(Assignee)

Comment 6

4 years ago
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.
Attachment #721894 - Flags: review?(mark.finkle)
(Assignee)

Comment 7

4 years ago
Created attachment 721899 [details] [diff] [review]
Bigger Patch

Wrong patch.
Attachment #721894 - Attachment is obsolete: true
Attachment #721894 - Flags: review?(mark.finkle)
Attachment #721899 - Flags: review?(mark.finkle)
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).
(Assignee)

Comment 9

4 years ago
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.
(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;
}
(Assignee)

Comment 11

4 years ago
Created attachment 722915 [details] [diff] [review]
Small patch v2

Throwing this to mbrubeck since he's the viewport wizard.
Attachment #720158 - Attachment is obsolete: true
Attachment #720158 - Flags: review?(mark.finkle)
Attachment #722915 - Flags: review?(mbrubeck)
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.
Attachment #722915 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 13

4 years ago
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"].
Attachment #721899 - Attachment is obsolete: true
Attachment #721899 - Flags: review?(mark.finkle)
Attachment #722969 - Flags: review?(mbrubeck)
(Assignee)

Comment 14

4 years ago
Created attachment 722970 [details]
Screen shot at 1.0 zoom

Here's this at 1.0 zoom. Big, but not overflowing.
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).
Attachment #722969 - Flags: review?(mbrubeck) → review+

Updated

4 years ago
Duplicate of this bug: 769485

Updated

4 years ago
Duplicate of this bug: 772286
(Assignee)

Comment 18

4 years ago
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.
Attachment #722915 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 19

4 years ago
Comment on attachment 722915 [details] [diff] [review]
Small patch v2

See aurora approval request
Attachment #722915 - Flags: approval-mozilla-beta?
(Assignee)

Comment 20

4 years ago
Landed the simple patch (first) on inbound for testing. I'll land the more complex patch once its confirmed.
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.
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
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 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).
Attachment #722915 - Flags: approval-mozilla-beta?
Attachment #722915 - Flags: approval-mozilla-beta-
Attachment #722915 - Flags: approval-mozilla-aurora?
Attachment #722915 - Flags: approval-mozilla-aurora-
(Assignee)

Comment 25

4 years ago
Landed the larger patch on Center:

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

Found a fix for the small one. Patch coming...
(Assignee)

Comment 26

4 years ago
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.
Attachment #728511 - Flags: review?(mbrubeck)
https://hg.mozilla.org/mozilla-central/rev/c81af6c72726
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22

Updated

4 years ago
status-firefox22: affected → fixed
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. :(
Attachment #728511 - Flags: review?(mbrubeck) → review+
(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
(Assignee)

Comment 30

4 years ago
What logging code?

https://hg.mozilla.org/integration/mozilla-inbound/rev/943c43de7329
You missed a couple :)

https://hg.mozilla.org/integration/mozilla-inbound/file/943c43de7329/mobile/android/chrome/content/browser.js#l3386
(Assignee)

Comment 32

4 years ago
Gah. Serves me right for thinking I could wait between writing/landing this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bff25b01fb0d
(Assignee)

Comment 33

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/67cb3a9a4ad1
(Assignee)

Comment 34

4 years ago
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.
Attachment #728511 - Flags: approval-mozilla-beta?
Attachment #728511 - Flags: approval-mozilla-aurora?
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.
Attachment #728511 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
relnote-firefox: --- → ?
https://hg.mozilla.org/mozilla-central/rev/67cb3a9a4ad1
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 !
Attachment #728511 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Keywords: qawanted, verifyme
https://hg.mozilla.org/releases/mozilla-aurora/rev/34f00081ec61
status-firefox19: affected → wontfix
status-firefox20: affected → wontfix
status-firefox21: affected → fixed

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox21: fixed → verified
status-firefox22: fixed → verified
status-firefox23: --- → verified
Keywords: qawanted, verifyme

Updated

4 years ago
Target Milestone: Firefox 22 → Firefox 21

Updated

4 years ago
relnote-firefox: ? → 21+
Depends on: 858111
You need to log in before you can comment on or make changes to this bug.