830644 - Support WVGA and other non-HiDPI screen dimensions and resolutions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Rex has a work-in-progress patch that enables WVGA and QHD support for Gaia.

To prevent further maintenance burden, I would like to have the patch landed at the earliest when the tree is ready to receive non-v1 changes.

After that, we should establish a rule in Gaia to maintain the support of these screens.

Vivien, what's the earliest convenience for such changeset to land?

Comment 1

[KM Lee [:rexboy] (inactivate after Sep. 22)]

The changes are relatively wide (mainly on css that changes unit from px to rem and resize images), we may need some effort on reviewing and revising.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #0)
> Rex has a work-in-progress patch that enables WVGA and QHD support for Gaia.
> 
> To prevent further maintenance burden, I would like to have the patch landed
> at the earliest when the tree is ready to receive non-v1 changes.
> 
> After that, we should establish a rule in Gaia to maintain the support of
> these screens.
> 
> Vivien, what's the earliest convenience for such changeset to land?

The tree won't be ready to receive non v1 change until we create a v1 branch. Sadly I don't know when this will happen (I really would like :/).

So this one should likely land in the same time frame than shira.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #2)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #0)
> > Vivien, what's the earliest convenience for such changeset to land?
> 
> The tree won't be ready to receive non v1 change until we create a v1
> branch. Sadly I don't know when this will happen (I really would like :/).
> 
> So this one should likely land in the same time frame than shira.

What can we do to make it happen sooner?

Comment 4

[KM Lee [:rexboy] (inactivate after Sep. 22)]

https://github.com/rexboy7/gaia/commits/resolution_landing
Just arranged the whole branch based on master of 1/24 here.  Basically there's one commit for each app. After discussing with Tim, I would send a pull request and post each commit a patch file here for reviewing.
What this patch mainly do is:
- Put a shared css file called resolution.css, which is included in every apps, for detecting phone resolution and set font-size on body correspondingly.
- Change unit from px/em to rem.
- set background-size for picutres which need to be scaled on larger resolution.

with miscellaneous changes.
The work started about one month ago with several merges, I tried my best to catch new features need to patch and you may notice me if I missed.  I'll start to send review request later.

Comment 5

[Daniel Coloma:dcoloma]

(In reply to KM Lee [:rexboy] from comment #4)
> https://github.com/rexboy7/gaia/commits/resolution_landing
> Just arranged the whole branch based on master of 1/24 here.  Basically
> there's one commit for each app. After discussing with Tim, I would send a
> pull request and post each commit a patch file here for reviewing.
> What this patch mainly do is:
> - Put a shared css file called resolution.css, which is included in every
> apps, for detecting phone resolution and set font-size on body
> correspondingly.
> - Change unit from px/em to rem.
> - set background-size for picutres which need to be scaled on larger
> resolution.
> 
> with miscellaneous changes.
> The work started about one month ago with several merges, I tried my best to
> catch new features need to patch and you may notice me if I missed.  I'll
> start to send review request later.

We agreed with Andreas Gal that we will land the code developed for the Geeksphone Peak device that is already handling multiple resolution after freezing 1.0.0. Can we coordinate on this before landing anything?

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am not aware of any development on the Geeksphone Peak device -- until yesterday actually. Nonetheless, I am happy as long as this bug is fixed. Let's coordinate -- please suggest how we should do it.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

So, Daniel, couple of questions on works from your devs:

1) Which branch would you intend to put the multi-resolution support? Specifically, which version of Gaia is the Geeksphone Peak devices will be using? Is it v1-train, master, or a specific v1.x version only?
2) Is the WIP code publicly available somewhere?
3) What's the target resolution the code is supported currently? Taipei would like to target at least QHD and WVGA.
4) What else does the code do? I am told that some magic has been done on build script

We should be targeting landing a version where all the feature and the requirements is included, landed, and maintained. Thanks.

Comment 8

[Brendan Eich [:brendan]]

Seems best (this is standard practice for Gecko changes) always to go into mozilla-central (via mozilla-inbound) for Gecko and master for Gaia first, no?

Or are those out of date and in need of merge-back from mozilla-b2g18 and v1-train?

/be

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Brendan Eich [:brendan] from comment #8)
> Seems best (this is standard practice for Gecko changes) always to go into
> mozilla-central (via mozilla-inbound) for Gecko and master for Gaia first,
> no?
> 
> Or are those out of date and in need of merge-back from mozilla-b2g18 and
> v1-train?
> 
> /be

Yes, everything will be checked into master first. I simply want to know how many branches Tef is going to land, for this feature.

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

Hi Tim

Ismael has been working in this branch:

https://github.com/basiclines/gaia/tree/twist-nightly

As you can see there are some changes in gaia, and apps (not that many). Any way we will need to coordinate how we are going to move this changes to master.

Thanks!

Comment 11

[Daniel Coloma:dcoloma]

Yes, sorry, I forgot to follow-up on this one.

Ismael has currently sync that branch with v1.0.0, but he could easily put it in sync with master or v1-train, whatever is required. 

Ismael has mostly changes in CSS and some minor things required to have a good UX (e.g. changing the number of icon rows in the grid). 

The branch includes graphical resources already updated to other resolutions, it is designed in a way that you can decide the resolution when building Gaia, e.g. SCREEN_TYPE=qHD make install-gaia or SCREEN_TYPE=wvga make install-gaia.

Comment 12

[Ismael Gonzalez [:basiclines]]

Hi all guys just to to clarify the work done on twist-nightly branch:
* Added assets with double resolution ([e-mail]) for all the system and core apps
* Background size declarations to handle this new assets
* Makefile changes to decide when to send @2x assets or when to do not.
* Changes in a couple of applications that make some JS calcs based on 320x480 device 
* Window manager JS changes for scaling the content of wrapped apps

All this changes has been done in Gaia because of https://bugzilla.mozilla.org/show_bug.cgi?id=828531 
Gecko should be responsible in returning more CSS pixel per Device Pixels, this means that if we can fix the above bug the only necessary changes in Gaia will be:

* Add assets with double resolution ([e-mail]) for all the system and core apps
* Background size declarations to handle this new assets
* (optional)Makefile changes to decide when to send @2x assets or when to do not.

With this approach is no longer needed to use "rem", we could keep working on "px" as Gecko will give us the proper amount of pixels. Also any web content loaded from Browser app or any wrapped apps will be scaled meanwhile with the Gaia "scale hack" will be much difficult because of this https://bugzilla.mozilla.org/show_bug.cgi?id=827802

I hope this info helps everyone

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Ok. I think we need to start looking at those changes and land them on master now.

Ismael and Rexboy can you sync together in order to take the best of the 2 worlds and to have only one branch?

I gave a quick look at both branch and here are some questions:
 
- There are some background-size: 3rem; on bitmap images. Maybe having more media queries for this? Or does SVG could be a solution for that?

 - There are some css properties where the usage of rem can be discussed too (height, width, left, top, transform). Such values means the UI will fully change based on font-size. Basically you assume: one resolution == one font size, and I'm not 100% convinced this will always be true if we want to make some apps more comfortable to use for people with bad eyes. For example if an option in Settings let people configure ask for a bigger font-size (or is there a way to do that with a transparent preference?). Asking dbaron.

 - Ismael, is the reason to add a new build option is because you can't emulate device-width on Firefox responsive design tool so you prefer to include only one css file instead of having a resolution.css file like in RexBoy branch? If yes (and I tend to think even if no) let's see if the devtools team can help or have anything to propose? Adding Mihai to the discussion but I guess roc can answer if there is any way to do it at all?

Ismael I'm not 100% sure we want to use the layout.css.devPixelsPerPx preference of bug 828531. Let's ask roc about it.


I may have more questions at some point but let's work on the first set of questions already :)

Comment 14

[Ismael Gonzalez [:basiclines]]

Ok, so let me explain you guys some points:

- Gaia responsive UI work:
  * background-size rules allow us to define bigger bitmap assets (double size of our current ones), and use them for a variety of resolutions from "1 to 2" scales factor, this mean that the same image could be used for a Peak device (540x960, scale factor 1.6785) and for a Samsung Galaxy (480x600, scale factor 1.35), and we won't need to use different images for different devices.
Using SVG was discard because of low performance issues, as much SVG as you have the lower response of the device. 
  * About rem usage, the problem was on defining the html font-size to 10px, if we define 62.5% instead of 10px, we will have the 10px=1rem equality and we will allow to the user to change any preference to increase the size of the UI (via browser default font-size). I.E Peak font-size: 104% instead of 16.8px
  * About resolution.css, that was my first approach (instead of having different screens css files qhd.css, fwvga.css, etc..). This approach has some issues when you are using iframes inside your application. 
I.E communications and the contacts tab, the loaded content will have a different viewport that will not match the proper mediaquery. ( 320x460 (20px statusbar) comms viewport , contacts in comms 320x420 (40px of tabs)). So if any application use internal iframes will not match correctly.
That is the reason to do all this work on build process.
  * I'm also doing some work with images at build process (webap-zip.js), only sending to the phone the @2x version (renaming it with the original filename), this avoid having to maintain hundred of CSS files with mediaqueries for image replacing.
  * Gaia responsive UI work is not reflected on mediaquieries (min-resolution) or window.devicePixelRatio, both of them return static values right now (96 and 1), also our window.innerWidth & window.innerHeight will return device pixels instead of CSS pixels (scaled ones). That means we will have compatibilities problems with some websites/external apps.
  * Any change in Gecko that assumes to read the dpi (remember right now returns 96) of the device will not be reflected and we will have upcoming bugs (right now we have some problems in Peak device with touch areas because of https://bugzilla.mozilla.org/show_bug.cgi?id=823619)


- Gecko responsive UI work, this should be the way to follow, why?
  * To avoid the last 2 points from the previous explanation (Gaia responsive UI work)
  * Requires lees change sin Gaia (just bigger assets and some background-size)
  * W3C specification (i can't find the link right now) suggest that is the UA who has to manage this scale work if the device dpi is much larger than normal (normal is 160 in our phones but Peak almost have 250).
  * Some ideas that are being discussed right now in the W3C about device adaptation http://dev.w3.org/csswg/css-device-adapt/

Remember that Gecko right now is ready to do all this works, it has some bugs, but is more less ready: https://bugzilla.mozilla.org/show_bug.cgi?id=828531 


Thx!

Comment 15

[Mihai Sucan [:msucan]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #13)
>  - Ismael, is the reason to add a new build option is because you can't
> emulate device-width on Firefox responsive design tool so you prefer to
> include only one css file instead of having a resolution.css file like in
> RexBoy branch? If yes (and I tend to think even if no) let's see if the
> devtools team can help or have anything to propose? Adding Mihai to the
> discussion but I guess roc can answer if there is any way to do it at all?

Paul Rouget did a lot of the work on the dev tools UI (most of it?). I do not have much experience with complex UIs, but if you have any specific questions please let me know - I can try to help.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please read the discussion in bug 796490 before replying :-).

I think it's very important that we leverage Gecko here and set layout.css.devPixelsPerPx to something other than 1.0 for high-DPI devices, and fix any Gecko bugs you encounter with that. You should be able to use px units for your UI and get results that look visually reasonable no matter what the device DPI is. This is the way the Web works and FFOS should follow.

Device-pixel screen size and device-pixel density are separate issues. Here it seems we're mostly talking about density although in the bug summary, "QHD" and "WVGA" are about device-pixel screen sizes.

Comment 17

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Let me do some explain on resolution.css:
As far as I know when using media query, width and height do always depends on iframe width and height thus causes problem on apps using iframe.  But we can use device-width and device-height instead, which depends on real device width/height no matter the iframe size to overcome the problem.  Another issue is that orientation properties in media query also depends on iframe width/height, so portrait/landscape mode really determines whether it's an "upright" iframe, which is not what we wanted.  So at last I determine whether it's landscape or portrait by watching whether device-aspect-ratio is greater/lesser than 1.  I found the problem above when tuning statusbar (an Iframe with 320x20px).  Isamel is this the problem you encountered?

I didn't find a way to solve wrapped apps problem Ismael mentioned.  That's a great work.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Hi Ismael,

It looks like the part requires more clarification is about devicePixelRatio. I agree with what roc said in comment 16, however that means more work for qHD support.

Is it possible, with Rexboy's help, we could land WVGA support first in master, in this week? Taipei is going away for one week holiday next week. Tell me what you need and I'll see what I could do.

Comment 19

[Ismael Gonzalez [:basiclines]]

@Mihai Thx you, right now i think the developers tools cover all our needs in this matter. @Vivien, having changes in build process is not about i can't emulate device size in emulator

@Robert i'm totally agree with you, and yes we are basically talking about dpi.

@KM Lee Yes that the problem that i found :D, anyway using this resolution.css (or qhd.css, etc...) is an attempt to emulate the scale process in Gaia side, as @Robert pointed this should be done at Gecko level.

@Tim landing wvga is the same as landing qhd, as the changes i can imagine are 
traversals for both resolutions.
So we should know how many Gecko effort do we need to fix layout.css.devPixelsPerPx,
in that way the things that we can already start landing (both compatible with Gecko scale and Gaia scale approach) are background-size declarations and @2x assets.

Also i will like to know @Vivien opinion about having some build process to determine when to send @2x assets to the phone or when to do not. (this is already implemented on twist-nightly branch) or it must be the applications CSS files which with mediaqueries should determine it (remember we need support for min-resolution to follow this approach)

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Ismael from comment #19)
> @Tim landing wvga is the same as landing qhd, as the changes i can imagine
> are 
> traversals for both resolutions.
> So we should know how many Gecko effort do we need to fix
> layout.css.devPixelsPerPx,
> in that way the things that we can already start landing (both compatible
> with Gecko scale and Gaia scale approach) are background-size declarations
> and @2x assets.

OK. That's good to know.

I am a bit confused and concerned on what you expect the Gecko should do. Reading your comment in the code here:

https://github.com/basiclines/gaia/blob/twist-nightly/shared/screens/fwvga.css

> B2G always returns with 'min-resolution' 96dpi instead of real dpi (equal to Macbook Pro 13' dpi's)

A) This is a bug that should be fixed. However, this should not affect our ability to target screens with min-device-width/height.

> B2G always returns with 'min--moz-device-pixel-ratio' 1 instead of real pixel ratio.

B) This is also a bug that should be fixed in Gecko, However, this should not affect our ability to support WVGA since it is still = 1. (It's 2 in qHD though)

> We are not using any media query because of applications internal iframes size will not match the device query

C) min-height will not match the size of the screen, and it's _by design_. What will match is always min-device-width/height.

Also, read bug 796490. roc, correct me if I am wrong, |layout.css.devPixelsPerPx| will scale up/down the entire web content away from the device pixel grid, which will make the image blurry... it's not something we are looking for, and we shouldn't expect Gecko to do this for web content.

That's what I meant we can land WVGA support without Gecko fixing (A) and (B). |rem| trick will work here and we can depend on that even after Gecko fixes those bugs. Additionally, having Makefile to delete the @2x files is a great feature that we should land it.

Let's start land WVGA support by submitting review requests for CSS/JS changes app by app. It's that OK for you, Ismael? Fake QHD support can also be included by targeting until we find out how to fix (B) in Gecko.

roc, I believe min-resolution media query is hooked to |layout.css.dpi|, is that correct? What CSS unit will the perf affects? With the information we could solve (A). And, how do we correctly set moz-device-pixel-ratio to 2? |layout.css.devPixelsPerPx| is not the answer here, obviously.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #20)
> Also, read bug 796490. roc, correct me if I am wrong,
> |layout.css.devPixelsPerPx| will scale up/down the entire web content away
> from the device pixel grid, which will make the image blurry... it's not
> something we are looking for, and we shouldn't expect Gecko to do this for
> web content.

Most Web content scales fine. We snap borders etc to pixel edges to avoid blurriness in most cases. For images, you can avoid scaling the image by providing a higher-resolution image. For example if devPixelsPerPx is 1.5 and you provide a 150x150 image for an element with background-size:100px 100px, Gecko won't need to scale it and it'll look great.

The effect of setting layout.css.devPixelsPerPx is very much like full-zoom in desktop Firefox. So it's easy to experiment to see how things will look.

> roc, I believe min-resolution media query is hooked to |layout.css.dpi|, is
> that correct? What CSS unit will the perf affects? With the information we
> could solve (A). And, how do we correctly set moz-device-pixel-ratio to 2?
> |layout.css.devPixelsPerPx| is not the answer here, obviously.

Actually it is.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW min-resolution is a bit confusing in CSS because "min-resolution:Ndpi" refers to CSS inches, not physical inches, so it doesn't correspond to the "actual DPI" of the screen. device-pixel-ratio is less confusing.

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #20)
> > Also, read bug 796490. roc, correct me if I am wrong,
> > |layout.css.devPixelsPerPx| will scale up/down the entire web content away
> > from the device pixel grid, which will make the image blurry... it's not
> > something we are looking for, and we shouldn't expect Gecko to do this for
> > web content.
> 
> Most Web content scales fine. We snap borders etc to pixel edges to avoid
> blurriness in most cases. For images, you can avoid scaling the image by
> providing a higher-resolution image. For example if devPixelsPerPx is 1.5
> and you provide a 150x150 image for an element with background-size:100px
> 100px, Gecko won't need to scale it and it'll look great.
> 
> The effect of setting layout.css.devPixelsPerPx is very much like full-zoom
> in desktop Firefox. So it's easy to experiment to see how things will look.
> 
> > roc, I believe min-resolution media query is hooked to |layout.css.dpi|, is
> > that correct? What CSS unit will the perf affects? With the information we
> > could solve (A). And, how do we correctly set moz-device-pixel-ratio to 2?
> > |layout.css.devPixelsPerPx| is not the answer here, obviously.
> 
> Actually it is.

Thanks for the clarification. That means we will be using |layout.css.devPixelsPerPx = 2| for HiDPI devices, and fix OOM/OTMA issues with it, which Ismael have already filed some bugs.

Ismael, I am still not hear anything from you since comment 20... are you occupied on other works? Do you need Rex to take over and set review/land your patches instead?

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

oops, s/OOM/OOP/, out-of-process (rendering) issues.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #13)
>  - There are some css properties where the usage of rem can be discussed too
> (height, width, left, top, transform). Such values means the UI will fully
> change based on font-size. Basically you assume: one resolution == one font
> size, and I'm not 100% convinced this will always be true if we want to make
> some apps more comfortable to use for people with bad eyes. For example if
> an option in Settings let people configure ask for a bigger font-size (or is
> there a way to do that with a transparent preference?). Asking dbaron.

I'm not sure what the question is, though the use of 'rem' for resolution-independence has always seemed pretty fishy to me.

(If you want to ask me questions, better to use needinfo?.)

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Given the fact we should be tweaking Gecko for support HiDPI devices, I am split this into two bugs.

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #13)
> > Basically you assume: one resolution == one font size
IMO this is incorrect. For a certain size, we will be targeting a range of screen dimensions with media queries. The goal is not to make the physical size of the text look exactly the same on every device.

> > For example if
> > an option in Settings let people configure ask for a bigger font-size (or is
> > there a way to do that with a transparent preference?)

I don't think accessibility is the topic of the bug here, though I agree the fix here shouldn't limit our options.

(In reply to David Baron [:dbaron] from comment #25)
> I'm not sure what the question is, though the use of 'rem' for
> resolution-independence has always seemed pretty fishy to me.

It looks fishy to me too. However I cannot come up with a better alternative, without wrapping a large part of style into media query block for each resolution. That's just a trick people come up so that we could target a resolution in 1 lines like

@media (...) { html { font-size: <new rem base number here>px } }

Until there is a better alternative ...

Comment 28

[Ismael Gonzalez [:basiclines]]

@Tim Sorry for not replaying again, i haven't so much time, but i've been reading you guys.

> A) This is a bug that should be fixed. However, this should not affect our ability to target screens with min-device-width/height.
> B) This is also a bug that should be fixed in Gecko, However, this should not affect our ability to support WVGA since it is still = 1. (It's 2 in qHD though)

This affects external developers ability to swap images for different resolutions, but we can go without it by now if we use the makefile to send that kind of images to the phone (already implemented on twist-nightly).

> C) min-height will not match the size of the screen, and it's _by design_. What will match is always min-device-width/height.

I'm agree on using only one resolution.css (with mediaqueries for device-height) instead of qhd.css or fwvga.css, seems pretty much nicer.

@Robert
> The effect of setting layout.css.devPixelsPerPx is very much like full-zoom in desktop Firefox. So it's easy to experiment to see how things will look.

This also provides mediaqueries and devicePixelRatio to return the correct values.

@Tim, @David
> I'm not sure what the question is, though the use of 'rem' for resolution-independence has always seemed pretty fishy to me.
(If you want to ask me questions, better to use needinfo?.)

> I don't think accessibility is the topic of the bug here, though I agree the fix here shouldn't limit our options.

I think @Vivien question was more/less clarified in the above comments, we can keep accessibility features and bulletproof scaled UI:
> * About rem usage, the problem was on defining the html font-size to 10px, if we define 62.5% instead of 10px, we will have the 10px=1rem equality and we will allow to the user to change any preference to increase the size of the UI (via browser default font-size). I.E Peak font-size: 104% instead of 16.8px


So, final conclusions, just to clarify:
1) We should land WVGA with Gaia patches (me and rexboy)
2) We will fix the gecko (when?) bugs for using layout.css.devPixelsPerPx in QHD resolutions and any other one

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Ismael from comment #28)
> So, final conclusions, just to clarify:
> 1) We should land WVGA with Gaia patches (me and rexboy)
> 2) We will fix the gecko (when?) bugs for using layout.css.devPixelsPerPx in
> QHD resolutions and any other one

YES. Thank you :)

Comment 30

[Daniel Coloma:dcoloma]

I think we are mixing two topics here:

1/ The fixes in Gecko (that should be done for any resolution that is not the standard one HVGA)
2/ The adaptations in Gaia to support multiple resoultions, including the hack to bypass Gecko limitations for the time being. These adapations currently work for multiple resolutions (including WVGA and qHD)

This bug should be about landing 2/ so that Gaia can already work for those resolutions. qHD is essential to be landed asap as it is already used in Firefox OS developer devices. 

Ismael will work starting tomorrow on 2/ as he has been focused so far in finishing the version that will be used for FirefoxOS Peak device for mass production.

Comment 31

[KM Lee [:rexboy] (inactivate after Sep. 22)]

If we decide to use resolution.css just include it in index.html need it and things should work.  I did a quick merge putting resolution.css on and remove wvga.css/qhd.css and seems there are no significant UI break.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Daniel Coloma:dcoloma from comment #30)
> 2/ The adaptations in Gaia to support multiple resoultions, including the
> hack to bypass Gecko limitations for the time being.

What hack is that, exactly?

Also, when you say "resolution", do you mean screen size or do you mean pixel density? I think it would be best to not use the word "resolution" at all :-)

Comment 33

[Ismael Gonzalez [:basiclines]]

> What hack is that, exactly?
@Daniel means the work done by rexboy and me on Gaia to emulate the scaled UI (with rem units), until we have a better approach from Gecko side

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why exactly can't we just set layout.css.devPixelsPerPx right now and proceed without the rem hack?

I'm aware of bug 828531. We have a developer working on it and I expect it will get fixed very soon. In the meantime it shouldn't block your work since it only happens while transitions/animations are running.

Comment 35

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Ismael from comment #33)
> > What hack is that, exactly?
> @Daniel means the work done by rexboy and me on Gaia to emulate the scaled
> UI (with rem units), until we have a better approach from Gecko side

Ismael, you are not reading my comment 27.

rem is not a "hack", is a CSS unit that we are going to leverage to get Gaia ships in multi-dimension with the smallest possible media query code. Unless you got a better alternative...

|layout.css.devPixelsPerPx| and other Gecko work will not replace that, and it's legit use for us to support HiDPI device will be addressed in bug 838505.

Taipei is slip into long holiday in like 3 hours ... we really don't have time for this kind of miscommunication.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> Why exactly can't we just set layout.css.devPixelsPerPx right now and
> proceed without the rem hack?

Repeat, we are NOT going to adjust |layout.css.devPixelsPerPx| to a non-integer to resize Gaia. We should resize Gaia in CSS, as all mobile web developer do.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

"rem" is a hack because CSS provides a resolution-independent unit, which is "px", and that's the unit we should be using.

> Repeat, we are NOT going to adjust |layout.css.devPixelsPerPx| to a non-integer
> to resize Gaia.

Why not?

> We should resize Gaia in CSS, as all mobile web developer do.

I don't know what you're referring to.

Comment 37

[Ismael Gonzalez [:basiclines]]

:timdream I've read your comment #27 and I'm agree in using 'rem' and a resolution.css with "%" instead of "px" for satisfying :vingtetun suggestions.

Anyway I'm totally agree with :roc, Gecko should be the responsible for it.
This matter is enough important to give it a chance to discuss properly. Maybe we should involve more people from platform to discuss it.

Meanwhile i will start landing the fixes needed for both scales methods (from Gecko and Gaia, background-size css declarations, higher resolution assets, and some Makefile additions)
By the way :roc still using 'rem' is not a bad idea, as it is directly relational with "px".

Comment 38

[Ismael Gonzalez [:basiclines]]

Attachment: PR

Comment 39

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What's the question?

Comment 40

[Ismael Gonzalez [:basiclines]]

[:cjones] The question is: Should be Gecko responsible via layout.css.devPixelsPerPx for scaling Gaia or it should be Gaia the responsible for it via "rem" and root font size adjusts?

Comment 41

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Generally I want to do whatever roc suggests :).  However, we have to ensure that pages like games can "see" the real resolution.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For canvas to work optimally with layout.css.devPixelsPerPx != 1 we need to fix bug 780362. We can prioritize that if it's needed. However, existing canvas code should work OK with layout.css.devPixelsPerPx != 1, it'll just be a bit less crisp.

Comment 43

[Jose Manuel Cantera]

Comment on attachment 712479 [details]
PR

the proposed approach for dealing with 2x resources looks good to me, although it would need thorough review from makefile owners. Concerning discussions on which layer should be scaling +1 to do in Gecko as it is more a platform issue than a Gaia issue. However, I still believe well-designed apps should define measurements in rem and not in px, just to enable future accessibility or zooming requirements. 

Concerning the games issue pointed out by Chris I think that's a very good point and we should ensure that automatic translation between logical and physical pixels do not break other apps.

Comment 44

[Jose Manuel Cantera]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #41)
> Generally I want to do whatever roc suggests :).  However, we have to ensure
> that pages like games can "see" the real resolution.

Shouldn't this be controlled by using proper meta viewport directives?, like target-densitydpi=device-dpi which has been available in Android for a long time. 

http://developer.android.com/guide/webapps/targeting.html

There are related discussions in Webkit at

https://lists.webkit.org/pipermail/webkit-dev/2012-May/020847.html and related bug at

https://bugs.webkit.org/show_bug.cgi?id=88047

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jose M. Cantera from comment #43)
> the proposed approach for dealing with 2x resources looks good to me,
> although it would need thorough review from makefile owners. Concerning
> discussions on which layer should be scaling +1 to do in Gecko as it is more
> a platform issue than a Gaia issue. However, I still believe well-designed
> apps should define measurements in rem and not in px, just to enable future
> accessibility or zooming requirements. 

You can use rem as well as setting layout.css.devPixelsPerPx, if you want, but that seems like overkill.

> Concerning the games issue pointed out by Chris I think that's a very good
> point and we should ensure that automatic translation between logical and
> physical pixels do not break other apps.

We have a lot of experience with varying devPixelsPerPx on desktop since it's exactly the same mechanism that we use for "full zoom" of Web pages. It generally works well.

Comment 46

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 712479 [details]
PR

Hello, the media query seems OK to me but a little doubt if it works on both landscape/portrait mode.
You may see the PR on github.  Thanks!

Comment 47

[Ismael Gonzalez [:basiclines]]

Comments addressed :D

Comment 48

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Sorry for not looking at this sooner - I'm busy with some performance issues and I have an hard time to focus on the review. I feel like Tim can do it (and fwiw I feel like we should follow roc suggestions about the pref).

Comment 49

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 712479 [details]
PR

The general approach of this part is correct, but I will strongly against we land this as-is:

0) Various typo and grammar error in comments. Having ambiguous comment is worse than having no comment.
1) Instead of rewriting settings.py (why would you do that?), please pass the HIDPI parameter to the files, and patch the code respond to the parameter.
2) Why would you need to rewrite the Makefile?
3) The two checks in webapp-zip.js looks like duplicates from me. I thought the only thing you want to do in addToZip() is
-- if (HIDPI && this_is_1x_file && the_2x_file_exists ) return;
-- if (!HIDPI && this_is_2x_file && the_1x_file_exists ) return;
right?
4) I suppose responsive.css will be included to all apps as shared style. That works.
5) I am not sure the 94% in |html { font-size: 94%!important; }| is intentional or not. 94% of what value you are referring too? Looks to me this will result the font-size being set to 15.033px, which is 94% of the ua default (16px). I would recommend setting the value to a literal pixel value.
6) In the media query, |orientation| should be |device-aspect-ratio|. |orientation| is by design responsive to the size of the app iframe, not the real orientation of the phone screen.
7) Please remove the qHD entry in responsive.css, as the setting right now is a incorrect workaround. for qHD device (540×960, device-pixel-ratio=2), Gaia should resize to 270x480 (css-)pixels, and have Gecko to map 2 device pixels to 1 css pixel -- but all that should be addressed in bug 838505, not here.

Comment 50

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #45)
> (In reply to Jose M. Cantera from comment #43)
> > the proposed approach for dealing with 2x resources looks good to me,
> > although it would need thorough review from makefile owners. Concerning
> > discussions on which layer should be scaling +1 to do in Gecko as it is more
> > a platform issue than a Gaia issue. However, I still believe well-designed
> > apps should define measurements in rem and not in px, just to enable future
> > accessibility or zooming requirements. 
> 
> You can use rem as well as setting layout.css.devPixelsPerPx, if you want,
> but that seems like overkill.

That's exactly my intention, and no, that's an overkill.

Web content (e.g. Gaia), especially web apps, should always be fluid to device dimensions, and web platform (e.g. Gecko) should be reliable enough to expose the right parameters to web contents. A qHD device by definition is 540×960 (physical-)pixels and device-pixel-ratio=2.

What I strongly against was to have Gaia ignore all that differences (and remove the rem work), and set Gecko pref of device-pixel-ratio to an arbitrary number (say, 2.232) just because the font look too small. I know Gecko should works well with the pref, but that's not right.

> We have a lot of experience with varying devPixelsPerPx on desktop since
> it's exactly the same mechanism that we use for "full zoom" of Web pages. It
> generally works well.

Good to know! I remembered back in the day when full page zoom was first introduced in Firefox, the scolling will be slow as hell when the content is zoomed. That has improved a lot to a point that I have ever set Facebook to zoom -- until I accidentally hit Ctrl+0 one day.

Comment 51

[Ismael Gonzalez [:basiclines]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #49)
> Comment on attachment 712479 [details]
> PR
> 
> The general approach of this part is correct, but I will strongly against we
> land this as-is:
> 
> 0) Various typo and grammar error in comments. Having ambiguous comment is
> worse than having no comment.

I will fix it, that's an easy thing

> 1) Instead of rewriting settings.py (why would you do that?), please pass
> the HIDPI parameter to the files, and patch the code respond to the
> parameter.

Ok, sounds a better solution :D (the rewriting stuff was a legacy solution)

> 2) Why would you need to rewrite the Makefile?

Same as above

> 3) The two checks in webapp-zip.js looks like duplicates from me. I thought
> the only thing you want to do in addToZip() is
> -- if (HIDPI && this_is_1x_file && the_2x_file_exists ) return;
> -- if (!HIDPI && this_is_2x_file && the_1x_file_exists ) return;
> right?

There are 2 check because of branded shared resources function, it only pass to the zip the linked branded shared resources (instead of scanning all the folder files as the regular function), that means i will never pass an @2x file. So, I'm forcing to pass that branded asset as "@2x" and if it exists will be added to the zip also.


> 4) I suppose responsive.css will be included to all apps as shared style.
> That works.

It will be added in next PR, we want to keep this merging step by step.

> 5) I am not sure the 94% in |html { font-size: 94%!important; }| is
> intentional or not. 94% of what value you are referring too? Looks to me
> this will result the font-size being set to 15.033px, which is 94% of the ua
> default (16px). I would recommend setting the value to a literal pixel value.

Default ua font-size is 16px sure, and 62.5% of 16px is equal to 10px, that is our baseline. So 62.5% * scale_ratio = wvga 94%, qhd 105%
I'm sure this calcs are solid and flexible enough to be used. And also covers the case that concerns Vivien about accessibility.

> 6) In the media query, |orientation| should be |device-aspect-ratio|.
> |orientation| is by design responsive to the size of the app iframe, not the
> real orientation of the phone screen.

This has been already fixed, as a suggestion of :rexboy

> 7) Please remove the qHD entry in responsive.css, as the setting right now
> is a incorrect workaround. for qHD device (540×960, device-pixel-ratio=2),
> Gaia should resize to 270x480 (css-)pixels, and have Gecko to map 2 device
> pixels to 1 css pixel -- but all that should be addressed in bug 838505, not
> here.

If we remove the qhd rules we should remove also the wvga ones, as both of them are totally above from our base screen resolution (320x480:163).
The bug https://bugzilla.mozilla.org/show_bug.cgi?id=828531 has been already fixed and landed on mc. That means we won't need the whole responsive.css file anymore.
In the other way, if you want to keep this file having the rules for qhd does not creates any conflict with wvga ones, and there is no reason to remove it, as both screens are suppose to be supported by Gaia.

Comment 52

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 712479 [details]
PR

Looks good with comment addressed on Github, thank you!

Comment 53

[Ismael Gonzalez [:basiclines]]

Merged in master: https://github.com/mozilla-b2g/gaia/commit/cf81c9281538d9d94cbccf3844cc7896eb8ab505

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #50)
> Web content (e.g. Gaia), especially web apps, should always be fluid to
> device dimensions, and web platform (e.g. Gecko) should be reliable enough
> to expose the right parameters to web contents. A qHD device by definition
> is 540×960 (physical-)pixels and device-pixel-ratio=2.

device-pixel-ratio depends on the physical screen size and viewing distance. As far as I know qHD is just 960x540 and could be any device-pixel-ratio.

> What I strongly against was to have Gaia ignore all that differences (and
> remove the rem work), and set Gecko pref of device-pixel-ratio to an
> arbitrary number (say, 2.232) just because the font look too small. I know
> Gecko should works well with the pref, but that's not right.

Sizing text in rems makes some sense. Sizing other things like images in rems makes less sense IMHO.

Comment 55

[Ismael Gonzalez [:basiclines]]

Attachment: lockscreen PR

Comment 56

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 712479 [details]
PR

Already reviewed by Tim :)

Comment 57

[Jose Manuel Cantera]

(In reply to Jose M. Cantera from comment #44)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #41)
> > Generally I want to do whatever roc suggests :).  However, we have to ensure
> > that pages like games can "see" the real resolution.
> 
> Shouldn't this be controlled by using proper meta viewport directives?, like
> target-densitydpi=device-dpi which has been available in Android for a long
> time. 
> 
> http://developer.android.com/guide/webapps/targeting.html
> 
> There are related discussions in Webkit at
> 
> https://lists.webkit.org/pipermail/webkit-dev/2012-May/020847.html and
> related bug at
> 
> https://bugs.webkit.org/show_bug.cgi?id=88047

A related bug is bug 677989

Comment 58

[Ismael Gonzalez [:basiclines]]

Lockscreen PR merged: https://github.com/mozilla-b2g/gaia/commit/a9e47fcc417ebe102c6aa1bd5de0ebeb9c95fe1b

Comment 59

[Ismael Gonzalez [:basiclines]]

Attachment: homescreen PR

Comment 60

[Dan Heberden]

I'm not sure why PR https://github.com/mozilla-b2g/gaia/pull/8297 was merged - it seems to be causing adverse effects in B2G Desktop http://danheberden.com/share/cf3062b.png - jugglinmike was also able to confirm. 

How I verified: 
git reset --hard 91d345428 # problem non-existant
git reset --hard a9e47fcc41 # problem introduced

Commit: https://github.com/mozilla-b2g/gaia/commit/a9e47fcc417ebe102c6aa1bd5de0ebeb9c95fe1b

Comment 61

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 718447 [details]
homescreen PR

This r+ is only valid once Rex give his f+.

Comment 62

[Jose Manuel Cantera]

Comment on attachment 718447 [details]
homescreen PR

delegating my feedback to Cristian, as he knows perfectly the HS implications. By the way, it would be also nice test that all the subsequent PRs do not break anything in B2G desktop. 

thanks!

Comment 64

[Cristian Rodriguez (:crdlc)]

Comment on attachment 718447 [details]
homescreen PR

My feedback is + but review this comment https://github.com/mozilla-b2g/gaia/pull/8335#discussion_r3167535 that is very important

Comment 65

[Ismael Gonzalez [:basiclines]]

(In reply to danheberden from comment #60)
> I'm not sure why PR https://github.com/mozilla-b2g/gaia/pull/8297 was merged
> - it seems to be causing adverse effects in B2G Desktop
> http://danheberden.com/share/cf3062b.png - jugglinmike was also able to
> confirm. 
> 
> How I verified: 
> git reset --hard 91d345428 # problem non-existant
> git reset --hard a9e47fcc41 # problem introduced
> 
> Commit:
> https://github.com/mozilla-b2g/gaia/commit/
> a9e47fcc417ebe102c6aa1bd5de0ebeb9c95fe1b

That is happening because b2g desktop does not return the proper value for min-device-width, it return computer screen width instead b2g frame.

I will open a bug if there is no one open for this issue

Comment 66

[Ismael Gonzalez [:basiclines]]

Homescreen PR merged: https://github.com/mozilla-b2g/gaia/commit/78afd0dd4e15ced437e1f4c4958977e8e0e2f7e7

Comment 67

[Ismael Gonzalez [:basiclines]]

Attachment: shared folder PR

Comment 68

[Ismael Gonzalez [:basiclines]]

Attachment: sms pr

Comment 69

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Ismael from comment #65)
> 
> That is happening because b2g desktop does not return the proper value for
> min-device-width, it return computer screen width instead b2g frame.
> 
> I will open a bug if there is no one open for this issue

Hmmm I think we can workaround it simply by changing 
@media all　…
to
@media handheld　…
inside responsive.css to make media query affects only mobile devices, but I haven't tested it yet.  Ideally this should work.

Comment 70

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 720759 [details]
sms pr

[e-mail] is not updated to the correct size.

Do you want to isolate the responsive.css fix and land it first?

Comment 71

[Ismael Gonzalez [:basiclines]]

Tim i don't know what do you mean about [e-mail] is not updated to the correct size. Is just twice times bigger than the normal one.

About the responsive.css fix, i think if i can get the r+ from sms owners we can land it today. Anyway if the review takes too much time i will land it isolated.

Thx!

Comment 72

[Jose Manuel Cantera]

Comment on attachment 720759 [details]
sms pr

it was working ok although there is the problem you mention with the keyboard. Please Borja have a look at it before landing this.

Comment 73

[Borja Salguero [:borjasalguero]]

Comment on attachment 720759 [details]
sms pr

Please, address the comment and for me it's R+! ;)

Comment 74

[Ismael Gonzalez [:basiclines]]

KM Lee, handheld does not work :( so we are going to merge with this dirty patch until there is one better available!

Comment 75

[Ismael Gonzalez [:basiclines]]

sms&responsive fixes pr merged: https://github.com/mozilla-b2g/gaia/commit/400b9f1752e1dfa8b118f8ca2ebe4549a8cd0471

Comment 76

[:gerard-majax]

Attachment: SMS screenshot after pull request #8444

Hello,

I've updated my Gaia, got PR #8444 in, and when looking at the result on Nexus S, I feel the font is way too big. Please find the attached screenshot that exposes this.

Comment 77

[:gerard-majax]

Attachment: SMS picture after pull request #8444

Same content, with the full phone picture.

Comment 78

[Sergi]

(In reply to Alexandre LISSY :gerard-majax from comment #76)
> Created attachment 721172 [details]
> SMS screenshot after pull request #8444
> 
> Hello,
> 
> I've updated my Gaia, got PR #8444 in, and when looking at the result on
> Nexus S, I feel the font is way too big. Please find the attached screenshot
> that exposes this.

This is because the system is not responsive. What we do is scaling up things. The way to approach this is to either fine tune the fonts for nexus S so they look smaller, or make the system full responsive (which i think it's out of scope right now). As far as everything is automatically scaled, it's not possible to reduce the font size as this change would be propagated to the rest of screen resolutions we're supporting right now.

Comment 79

[:gerard-majax]

(In reply to Sergi from comment #78)
> (In reply to Alexandre LISSY :gerard-majax from comment #76)
> > Created attachment 721172 [details]
> > SMS screenshot after pull request #8444
> > 
> > Hello,
> > 
> > I've updated my Gaia, got PR #8444 in, and when looking at the result on
> > Nexus S, I feel the font is way too big. Please find the attached screenshot
> > that exposes this.
> 
> This is because the system is not responsive. What we do is scaling up
> things. The way to approach this is to either fine tune the fonts for nexus
> S so they look smaller, or make the system full responsive (which i think
> it's out of scope right now). As far as everything is automatically scaled,
> it's not possible to reduce the font size as this change would be propagated
> to the rest of screen resolutions we're supporting right now.

Nexus S is 480x800, it's below qHD of the Peak device. Does this means fonts will be that big on Peak ? Right now, the only solution would be to fix font for Nexus S ?

Comment 80

[Sergi]

I'm afraid that's correct. Maybe Ismael can correct me if i'm wrong. Right now we are scaling proportionally based in the font size which is already defined in the CSS. Changing something to fine tune the fonts for the Nexus S (WVGA) will screw up what we have for HVGA and qHD.

Anyway i have a Geeksphone Peak with me right now, we've spent some time in adapting visuals, and i don't feel like the fonts are that big.

Comment 81

[Sergi]

Attachment: SMS :: HVGA vs qHD :: Picture

Find attached a sample picture on how the SMS app looks in a Geeksphone Peak (qHD) and a Unagi (HVGA). Although the aspect ratio is different, and that makes we have more vertical space, fonts are scaled up proportionally based in the base font size defined.

Comment 82

[:gerard-majax]

(In reply to Sergi from comment #81)
> Created attachment 721296 [details]
> SMS :: HVGA vs qHD :: Picture
> 
> Find attached a sample picture on how the SMS app looks in a Geeksphone Peak
> (qHD) and a Unagi (HVGA). Although the aspect ratio is different, and that
> makes we have more vertical space, fonts are scaled up proportionally based
> in the base font size defined.

Yep, that's what basiclines explained to me on IRC a couple of hours ago. I still think, even if it might not be the best place for this, that the final rendering makes it too big. I don't remember having the same feeling when seeing Unagi, Otoro or Geeksphone devices.

Comment 83

[:gerard-majax]

I did aquick hack testing:
$ git diff
diff --git a/shared/style/responsive.css b/shared/style/responsive.css
index 2bb145e..890f0d1 100644
--- a/shared/style/responsive.css
+++ b/shared/style/responsive.css
@@ -19,12 +19,12 @@
 */
 /*Portrait*/
 @media all and (min-device-width: 480px) and (max-device-width: 540px) and (max-device-aspect-ratio: 1/1) {
-  html { font-size: 94%!important; }
+  html { font-size: 80%!important; }
 }
 
 /*Landscape*/
 @media all and (min-device-width: 800px) and (max-device-width: 960px) and (min-device-aspect-ratio: 1/1) {
-  html { font-size: 94%!important; }
+  html { font-size: 80%!important; }
 }

I my opinion, it gives a better user experience: fonts are readable, icons are still a bit too big, but it's manageable, and it makes a better use of the screen space available. But it's just my personnal feeling, comparing my daily use on FirefoxOS and Android 2.3 (WVGA too).

Comment 84

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Ismael from comment #71)
> Tim i don't know what do you mean about [e-mail] is not
> updated to the correct size. Is just twice times bigger than the normal one.
> 

My mistake. I was in a hurry...

> About the responsive.css fix, i think if i can get the r+ from sms owners we
> can land it today. Anyway if the review takes too much time i will land it
> isolated.
> 
> Thx!

It's fine. Thanks.

Comment 85

[Ismael Gonzalez [:basiclines]]

Attachment: Browser pr

Comment 86

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 721635 [details]
Browser pr

I'll leave this to Ben.

Comment 87

[Patryk Adamczyk [:patryk] UX]

Just caught this bug. Is there a spec you are working against? Reading the comments I can't tell if the following challenges for responsive UI are addressed:

1. The visual design team is investigating various ways to reduce the graphic foot print, ie. putting the action icons into a font rather than keeping them as bitmaps at various sizes. But! To fully support various screen sizes we need 5+ bitmap sizes, with the current 320x480 resolution being the smallest. Are we splitting up gaia into 5+ graphic repos. If not how will a bigger graphic foot print fit onto an Otoro that already is severely space constraint. BTW the rough calculation goes like 2x pixels = 4x bytes. Meaning if all our graphics now are 10MB on a 320x480 device to full support various phone (5) screens like Android it will take up roughly 360MB. 

2. We simply can not have the same layout for a 320x480 3.5" @165ppi screen as we would for a 1080x1920 4.8" @ 432ppi screen due to finger reach. On the web responsive websites often have 2-3 different layouts depending on screen size, is this being built in? We shouldn't just enlarge every thing up to the resolution required. When designing for bigger screens there are advantages we need to exploit, like the addition of 1 more row of icons or an extra row of text in a list.

3. Font sizes & touch targets. Currently at 320x480 the default font size for many apps is either 17 or 19px. I really wanted to use physical sizes (mozmm) to optimize consistency and readability. What is our strategy to maintain minimum touch target sizes (atleast 5.5mm) and minimum font sizes (6pt smallest, 8pt default) in the responsive design?

Realistically we need to support around 15 layouts in each orientation per app to be responsive. I hope we build that in now, then spend months around v.2 doing this or need to depending on out sourcing all this work.

Comment 88

[Dan Heberden]

I am probably misreading this, so please help me understand more clearly :) I'm concerned about the "15 layouts in each orientation per app" part. We have a great deal of layout opportunity since we can use CSS. 

But perhaps you just mean "layouts" as some rough idea at what a, say, percentage scale on a nav bar would look like at various resolutions?

Comment 89

[Ismael Gonzalez [:basiclines]]

(In reply to Patryk Adamczyk [:patryk] UX from comment #87)
> Just caught this bug. Is there a spec you are working against? Reading the
> comments I can't tell if the following challenges for responsive UI are
> addressed:

I can "understand" that you were not aware of this bug from the work done by Tef (not really, but ok).
But there was a lot of effort also from your mates in Taipei about this same issue.
So you should have been aware about all this work.

> 1. The visual design team is investigating various ways to reduce the
> graphic foot print, ie. putting the action icons into a font rather than
> keeping them as bitmaps at various sizes. But! To fully support various
> screen sizes we need 5+ bitmap sizes, with the current 320x480 resolution
> being the smallest. Are we splitting up gaia into 5+ graphic repos. If not
> how will a bigger graphic foot print fit onto an Otoro that already is
> severely space constraint. BTW the rough calculation goes like 2x pixels =
> 4x bytes. Meaning if all our graphics now are 10MB on a 320x480 device to
> full support various phone (5) screens like Android it will take up roughly
> 360MB. 

That is not true, we don't need 5 kind of bitmaps, by now we are just using 2, normal ones and HIDPI ones (@2x double sized). Then we scale down the big ones (for HIDPI devices). And yes this is fairly good at visual requirements.

Please also summarize these 5 different screens that we have. I only got 3 (320x480, 480x800, 540x960).

> 2. We simply can not have the same layout for a 320x480 3.5" @165ppi screen
> as we would for a 1080x1920 4.8" @ 432ppi screen due to finger reach. On the
> web responsive websites often have 2-3 different layouts depending on screen
> size, is this being built in? We shouldn't just enlarge every thing up to
> the resolution required. When designing for bigger screens there are
> advantages we need to exploit, like the addition of 1 more row of icons or
> an extra row of text in a list.

I don't think that right now (or ever) exists a screen with that features:
"1080x1920 4.8" @ 432ppi" I can imagine you are comparing to Samsung GS4 (not released yet) and is just an example but please use real world examples for real world problems.

If we are talking about mobile devices i think we are talking about the same layouts in most cases, i mean just scale up the content for HIDPI devices and if the have an aspect ratio much larger than our base device (like Peak one) just use the extra available vertical space to show up more elements in a list(automatically, no extra effort), homescreen (already done in this Bug), etc..

This bug is aiming to achieve the scale process for mobile devices not to have a responsive system that can work in a variety of devices (tablet layouts, mobile layouts, tv layouts, etc...) 
Please don't mix it.

> 3. Font sizes & touch targets. Currently at 320x480 the default font size
> for many apps is either 17 or 19px. I really wanted to use physical sizes
> (mozmm) to optimize consistency and readability. What is our strategy to
> maintain minimum touch target sizes (atleast 5.5mm) and minimum font sizes
> (6pt smallest, 8pt default) in the responsive design?
 
Font-size & touch target are scaled up proportionally, taking unagi,Keon screens type as a base screen.

Physical css units (mozmm, pt, etc..) are for physical elements (print media) we should not use ever that units. Just px or rem units.

> Realistically we need to support around 15 layouts in each orientation per
> app to be responsive. I hope we build that in now, then spend months around
> v.2 doing this or need to depending on out sourcing all this work.

Please be focus on the Bug title (maybe my branch name is not correct at all responsive-ui), we are just talking about mobile phones layouts.

Comment 90

[Ben Francis [:benfrancis]]

Comment on attachment 721635 [details]
Browser pr

I've noticed a small regression with header-divider.png not being displayed correctly in the tab tray at 320x480 resolution.

Apart from that I can't spot any obvious regressions caused by this patch so *with that fixed* I can give r+me.

I'm a little concerned how future-proof the general approach is here, and using rem and % to size background images is a little concerning too, but I don't want to hold up this whole bug just from the browser patch.

As a general comment, we will have a *lot* more than 3 different resolutions to optimise for!

Comment 91

[Sergi]

I think we are mixing things here...

For me one thing is resolution and the other one is display's screen size. The responsive approach, that means designing different layouts for the same app, makes sense when we have different screen sizes, like for example going from a smartphone to a tablet. In this case it makes sense to have a responsive layout, and load one or the other depending on the device. It's obvious that, having more physical space, it doesn't make sense to just scale things up, but providing the user a different way of doing things taking advantage of the bigger screen size.

Another thing is when we have different screen resolution but the screen size is similar. For example when we want to adapt an app from WVGA to qHD. The resolution is different, as well as the pixel density that may also vary, but for me it doesn't make sense to create a responsive UI to go from 960x540px(qHD) to 800x480px(WVGA) taking into account the screen size in inches is will be really similar from one device to the other. What we should present to the user in terms of layout from one smartphone to the other should be the same, but adapting the components in size to fill the device. The responsiveness of the layout should be intelligent enough to know the physical screen size, not the optical resolution.

As Ismael pointed out, in the case we have different aspect ratios, and taking into account we are using web technologies, we can scale component's width, and fill the difference in height by displaying more elements. Like it's done in the web.

I agree we should think in a responsive approach and that we will have a huge amount of different devices in the future, from smartphones to tablets or TVs, but i'm not sure that's in the scope right now or the purpose of this thread or that it's possible to switch to a responsive approach without investing quite an important amount of time.

Comment 92

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Hello Ismael:

Since there are per-application patches remaining, may we work together to speed-up the landing process?
In specific, I can take some of the apps from your repository, rebase & clean them up, then put them to review.  Let me know how do you think about it.  Thank you!

Comment 93

[Ismael Gonzalez [:basiclines]]

(In reply to Ben Francis [:benfrancis] from comment #90)
> Comment on attachment 721635 [details]
> Browser pr
> 
> I've noticed a small regression with header-divider.png not being displayed
> correctly in the tab tray at 320x480 resolution.
> 
> Apart from that I can't spot any obvious regressions caused by this patch so
> *with that fixed* I can give r+me.
> 
> I'm a little concerned how future-proof the general approach is here, and
> using rem and % to size background images is a little concerning too, but I
> don't want to hold up this whole bug just from the browser patch.
> 
> As a general comment, we will have a *lot* more than 3 different resolutions
> to optimise for!

I will fix that background and merge the PR, thx Ben!


(In reply to KM Lee [:rexboy] from comment #92)
> Hello Ismael:
> 
> Since there are per-application patches remaining, may we work together to
> speed-up the landing process?
> In specific, I can take some of the apps from your repository, rebase &
> clean them up, then put them to review.  Let me know how do you think about
> it.  Thank you!

Sure, i'm happy to hear that!

Comment 94

[Ismael Gonzalez [:basiclines]]

Browser PR merged: https://github.com/mozilla-b2g/gaia/commit/6136ef06f37ba9785b5ac461e81d9db4f059055f

Comment 95

[Ismael Gonzalez [:basiclines]]

Shared PR merged: https://github.com/mozilla-b2g/gaia/commit/940124b97032a223a3f618137f8f86b86acc0738

Comment 96

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Ismael from comment #89)
> Physical css units (mozmm, pt, etc..) are for physical elements (print
> media) we should not use ever that units. Just px or rem units.

pt is just 4/3 of a px. mozmm is very different: assuming everything's implemented properly in Gecko (which it isn't, currently, for FFOS/Gonk), and no zoom, a mozmm is an actual physical millimetre on the screen. So it's a good unit to use for touch targets. Unfortunately it's not standardized, but if app developers think they'd like to have it, that would help me get it standardized :-).

Comment 97

[Sergi]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #96)
> (In reply to Ismael from comment #89)
> > Physical css units (mozmm, pt, etc..) are for physical elements (print
> > media) we should not use ever that units. Just px or rem units.
> 
> pt is just 4/3 of a px. mozmm is very different: assuming everything's
> implemented properly in Gecko (which it isn't, currently, for FFOS/Gonk),
> and no zoom, a mozmm is an actual physical millimetre on the screen. So it's
> a good unit to use for touch targets. Unfortunately it's not standardized,
> but if app developers think they'd like to have it, that would help me get
> it standardized :-).

Sounds really interesting, but people have been complaining a lot about using browser specific functions. I think we should go just the opposite, trying to make the code as much standard and cross-browser as possible

Comment 98

[Patryk Adamczyk [:patryk] UX]

Hey Rex, can you please let me know what feature spec are you working from?

Comment 99

[Ismael Gonzalez [:basiclines]]

Attachment: System-1 PR [statusbar, quick settings, utility tray, notifications, sound manager]

Not sure who is the owner of this system components, if you know him, please add them for reviewers.

Comment 100

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 722732 [details]
System-1 PR [statusbar, quick settings, utility tray, notifications, sound manager]

Some small nits.

Alive, would you review the Volume indicator part?

Comment 101

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Sergi from comment #97)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #96)
> > pt is just 4/3 of a px. mozmm is very different: assuming everything's
> > implemented properly in Gecko (which it isn't, currently, for FFOS/Gonk),
> > and no zoom, a mozmm is an actual physical millimetre on the screen. So it's
> > a good unit to use for touch targets. Unfortunately it's not standardized,
> > but if app developers think they'd like to have it, that would help me get
> > it standardized :-).
> 
> Sounds really interesting, but people have been complaining a lot about
> using browser specific functions. I think we should go just the opposite,
> trying to make the code as much standard and cross-browser as possible

We should use standardized cross-browser features whenever they meet our needs, but sometimes we need features which aren't covered by any existing standard. In that case we create something new and try to get it standardized. In this case I've already discussed mozmm with the CSS WG and the main obstacle to its standardization right now is that no-one's saying they need it.

Comment 102

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 722732 [details]
System-1 PR [statusbar, quick settings, utility tray, notifications, sound manager]

r- for sound_manager.css
I don't understand why the patch ignores all rules about audio channels.
It breaks the voice channel volume overlay, please fix.

Comment 103

[Ismael Gonzalez [:basiclines]]

Comment on attachment 722732 [details]
System-1 PR [statusbar, quick settings, utility tray, notifications, sound manager]

Fixed telephony data-channel

Comment 104

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 722732 [details]
System-1 PR [statusbar, quick settings, utility tray, notifications, sound manager]

r+ 'for now':

I am going to land https://github.com/mozilla-b2g/gaia/pull/8564  It's a tef+ bug. The main idea is increase the count of segments of telephony 5->6, bt_sco 15->16

So plz make sure you would resolve the conflict then and test your patch again when you are going to merge. I could r+ your patch 'for now'.

Comment 105

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Patryk Adamczyk [:patryk] UX from comment #98)
> Hey Rex, can you please let me know what feature spec are you working from?

Hi Patryk:
Well, let me try to describe it briefly here.  I am now helping Ismael landing his changeset which fixes layout of Gaia applications on high-resolution devices.  As far as I know, although we are arguing about underlying solution, most of the layouts are kept similar (on 320x480 device nothing changes), with font and picture enlarged against resolution of different devices.  In the changeset there are double-sized picture assets scaled-down to adapt on high resolution devices.

Comment 106

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Settings PR

Tidied up from Ismael's repo.

Comment 107

[Ismael Gonzalez [:basiclines]]

Comment on attachment 724373 [details]
Settings PR

Everything looks fine, but there are some old changesets that we must review with a Settings app owner. 
The main point in this PR is removing -moz-image-rect and using background-positions with pseudo-elements.

Thx :rexboy

Comment 108

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 724373 [details]
Settings PR

Evelyn, or any of the Settings app owner can review this.

Comment 109

[Ismael Gonzalez [:basiclines]]

System-1 PR merged: https://github.com/mozilla-b2g/gaia/commit/daf3928e0ca82a65c79a2c5ba6c2ce6226075ffc

Comment 110

[Ben Francis [:benfrancis]]

I've noticed that all the UI elements look oversized in B2G Desktop recently, could it be anything to do with the patches in this bug?

Comment 111

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Ben Francis [:benfrancis] from comment #110)
> I've noticed that all the UI elements look oversized in B2G Desktop
> recently, could it be anything to do with the patches in this bug?

https://bugzilla.mozilla.org/show_bug.cgi?id=845829

Comment 112

[Ismael Gonzalez [:basiclines]]

Attachment: System-2 PR

Comment 113

[José Antonio Olivera Ortega [:jaoo]]

After landing commit eaea6311e808ac36cfaea2a74d6a789c68f0863f in gaia master branch the call forwarding icon is no longer shown.

Comment 114

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 725346 [details]
System-2 PR

Looks good! Thanks.

Comment 115

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Did the SMS pr breaks the build and cause bug 851827?

Comment 116

[Maria Angeles Oteo (:oteo)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (PTO Mar 22 - Apr 7) from comment #115)
> Did the SMS pr breaks the build and cause bug 851827?

I don't know but Ismael can not check it as he's on Holidays next two weeks, perhaps Rexboy can have a look at it.

Comment 117

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Gallery PR

Comment 118

[David Flanagan [:djf]]

Comment on attachment 726139 [details]
Gallery PR

Not giving an r- here, but clearing the review flag because I'm not sure I can actually review it.  I don't have a device with a different screen size, which makes it hard to test the proposed changes. Any suggestions on how to try this out?

I've only taken a quick glance at the patch, but it looks mostly like dividing pixels by 10 and converting 'px' to 'rem'.  That seems mostly harmless.

I'm not certain about converting the dimensions of image thumbnails from 106px to 10.6rem, however.

On 320x480 devices, the 106px is (320px minus 2px for margins)/3. It divides evenly. On a 480x800 device, the same math does not divide evenly. Does this mean that the thumbnail imags won't be on pixel boundaries? How will that affect their appearance? For the gallery app, I suspect we want everything to be as crisp as we can make it, so I suspect that using resolution-specific media queries may be necessary here, rather than just converting pixels to rems.  But I confess that I don't really understand how layout works when using floating-point numbers.  Or, if we don't use a media query, maybe we should at least use calc() to compute the thumbnail size rather than using rems.  (I think we already have a media query for portait vs landscape mode on the thumbnails.)

Comment 119

[KM Lee [:rexboy] (inactivate after Sep. 22)]

:oteo (In reply to Maria Angeles Oteo:oteo from comment #116)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (PTO Mar 22 - Apr 7)
> from comment #115)
> > Did the SMS pr breaks the build and cause bug 851827?
> 
> I don't know but Ismael can not check it as he's on Holidays next two weeks,
> perhaps Rexboy can have a look at it.
OK I will take a look.

Comment 120

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to David Flanagan [:djf] from comment #118)
> Comment on attachment 726139 [details]
> Gallery PR
> 
> Not giving an r- here, but clearing the review flag because I'm not sure I
> can actually review it.  I don't have a device with a different screen size,
> which makes it hard to test the proposed changes. Any suggestions on how to
> try this out?
Hi David, thanks for reviewing the patch. Using desktop build may be a way if it is convenient for you. But the media query inside shared/responsive.css should be changed before trying because the queries are just for phone now.

For the .thumbnail, It's true that nesting rem inside a media query makes its behavior somewhat unclear.  Since the media query is reserved here in css, I think we don't need to stick everything on rem here.  I'd try to use px for .thumbnail margin and width then, so that we can solve the floating-point width problem.

Comment 121

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: System-2 PR

Ismael is away so I open another PR to merge for him, with addressed comments by Tim corrected.

Carrying r=tim.

Comment 122

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 726139 [details]
Gallery PR

Updated the code. There are also some update for determining thumbnail size on different resolution devices.

For now I am now testing this on both desktop build and S2.  When testing on b2g-desktop, I would add "html { font-size: 105%!important; }" in shared/responsive.css in advance with b2g client size set to 960x540.  Not sure if there are better way for trying without device.  Most of them are just css unit and asset changes though.

Comment 123

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Clock PR

Comment 124

[Evelyn Hung]

Comment on attachment 724373 [details]
Settings PR

r=me, thanks!

Comment 125

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 724373 [details]
Settings PR

https://github.com/mozilla-b2g/gaia/commit/7202403530d43a3fbac223f3beef5899faa46665

Settings PR Merged. Thanks for your help, Evelyn!

Comment 126

[[e-mail], [e-mail]]

Comment on attachment 726623 [details]
Clock PR

Since Rex already fixed the nit from github, it works for me. r+ Nice job!

Comment 127

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Calendar PR

James may you review this patch? thanks a lot!

Comment 128

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 726623 [details]
Clock PR

https://github.com/mozilla-b2g/gaia/commit/4072db7b74e4d9677314a73c51ffa849170e8d7c
Clock PR merged. Thank you for reviewing Ian!

Comment 129

[Anthony Ricaud (:rik)]

I believe https://github.com/mozilla-b2g/gaia/commit/0f3a6c729085e322502c05b5dc932bf27ea0139c or https://github.com/mozilla-b2g/gaia/commit/8f5ebdd30c0a41c2fc9e78a3336c7a148d36b35a caused a 100+ ms performance regression on startup of the Settings app. It's now above 1s.

Comment 130

[Anthony Ricaud (:rik)]

See https://datazilla.mozilla.org/b2g/?branch=master&range=7&test=startup_time&app=settings&app_list=settings&gaia_rev=7202403530d43a3f&gecko_rev=ce5c0b8f9aebdc65 for our metrics.

Comment 131

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: FM PR

FM PR

Comment 132

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: ftu PR

Comment 133

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Anthony Ricaud (:rik) from comment #129)
> I believe
> https://github.com/mozilla-b2g/gaia/commit/
> 0f3a6c729085e322502c05b5dc932bf27ea0139c or
> https://github.com/mozilla-b2g/gaia/commit/
> 8f5ebdd30c0a41c2fc9e78a3336c7a148d36b35a caused a 100+ ms performance
> regression on startup of the Settings app. It's now above 1s.
I may found the cause but let's fire another bug for this: Bug 853723

Comment 134

[Pin Zhang [:pzhang] (inactive)]

(In reply to KM Lee [:rexboy] from comment #131)
> Created attachment 727545 [details]
> FM PR
> 
> FM PR

@rexboy, I tried your branch (fd-fm) both on the Firefox Nightly with responsive design view (480x800) and the B2G desktop with --screen=480x800, I didn't see any responsive views, is this the right way to check the responsive designs? Please tell me if there is anything that I made a mistake.

Comment 135

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Pin Zhang [:pzhang] from comment #134)
> @rexboy, I tried your branch (fd-fm) both on the Firefox Nightly with
> responsive design view (480x800) and the B2G desktop with --screen=480x800,
> I didn't see any responsive views, is this the right way to check the
> responsive designs? Please tell me if there is anything that I made a
> mistake.
Pzhang:
Oh yes. See comment 122. When using desktop build, the query inspects resolution of our whole monitor, causing it doesn't work as expected. So we need to change a bit of code on resolution.css. 
Sorry for the confusion. Seems it's better to put a patch for checking purpose. I'll put a patch here.

Comment 136

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Apply before testing on desktop build (for --screen=480x800)

If you are checking PR on desktop build, apply it with --screen=480x800 so that it sets font-size correctly. As media query matches resolution of whole monitor, we need it to imitate behavior on WVGA phones.

Comment 137

[Pin Zhang [:pzhang] (inactive)]

(In reply to KM Lee [:rexboy] from comment #136)
> Created attachment 728080 [details] [diff] [review]
> Apply before testing on desktop build (for --screen=480x800)
> 
> If you are checking PR on desktop build, apply it with --screen=480x800 so
> that it sets font-size correctly. As media query matches resolution of whole
> monitor, we need it to imitate behavior on WVGA phones.

@rexboy, I applied the patch, yes, it works, but it just enlarged all the images, didn't use the images with @2x that I expected, then what are the @2x images used for?

Comment 138

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Pzhang,
Try $ HIDPI=1 make
The assets are not included in packages unless specifying by HIDPI.
Sorry for the confusion again. The usage has been included in README.md at root of repository so you can take a look.

Comment 139

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Video PR

Comment 140

[Dale Harvey (:daleharvey)]

Comment on attachment 728164 [details]
Video PR

I am happy with this, good job, would be nice to get it merged asap as there is other video work ongoing.

Comment 141

[Dale Harvey (:daleharvey)]

Merged video PR in: https://github.com/mozilla-b2g/gaia/commit/0d589c6991bb4335d69957df3b68596d8aa38964

Comment 142

[Pin Zhang [:pzhang] (inactive)]

(In reply to KM Lee [:rexboy] from comment #138)
> Pzhang,
> Try $ HIDPI=1 make
> The assets are not included in packages unless specifying by HIDPI.
> Sorry for the confusion again. The usage has been included in README.md at
> root of repository so you can take a look.

Thanks for your instruction, then the only question that I have is about the decimal precision, please check my comments on the PR page.

Comment 143

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Music PR

Comment 144

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Pin Zhang [:pzhang] from comment #142)
> Thanks for your instruction, then the only question that I have is about the
> decimal precision, please check my comments on the PR page.
Updated. May you review it again? Thanks a lot!

Comment 145

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Keyboard PR

Comment 146

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 727545 [details]
FM PR

r=me

Comment 147

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Email PR

Comment 148

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Pin Zhang [:pzhang] from comment #146)
> Comment on attachment 727545 [details]
> FM PR
> 
> r=me

https://github.com/mozilla-b2g/gaia/commit/2ca952bcca39eda4fa6693ee53cf8c7ec8a3bdcc
Merged. Thanks you Pin Zhang!

Comment 149

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Dialer PR

Comment 150

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 730535 [details]
Keyboard PR

r=me.

Rex,

Thanks for your work.

Comment 151

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Bluetooth PR

Comment 152

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Rudy Lu [:rudyl] from comment #150)
> Comment on attachment 730535 [details]
> Keyboard PR
> r=me.
> Rex,
> Thanks for your work.
Keyboard PR merged:
https://github.com/mozilla-b2g/gaia/commit/bf0089f663686d03843dbe2e3bec31ea5ed54426
Thank you Rudy!

Comment 153

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Costcontrol PR

Comment 154

[[e-mail], [e-mail]]

(In reply to KM Lee [:rexboy] from comment #151)
> Created attachment 730617 [details]
> Bluetooth PR

The pr is relative with Dialer app.

Comment 155

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Camera PR

Well.. not sure if this can be tested on desktop build. I can take some screenshot from my S2 if needed. Thanks.

Comment 156

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Bluetooth PR

I'm so sorry Ian. It should redirect to the correct place now.

Comment 157

[[e-mail], [e-mail]]

Comment on attachment 731107 [details]
Bluetooth PR

It works for me. r+

Comment 158

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to Ian Liu [:ianliu] from comment #157)
> Comment on attachment 731107 [details]
> Bluetooth PR
> 
> It works for me. r+
Merged:  https://github.com/mozilla-b2g/gaia/commit/6089f2f0a5698e77494881b58b2721d781c887fc
Thank you Ian!

Comment 159

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Wallpaper PR

This may not testable on desktop build, so I can record a video here if needed.

Comment 160

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Contact PR

This is the one with some javascript changes. Most of them are for adjusting image sizes.

Comment 161

[Fernando Campo (:fcampo)]

(In reply to KM Lee [:rexboy] (Away 4/4~4/14) from comment #132)
> Created attachment 727596 [details]
> ftu PR
There was some images on FTU that had been change recently. Not sure what would be better, to approve this and open a follow bug, or fix them in here before merging

I made some comments on the specific images on github.

Apart from that, looks fine to me :)

Comment 162

[Dietrich Ayala (:dietrich)]

Adding dev-doc-needed keyword. HIDPI should be in the official docs at least as a flag you can set in .userconfig:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Customization_with_the_.userconfig_file

And probably also in the Gaia docs as well.

Comment 163

[David Flanagan [:djf]]

Comment on attachment 726139 [details]
Gallery PR

I don't know much about Window.matchMedia(), but it seems like a pretty heavyweight API to use. How about just checking window.innerWidth and window.innerHeight. I've included some proposed code in my github comments.

Otherwise that than, the patch looks good.  I haven't tried it, since I don't have a high-resolution device and have forgotten how to use b2g-desktop

Comment 164

[David Flanagan [:djf]]

Comment on attachment 731824 [details]
Wallpaper PR

This PR adds new larger wallpaper images, but doesn't have any code that uses them. It it still hardcodes the 320x480 images size, I think.

Also, the current code resizes wallpapers in a way that will distort the image when used on a screen with a different aspect ratio. I think we might want to write code that crops the edges so that the aspect ratios match and the image is not distorted.

Both of these things are more JavaScript intensive than most of the other patches in this bug, so if you want to spin this off as a separate bug, I'd be happy to give r+ for just the CSS changes here.

Comment 165

[David Flanagan [:djf]]

Comment on attachment 728164 [details]
Video PR

Removing the r? flag on me here, since Dale got to this review well before I could.

Comment 166

[Salvador de la Puente González [:salva]]

Attachment: The warning icon is incorrect cropped

It seems a problem with the icon scale.

Comment 167

[Salvador de la Puente González [:salva]]

Attachment: The warning icon is too large

Comment 168

[Salvador de la Puente González [:salva]]

Attachment: The updating spinner is broken

Furthermore, in the widget the spinner rotation is eccentric.

Comment 169

[Salvador de la Puente González [:salva]]

Comment on attachment 731051 [details]
Costcontrol PR

Due to several regressions in warning icons and spinners when in default screen mode.

Comment 170

[Ben Francis [:benfrancis]]

Nominating for tef+ because this patch will fix bug 855021 which is tef+

Comment 171

[Ben Francis [:benfrancis]]

Sigh, that's not going to work because the needed commit (e0d4130a06481c881cbac587bf856f3730b4e505) is dependent on lots of other commits in this bug.

Comment 172

[James Lal [:lightsofapollo]]

Comment on attachment 727154 [details]
Calendar PR

><html>
>  <head>
>  <meta http-equiv="Refresh" content="2;
>    url=https://github.com/mozilla-b2g/gaia/pull/8980">
>  </head>
>  <body>
>    Redirect to pull request 8980>  </body>
></html>
>

Comment 173

[James Lal [:lightsofapollo]]

Calendar version rebased and has my R+ (rexboy still gets commit credit)...

Comment 174

[Ismael Gonzalez [:basiclines]]

Attachment: Costcontrol PR

Updated PR addressing Salva comments

Comment 175

[Ismael Gonzalez [:basiclines]]

Attachment: Wallpaper PR

Comment 176

[David Flanagan [:djf]]

Comment on attachment 733304 [details]
Wallpaper PR

I'm giving r+ on this with these caveats:

1) I haven't tested the code

2) Are you sure you want to remove the pick.js changes?  As it stands, the code will only work if the screen size matches the image size exactly. So if you are trying to support more than one large screen size, I don't think this will work.

3) Thanks for the explanation of how the @2x images work.  Now that I understand that, I'd prefer a patch that changed resources/320x480/ to resources/images/ so that we are not putting images of varying sizes in a 320x480 directory. But you can defer that to a follow-up patch if you prefer to land this one as is.

Comment 177

[Ismael Gonzalez [:basiclines]]

Hey David, about the pick.js changes: when the background is placed inside the system app it uses `background-size: cover` to deal with these aspect ratios differences. I mean if you use an image with different aspect ratio that the device one that CSS rule will fill all the screen with the image without deforming it.

If this is not working in this way, i will like to have a quick chat with you to be sure that what we are landing is safe enough.

Comment 178

[Dale Harvey (:daleharvey)]

Comment on attachment 731106 [details]
Camera PR

Merged in: https://github.com/mozilla-b2g/gaia/commit/a268ae3521516a8992fcaf17402ef2cac5ea2bd7

Comment 179

[Ismael Gonzalez [:basiclines]]

Attachment: FTU PR

Comment 180

[Fernando Campo (:fcampo)]

Comment on attachment 733830 [details]
FTU PR

For the FTU part:
- The original for image https://github.com/basiclines/gaia/blob/1040644ae18cfbb0286436d914769269d0d9ee79/apps/communications/ftu/css/images/bg_bubbles%402x.png has recently change (bug 823472) so we need a 2x version for the new one.

- Image https://github.com/basiclines/gaia/blob/1040644ae18cfbb0286436d914769269d0d9ee79/apps/communications/ftu/css/images/wifi_sprite.png is no longer used on FTU since bug 811149 landed, so https://github.com/basiclines/gaia/blob/1040644ae18cfbb0286436d914769269d0d9ee79/apps/communications/ftu/css/images/wifi_sprite%402x.png is no needed either.

- Image https://github.com/basiclines/gaia/blob/1040644ae18cfbb0286436d914769269d0d9ee79/apps/communications/ftu/css/images/tutorial/3%402x.png doesn't correspond the current tutorial (https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/ftu/css/images/tutorial/3.png) since bug 830380 landed.

- Image https://github.com/basiclines/gaia/blob/1040644ae18cfbb0286436d914769269d0d9ee79/apps/communications/ftu/css/images/icon_tour.png would create a regression, as it was changed on bug 822585. Then image https://github.com/basiclines/gaia/blob/1040644ae18cfbb0286436d914769269d0d9ee79/apps/communications/ftu/css/images/icon_tour%402x.png is not longer needed.

The rest of the changes seems ok to me :)
So please ask me for review once this changes are done

Thanks Isma!

Comment 181

[Ismael Gonzalez [:basiclines]]

Merged in master Wallpaper PR: https://github.com/mozilla-b2g/gaia/commit/15fcfa59b36023cac4b044af1e454f1efede7df1

Comment 182

[Fernando Campo (:fcampo)]

Comment on attachment 733830 [details]
FTU PR

Everything looks good to me :)

Thanks for the effort Isma!

Comment 183

[Ismael Gonzalez [:basiclines]]

FTU PR merged: https://github.com/mozilla-b2g/gaia/commit/c0ec827c4137903bac7b1841939dcefce32a5f36

Comment 184

[Ismael Gonzalez [:basiclines]]

Attachment: Calendar PR

Comment 185

[Salvador de la Puente González [:salva]]

Comment on attachment 733232 [details]
Costcontrol PR

Eccentric rotations seems to be another problem. Not blocking on this.

You have my blessings. The applications looks nice.

Thank you very much. Great job guys!

Comment 186

[Ismael Gonzalez [:basiclines]]

Costcontrol PR merged: https://github.com/mozilla-b2g/gaia/commit/e0c0f9d8fc38ef2b36d16f272979cd98bf9d50a3

Comment 187

[James Lal [:lightsofapollo]]

Comment on attachment 735093 [details]
Calendar PR

in master https://github.com/mozilla-b2g/gaia/commit/f98218f66fae3d6cfd635a7469b700824104a8f8

Comment 188

[Ismael Gonzalez [:basiclines]]

Attachment: Contacts PR

Comment 189

[Ismael Gonzalez [:basiclines]]

Attachment: Dialer PR

Comment 190

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 736308 [details]
Dialer PR

Looks perfect to me... and it even works :p -> r+

Thank you very much, Isma.

Comment 191

[Ismael Gonzalez [:basiclines]]

Dialer PR merged in master: https://github.com/mozilla-b2g/gaia/commit/2b1843511c64d7dda09fa66fc5ff1bae11d90f5f

Comment 192

[alberto.pastor]

Comment on attachment 735795 [details]
Contacts PR

I'm ok with the non-FB related stuff. Let's leave the FB review to Jose Manuel.

Comment 193

[Ismael Gonzalez [:basiclines]]

Attachment: Email PR

Comment 194

[Ismael Gonzalez [:basiclines]]

Contacts PR merged in master: https://github.com/mozilla-b2g/gaia/commit/574ea2af224bb2a389f33de84f848227cbc3cc5c

Comment 195

[Dominic Kuo [:dkuo]]

Comment on attachment 730517 [details]
Music PR

Rex,

I will cancel this one first because we have a new set of the default album arts in music app, and after you updated this patch, please re-assign to me
for reviewing, thanks.

Comment 196

[Ismael Gonzalez [:basiclines]]

Attachment: Music PR

Comment 197

[Dominic Kuo [:dkuo]]

Comment on attachment 738947 [details]
Music PR

Ismael,

When I am reviewing the music pr (attachment 738947 [details]), I keep finding many minor issues. Althought they are not major issue, but I think it's better to double check them before landing it. So I feel I should cancel the review request first, after all the issue are addressed, please re-assgin to me, thanks.

Comment 198

[Eric Chou [:ericchou] [:echou]]

(In reply to KM Lee [:rexboy] (Away 4/4~4/14) from comment #125)
> Comment on attachment 724373 [details]
> Settings PR
> 
> https://github.com/mozilla-b2g/gaia/commit/
> 7202403530d43a3fbac223f3beef5899faa46665
> 
> Settings PR Merged. Thanks for your help, Evelyn!

Hi Rex,

Bluetooth "Phone" icon couldn't be shown with this patch applied. I checked the commit and found that you seemed to miss to add ":before" at the tail of "bluetooth-type-pda" and "bluetooth-type-phone". Would you mind taking a look?

Comment 199

[Ismael Gonzalez [:basiclines]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #198)
> (In reply to KM Lee [:rexboy] (Away 4/4~4/14) from comment #125)
> > Comment on attachment 724373 [details]
> > Settings PR
> > 
> > https://github.com/mozilla-b2g/gaia/commit/
> > 7202403530d43a3fbac223f3beef5899faa46665
> > 
> > Settings PR Merged. Thanks for your help, Evelyn!
> 
> Hi Rex,
> 
> Bluetooth "Phone" icon couldn't be shown with this patch applied. I checked
> the commit and found that you seemed to miss to add ":before" at the tail of
> "bluetooth-type-pda" and "bluetooth-type-phone". Would you mind taking a
> look?

Hi Eric, i'll take care bout this. As soon as we get landed all the applications(we only miss 2) i'll make a PR with some polish with these kind of issues

Comment 200

[Eric Chou [:ericchou] [:echou]]

(In reply to Ismael from comment #199)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #198)
> > (In reply to KM Lee [:rexboy] (Away 4/4~4/14) from comment #125)
> > > Comment on attachment 724373 [details]
> > > Settings PR
> > > 
> > > https://github.com/mozilla-b2g/gaia/commit/
> > > 7202403530d43a3fbac223f3beef5899faa46665
> > > 
> > > Settings PR Merged. Thanks for your help, Evelyn!
> > 
> > Hi Rex,
> > 
> > Bluetooth "Phone" icon couldn't be shown with this patch applied. I checked
> > the commit and found that you seemed to miss to add ":before" at the tail of
> > "bluetooth-type-pda" and "bluetooth-type-phone". Would you mind taking a
> > look?
> 
> Hi Eric, i'll take care bout this. As soon as we get landed all the
> applications(we only miss 2) i'll make a PR with some polish with these kind
> of issues

Sure, no problem. Thank you, Ismael.

Comment 201

[Dominic Kuo [:dkuo]]

Comment on attachment 738037 [details]
Email PR

Ismael/Rex,

I have found several issues on this patch, please see the comments on github, they are quite noticeable so let's fix all of them before I give a r+ on this patch. After the issues are addressed, feel free to re-assign it to me, thanks.

Comment 202

[Dominic Kuo [:dkuo]]

Comment on attachment 738037 [details]
Email PR

Ismael,

I think the patch for email looks good to me now, before landing it, please fix the rest issues I have mentioned on github. I was not able to test all the resolutions but only make sure 320 x 480 was not broken, it will be nice if you can check the major sizes before merging it, thanks.

Comment 203

[Ismael Gonzalez [:basiclines]]

Email PR merged in master: https://github.com/mozilla-b2g/gaia/commit/31e94a9431d6ff7cfbc3ebaae5b33c126773742d

Comment 204

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 726139 [details]
Gallery PR

Let's put Gallery app back into review.
David, would you mind reviewing it again? I've made some changes based on the comments on Github. Thanks a lot!

Comment 205

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Eric:
Sorry for didn't notice that mistake. If you're in urgent I can send a pull request for that, or you can wait for the polish PR. Thanks a lot!

Comment 206

[Dominic Kuo [:dkuo]]

Comment on attachment 738947 [details]
Music PR

Ismael,

With Rex's new patch we finally solved the issues on the tiles! Let's fix the rest issues I have commented on github and this patch should be good to go, thanks!

Comment 207

[Ismael Gonzalez [:basiclines]]

Music PR merged: https://github.com/mozilla-b2g/gaia/commit/72addc6429e44190e8bf1ffc9c8ad495ca6c492a

Comment 208

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 726139 [details]
Gallery PR

I made some update on the Gallery app PR because Ismael told me there are some display issue on landscape mode for his device. The newly added code is in a single commit and will be squashed once the review is over.

Comment 209

[David Flanagan [:djf]]

Comment on attachment 726139 [details]
Gallery PR

Overall this looks good. See my comments on github though. Also, I don't have a hidpi device to test on, so I'm assuming that you have tested this PR carefully.

Of my various comments on github, the only one I'm particularly concerned about is the mismatch between the width of the #thumbnails container and the calculated widths of the .thumbnail.  One or the other of those needs to change so that the width of 3 thumbnails (in portrait) matches the width of the container.

Also, why do you calculate padding-bottom for the .thumbnail class instead of height? I assume it works, I just don't know why it is necessary.

Sorry it took me so long to do this review!

Comment 210

[Dominic Kuo [:dkuo]]

(In reply to David Flanagan [:djf] from comment #209)

> Also, why do you calculate padding-bottom for the .thumbnail class instead
> of height? I assume it works, I just don't know why it is necessary.

David,

I think Rex calculated padding-bottom instead of height for the .thumbnail because he wants to keep the thumbnails in square, since we are not going to use pixel and needs to calculate the width and height with css, we can't just write "width: percentage; height: percentage" cause width and height are with different respects. So this tricky hack is used on the gallery patch I think, and the padding-bottom is respecting to the width so that's why it works.(height must be set to 0)

The technique is also used in music app(https://github.com/mozilla-b2g/gaia/blob/master/apps/music/style/music.css#L427) becasue Rex, Ismael and I have first encountered the problem of keeping elements in square, and this tricky solution is our answer with just css modified.

Comment 211

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 726139 [details]
Gallery PR

https://github.com/mozilla-b2g/gaia/commit/767f8e1a41badd8906d0ef490e8e2302403e9a2a
Merged with comments addressed by David. Thank you for all these comments!
I end up with :nth-child to remove the separator of rightmost thumbnails. It works great on Unagi, WVGA and qHD devices, the edge is kept in sharp 1px without problem.

Comment 212

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Finally we've landed all the required apps needed for this bug. Thanks all who contributed in it! Ismael, would you like to open the polish PR here or should we open it in another bug? Either way is ok to me :)

Comment 213

[Ismael Gonzalez [:basiclines]]

Sure Rex, i'll try to open it today. And i will also attach it in this bug.
Thx for your work also!

Comment 214

[Salvador de la Puente González [:salva]]

Comment on attachment 733232 [details]
Costcontrol PR

I'm nominating this patch because its risk is low and blocks bug 828155.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: medium (in HDPI screens, visualization is improved)
Testing completed: yes
Risk to taking this patch (and alternatives if risky): low (major part of modifications are high resolution icons. Text modifications only affect CSS and visualization has been tested.)
String or UUID changes made by this patch: none

Comment 215

[Douglas Campos]

is there an easy way to test those changes? RTFM links more than welcome.

Comment 216

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 733232 [details]
Costcontrol PR

This does not look like a low risk amount of change to take for uplift to v1.1, afaik hidpi support is OOS for v1.1 and we're fine with having this ride the train to v1.2, getting a lot of bake time on nightlies in the meantime.  We'll make sure geeksphone are aware this work exists in case they want to take it into their build for the Peak.

Comment 217

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to KM Lee [:rexboy] from comment #212)
> Finally we've landed all the required apps needed for this bug. Thanks all
> who contributed in it! Ismael, would you like to open the polish PR here or
> should we open it in another bug? Either way is ok to me :)

Please submit new bugs for future polish. Thanks!

Comment 218

[Ran Ben Aharon [:ranbena] EverythingMe]

Attachment: Evme PR

Comment 219

[Ismael Gonzalez [:basiclines]]

Comment on attachment 747463 [details]
Evme PR

Everything looks fine, just remove a couple of `-moz` prefixes.
Great work Ran!

Comment 220

[Cristian Rodriguez (:crdlc)]

Comment on attachment 747463 [details]
Evme PR

My review is + but, please, before merging, go to Github where you can read some comments. Thanks for the great work because it works fine in my device

Comment 221

[Ismael Gonzalez [:basiclines]]

EV.me PR merged in master: https://github.com/mozilla-b2g/gaia/commit/3ee62a4fb004fc5f8f81b829845a2d46539efcea

Comment 222

[gentoo.stefano]

On geeksphone peak using branch master, keyboard doesn't fit in the screen: last key go on the row below. This doesn't happen with v1-train brach

Comment 223

[:gerard-majax]

(In reply to gentoo.stefano from comment #222)
> On geeksphone peak using branch master, keyboard doesn't fit in the screen:
> last key go on the row below. This doesn't happen with v1-train brach

I've seen a bug fixing this today.