881145 - hdpi/xhdpi resolution assets and layout for Everything.me panel in Homescreen app

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Everything.me, defect)

Description

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

This bug represents fixes needed in Everything.me panel in Homescreen app in order to deliver bug 881126:

-- Have visual supply us with @1.5x and the correct @2x images.
-- On v1.1hd, modify CSS/JS so that apps will display @1.5x/@2x images correctly.
-- On master, drop the @1.5x and @2x images and make sure there is no
more to tweak besides changes made on bug 830644.

As HIDPI support never landed on v1-train, I expect the patch for both
branches on this bug will be different. The CSS/JS patch on v1.1hd will be based on the patch on master, but I won't expect it to be as simple as a |git cherry-pick|; so this is
where the engineering work lays.

The patch on top of v1-train should land on v1.1hd.

Comment 1

[Wayne Chang [:wchang]]

Setting hd+ for 1.1hd specific requirement

Comment 2

[Ran Ben Aharon [:ranbena] EverythingMe]

-- Have visual supply us with @1.5x and the correct @2x images.
Should these images be supplied in the ticket or in the patch?
What do you mean correct @2x images. Are the current ones problematic?

-- On v1.1hd, modify CSS/JS so that apps will display @1.5x/@2x images correctly.
The current code has background-size set for all images. Does this not suffice?

-- On master, drop the @1.5x and @2x images and make sure there is no
more to tweak besides changes made on bug 830644.
We should remove the existing @2x images from the master branch?

Comment 3

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

(In reply to Ran Ben Aharon (Everything.me) from comment #2)
> > -- Have visual supply us with @1.5x and the correct @2x images.
> Should these images be supplied in the ticket or in the patch?

Patch is better than zip if you would like to work on the v1.1hd CSS uplift :)

> What do you mean correct @2x images. Are the current ones problematic?

Don't shoot the messenger :P According to our Visual Lead Patryk, correct @2x images should have enough graphics detail, not simply just a upscale export of the victor image.

> > -- On v1.1hd, modify CSS/JS so that apps will display @1.5x/@2x images
> correctly.
> The current code has background-size set for all images. Does this not
> suffice?

That's perfectly sufficient on master branch ... but someone would have to uplift all that CSS to v1.1hd.

> > -- On master, drop the @1.5x and @2x images and make sure there is no
> > more to tweak besides changes made on bug 830644.
> We should remove the existing @2x images from the master branch?

No, the two set of images should co-exist in the codebase.

Comment 4

[Ran Ben Aharon [:ranbena] EverythingMe]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #3)
> Patch is better than zip if you would like to work on the v1.1hd CSS uplift :)

Apparently, our HiDPI commit didn't make it in to v1-train. https://github.com/mozilla-b2g/gaia/commit/22eb1a81d9107a300823e98b3b3e871ca6991320.
1. I'll cherry-pick it into v1.1.0hd.
2. I'll create the @1.5x images and Pull Request into master and v1.1.0hd.

Makes sense?
 
> Don't shoot the messenger :P According to our Visual Lead Patryk, correct @2x images should have enough graphics detail, not simply just a upscale export of the victor image.

Sorry :)
Our only images are 4 simple icons
https://raw.github.com/mozilla-b2g/gaia/master/apps/homescreen/everything.me/images/[e-mail]
https://raw.github.com/mozilla-b2g/gaia/master/apps/homescreen/everything.me/images/[e-mail]
https://raw.github.com/mozilla-b2g/gaia/master/apps/homescreen/everything.me/images/[e-mail]
https://raw.github.com/mozilla-b2g/gaia/master/apps/homescreen/everything.me/images/[e-mail]

I believe these require vector resizing only. No extra details are needed IMO. Patryk, do you approve?

Comment 5

[Stephany Wilkes]

Adding Eric and Peter because this one is blocking HD and needs to move. Can one of you check comment 4 and see if you can answer the question addressed to Patryk? Thanks!

Comment 6

[Patryk Adamczyk [:patryk] UX]

Attachment: FFOS Icons

Attached 2 icons, can we update the forms in the e.me app? The "+" and "x" in a circle look really off.

Comment 7

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

See comment 6.

Comment 8

[[e-mail]]

I'd try to provide a PR to facilitate the progress.

Comment 9

[[e-mail]]

Attachment: pull request redirect to github

This bug aims to achieve similar goal with bug 830644 on hd1.1.0 branch.
Since we need some specified step to try on WVGA device (partial can be tested
on Nightly), if you care about the changes and would like to do a full review,
you may set review flag after reviewing; otherwise, you may set feedback flag
and let me find an appropriate reviewer in Taipei to review and confirm on WVGA
device.
These 1.5x assets in patch are here temporarily for confirming we're making right
layout changes, and will be replaced later after visual team supplying newly made
1.5x assets.
The step for testing by HD is noted in Github comment. Thanks a lot!

Comment 10

[Ismael Gonzalez [:basiclines]]

Seems that we missed a lot of changes for the suff landed in master in order to sopport hd for E.me.
Look at what we merged in master for hidpi support:
https://github.com/mozilla-b2g/gaia/pull/9644

Also this new PR does not look good at all on my Peak device: 
http://cl.ly/image/370F2W161N1G

Hope it helps to get this done!

Comment 11

[[e-mail]]

thanks for info, parital merged the bug 830644.

I'd try to solve text overlap issue. Any hints are welcome.

Comment 12

[[e-mail]]

text overlap issue fixed.

Ismael, can you help to review if there's any glitch on your Peak? thanks!

Comment 13

[Ismael Gonzalez [:basiclines]]

Hi Fred, seems that is looking better right now, but there is still a couple of issues to fix:

1. There must be another row of apps at least (as there is available space)
This may help you https://github.com/EverythingMe/gaia/blob/22eb1a81d9107a300823e98b3b3e871ca6991320/apps/homescreen/everything.me/js/Brain.js#L14
2. The icons for app/categories looks blurry. We need to use the @1.5x versions, generate the canvas at 1.5x size and then scale down via css

Screenshoot
http://cl.ly/image/111z102f0Z0H

I think Ran could clarify us a little bit more about this issues

Thanks Fred!

Comment 14

[[e-mail]]

Hi Ismael,

1. I think the extra row is configured by customization, so I tried to test on master with `make DEBUG=1 GAIA_DEV_PIXELS_PER_PX=2` setting and 
 change `enumberOfAppsToLoad` value in config.js from 16 to 20. It does not show another row of apps. Do I miss something or is 'another row of apps' a todo feature?

2. The app/categories icon comes from query the e.me. Is there any different API available for @1.5x or @2x image?

Thanks!

Comment 15

[Ismael Gonzalez [:basiclines]]

Comment on attachment 770077 [details]
pull request redirect to github

I think the one that must approve this patch is Ran from E.me. I should only take a look in to the code and make a general review as i'm not the owner of this module.

Comment 16

[[e-mail]]

Attachment: PR for master

1.5x images from v1.1.0hd for Master branch

Comment 17

[[e-mail]]

Thank you Ismael, your suggestions help me a lot to patch E.me.

Comment 18

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

Comment on attachment 770077 [details]
pull request redirect to github

I checked on my HD device and it works well. The only concern for me is also about the canvas problem, just as the one happend in Usage app.
But I'm not so familiar with codes of Ev.me code, so we need to wait for Ran's review.

Thanks for working on this!

Comment 19

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

Comment on attachment 770077 [details]
pull request redirect to github

Evyatar, would you be able to help Ran here?

Comment 20

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

Comment on attachment 772562 [details]
PR for master

Evyatar, would you be able to help Ran here?

Comment 21

[Ran Ben Aharon [:ranbena] EverythingMe]

Guys, I've looked at the PR code. On the side I also did a full merge to the #830644 commit.

There is an issue with the fact that we use window.innerWidth to calculate devicePixelRatio. We do this because the Peak device returns incorrect values. Are we still to rely on this? Will the 1.5 devices return 320/1.5?

Comment 22

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

(In reply to Ran Ben Aharon (Everything.me) from comment #21)
> There is an issue with the fact that we use window.innerWidth to calculate
> devicePixelRatio. We do this because the Peak device returns incorrect
> values. Are we still to rely on this? Will the 1.5 devices return 320/1.5?

Expect the |window.devicePixelRatio| to return the correct value. The problem has already been solved on the trunk and v1.1hd branch.

Comment 23

[Ran Ben Aharon [:ranbena] EverythingMe]

So can we expect the general font-size to stay 10px as well?

Comment 24

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

(In reply to Ran Ben Aharon (Everything.me) from comment #23)
> So can we expect the general font-size to stay 10px as well?

Yes.

Comment 25

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

Reassign to me since Fred is occupied this week.

Comment 26

[Ran Ben Aharon [:ranbena] EverythingMe]

Attachment: Evme Patch - redirect to github PR

* Fully merged the previous 22eb1a commit (meant for GeeksPhone @2x)
* Changed dependency on font-size and screen width to devicePixelRatio only reliance.
* Added 1.5x images
* Completed rem conversion

Comment 27

[Ran Ben Aharon [:ranbena] EverythingMe]

I have taken much of Fred's changes and incorporated in my PR
Notice that images are now crisp

Though, I could get the 1.5x images to load. Ismael, could it be that we're missing the build/webapp-zip.js fix? https://github.com/mozilla-b2g/gaia/blob/22eb1a81d9107a300823e98b3b3e871ca6991320/build/webapp-zip.js

Comment 28

[[e-mail]]

Ran, thanks for pick it up. Overall looks good.
I've left some comment on github for some missing `px` and `font` issue.


According to bug 881127, 1.1.0hd & master rewrote the @2x build patch to use `GAIA_DEV_PIXELS_PER_PX` instead of `HDPI` option in webapp-zip.js.

Comment 29

[Ismael Gonzalez [:basiclines]]

Comment on attachment 775709 [details]
Evme Patch - redirect to github PR

I've addressed a couple of issues in GitHub, plx fix them before merging.
The overall looks great, anyway i'm having some performance issues on my peak hope it is concerned only with Gecko.
Great work guys thx!

Comment 30

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

v1.1.0hd: https://github.com/mozilla-b2g/gaia/commit/56b6e515899af491742167e67b76e607c0846d6a

We would still need another patch to drop @1.5x assets into master branch.

Comment 31

[Ran Ben Aharon [:ranbena] EverythingMe]

Tim, the patch has the 4 @1.5x images but also "px" to "rem" code that's not implemented the way we did in our PR. Can we simply submit the images alone?

Comment 32

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

(In reply to Ran Ben Aharon (Everything.me) from comment #31)
> Tim, the patch has the 4 @1.5x images but also "px" to "rem" code that's not
> implemented the way we did in our PR. Can we simply submit the images alone?

We should just submit the images. Do you want to do it? Thanks.

Comment 33

[Ran Ben Aharon [:ranbena] EverythingMe]

Attachment: Evme Patch - images for master

Comment 34

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

Comment on attachment 777689 [details]
Evme Patch - images for master

Thank you!

Comment 35

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

master: https://github.com/mozilla-b2g/gaia/commit/5d8414449fe7be86a5664e969e0043a531d5cf72