Closed
Bug 840593
Opened 12 years ago
Closed 12 years ago
In content UI cut off on small screens
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox19 wontfix, firefox20 wontfix, firefox21 verified, firefox22 verified, firefox23 verified, relnote-firefox 21+, fennec20+)
VERIFIED
FIXED
Firefox 21
People
(Reporter: ibarlow, Assigned: wesj)
References
Details
(Whiteboard: [QVGA])
Attachments
(5 files, 3 obsolete files)
88.52 KB,
image/png
|
Details | |
4.48 KB,
patch
|
mbrubeck
:
review+
lsblakk
:
approval-mozilla-aurora-
lsblakk
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
27.03 KB,
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
95.94 KB,
image/png
|
Details | |
1.65 KB,
patch
|
mbrubeck
:
review+
bajaj
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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•12 years ago
|
OS: Mac OS X → Android
Hardware: x86 → ARM
Whiteboard: [QVGA]
Comment 1•12 years ago
|
||
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•12 years ago
|
Assignee: nobody → wjohnston
Updated•12 years ago
|
tracking-fennec: ? → 20+
Assignee | ||
Comment 2•12 years ago
|
||
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 3•12 years ago
|
||
Comment on attachment 720158 [details] [diff] [review]
Patch
Does | width=device-wdith | work? Maybe without the "initial-scale" ?
Assignee | ||
Comment 4•12 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 5•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Wrong patch.
Attachment #721894 -
Attachment is obsolete: true
Attachment #721894 -
Flags: review?(mark.finkle)
Attachment #721899 -
Flags: review?(mark.finkle)
Comment 8•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
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 12•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Here's this at 1.0 zoom. Big, but not overflowing.
Comment 15•12 years ago
|
||
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+
Assignee | ||
Comment 18•12 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•12 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•12 years ago
|
||
Landed the simple patch (first) on inbound for testing. I'll land the more complex patch once its confirmed.
Comment 21•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 years ago
|
||
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)
Comment 27•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Updated•12 years ago
|
Comment 28•12 years ago
|
||
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+
Comment 29•12 years ago
|
||
(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•12 years ago
|
||
What logging code?
https://hg.mozilla.org/integration/mozilla-inbound/rev/943c43de7329
Comment 31•12 years ago
|
||
Assignee | ||
Comment 32•12 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•12 years ago
|
||
Assignee | ||
Comment 34•12 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 35•12 years ago
|
||
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-
Updated•12 years ago
|
relnote-firefox:
--- → ?
Comment 36•12 years ago
|
||
Comment 37•12 years ago
|
||
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•12 years ago
|
Comment 38•12 years ago
|
||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
status-firefox23:
--- → verified
Updated•12 years ago
|
Target Milestone: Firefox 22 → Firefox 21
Updated•12 years ago
|
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•