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)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
VERIFIED FIXED
Milestone:
Firefox 21
Tracking Flags:
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- verified
firefox22 --- verified
firefox23 --- verified
relnote-firefox --- 21+
fennec 20+ ---

People

(Reporter: ibarlow, Assigned: wesj)

References

Details

(Whiteboard: [QVGA])

Attachments

(5 files, 3 obsolete files)

Screenshot
88.52 KB, image/png
Details
Small patch v2
4.48 KB, patch
mbrubeck
: review+
lsblakk
: approval-mozilla-aurora-
lsblakk
: approval-mozilla-beta-
Details | Diff | Splinter Review
Big patch v2
27.03 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
Screen shot at 1.0 zoom
95.94 KB, image/png
Details
Viewport fix
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.
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: nobody → wjohnston
tracking-fennec: ? → 20+
Attached patch Patch (obsolete) — — Splinter Review
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" ?
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)
Attached patch Bigger patch (obsolete) — — Splinter Review
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)
Attached patch Bigger Patch (obsolete) — — Splinter Review
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).
Attached image 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; }
Attached patch Small patch v2 — — Splinter Review
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+
Attached patch Big patch v2 — — Splinter Review
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)
Attached image 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+
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?
Comment on attachment 722915 [details] [diff] [review] Small patch v2 See aurora approval request
Attachment #722915 - Flags: approval-mozilla-beta?
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-
Landed the larger patch on Center: https://hg.mozilla.org/integration/mozilla-inbound/rev/c81af6c72726 Found a fix for the small one. Patch coming...
Attached patch Viewport fix — — Splinter Review
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
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
Gah. Serves me right for thinking I could wait between writing/landing this. https://hg.mozilla.org/integration/mozilla-inbound/rev/bff25b01fb0d
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-
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+
Keywords: qawanted, verifyme
Status: RESOLVED → VERIFIED
status-firefox21: fixed → verified
status-firefox22: fixed → verified
status-firefox23: --- → verified
Keywords: qawanted, verifyme
Target Milestone: Firefox 22 → Firefox 21
Depends on: 858111
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: