905455 - [B2G][Gaia][e.me] [homescreen] e.me icons are oversized on Helix

Status: VERIFIED FIXED

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

Description

[John Hammink]

Attachment: oversized eme icons helix.png

E.me icons are oversized on Helix

Device is turned on

Reproduction Steps:
1) Tap Search Bar
2) Do a basic search


Actual:
Suggested apps are not presented small enough - icons and text overlap. (Screenshot attached)

Expected:
Icons and text are small enough that icons and text do not overlap.

helix version info:
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/afd50da34b9f
Gaia   4788826c0cfc7ca00dec36ed8dc6b3eaee9ec571
BuildID 20130813042203
Version 18.0

Comment 1

[Wayne Chang [:wchang]]

Vivien, can you have this looked at? We're also working with a bit of a tight schedule here.

Comment 2

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

Vivien, is on vacation. Ran should be able to help.

Comment 3

[Ran Ben Aharon [:ranbena] EverythingMe]

On it

Comment 4

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

Attachment: Patch proposal

I wonder if you guys use any CSS preprocessor, but do update the source file.

We do need to talk about including these source files in Gaia codebase on dev-gaia in the long run.

Comment 5

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

Attachment: Patch

Comment 6

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

Attachment: Patch proposal

Comment 7

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

Alternatively, these CSS sizes should be set dynamically by the script. But I have trouble identifying where I should put these CSS rules....

Comment 8

[Ran Ben Aharon [:ranbena] EverythingMe]

Tim, I'm reluctant in continuing to have such differences between v1-train and v1.1.0hd.

If you build v1-train on to an hdpi device it all looks fine (apart for image quality). I've noticed quite a lot of substantial code diffs between v1-train and v1.1.0hd which do not have to do with hd compliance. I am worried this patch will further the gap between the branches.

I need a bit more time to understand what's going on. What the deadline for this?

Comment 9

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

(In reply to Ran Ben Aharon (Everything.me) from comment #8)
> Tim, I'm reluctant in continuing to have such differences between v1-train
> and v1.1.0hd.
> 
> If you build v1-train on to an hdpi device it all looks fine (apart for
> image quality). I've noticed quite a lot of substantial code diffs between
> v1-train and v1.1.0hd which do not have to do with hd compliance. I am
> worried this patch will further the gap between the branches.
> 

I have no opinion on that as long as we made progress on dealing this bug. 

I've identified that the cause of this bug is because these <img> were stretched to their native size (1.5x because of window.devicePixelRatio), and not bounded by a CSS size.

> I need a bit more time to understand what's going on. What the deadline for
> this?

I am not sure at this stage actually. Wayne, any update?

Comment 10

[Wayne Chang [:wchang]]

Ran, does this problem exist on master as well or has it been taken care of on master?

Comment 11

[Ran Ben Aharon [:ranbena] EverythingMe]

Problem doesn't exist on master.

Comment 12

[Geeksphone]

Happens too in Peak, and possibly in other HD devices. Maybe some test in other HD devices?

Comment 13

[Ran Ben Aharon [:ranbena] EverythingMe]

The reason for this is apparent. Evme related code in v1.1.0hd is not up to date and does not match v1-train and master.
It will look like that in all HD devices.

Comment 14

[Alejandro]

Hi Ran, all. What are the next steps that have to be followed in order to update v1.1.0hd with v1-train/master SW?

Thanks
Alejandro

Comment 15

[Ran Ben Aharon [:ranbena] EverythingMe]

Either we find the problematic merge and fix it, or if it's not just one, we'll have to insert the changes manually.
I'm not confident though we'd be free to do it in the next 2 weeks (because of the upcoming WW)

Comment 16

[Geeksphone]

Attachment: ev.me.patch

Patch proposal tested on Geeksphone Peak for v1.1.0hd gaia branch.

gaia commit: a5a7c5b23c02b99b64825676cffebc8639c264df

Regards.

Comment 17

[Geeksphone]

Attachment: ev.me.patch-2

Same patch, but solving fonts.

Comment 18

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: device-2013-09-30-172144.png

current version of gaia shows the attachment.  only the built in icons are huge and has a line through the middle of the text.

Gaia   5de99819ca192295a8179ddd345de6cf7ebb2287

Comment 19

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: device-2013-09-30-172516.png

also to note, adding to the collections will have the icons overlap.  I think it's because of the size of the built in apps that causes this issue to occur.

Comment 20

[Ran Ben Aharon [:ranbena] EverythingMe]

Thanks Naoki, we can reproduce and we're on it.

Comment 21

[Ran Ben Aharon [:ranbena] EverythingMe]

Apparently, Bug 881145 (which was meant for v1.1.0hd) wasn't merged well into master.
We are now reapplying the relevant code to solve this bug.

Comment 22

[Evyatar 'Tron' Amitay (everything.me)]

Attachment: Patch - redirect to github PR

This patch fixes:
1. Above the line icons being too large and cut-off
2. Below the line icons being the right size, but of low quality
3. Old devicePixelRatio calculations that were done before the device supported native window.devicePixelRatio.

To review the changes properly please use the ignore-whitespace feature of github:
https://github.com/mozilla-b2g/gaia/pull/12637/files?w=1

Comment 23

[Cristian Rodriguez (:crdlc)]

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

I'll feel good with Ran's review because all code is over Evme's roof :) Thanks

Comment 24

[Ran Ben Aharon [:ranbena] EverythingMe]

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

Fully tested on a Geeksphone - fixed some bugs and works well.

Comment 25

[Ran Ben Aharon [:ranbena] EverythingMe]

Landed on master
https://github.com/mozilla-b2g/gaia/commit/847ad7a594566a5985f15991f11f648a93133f87

Comment 26

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

Need uplift.

Comment 27

[William Hsu [:whsu]]

Hi, Wayne,

Are you looking for this case?
Thanks!

Comment 28

[Wayne Chang [:wchang]]

Yes thanks. Have spoken to Tim on this already.

Comment 30

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

Evyatar, I am not be able to uplift this to v1.1.0hd. There are too many conflicts and I am not be able resolve it. Please provide a v1.1.0hd only patch for this bug. Thanks.

Comment 31

[Geeksphone]

Please review comment 17, is our proposed patch for v1.1.0hd and works correctly on Peak.

Comment 32

[Ran Ben Aharon [:ranbena] EverythingMe]

Hey Tim, this patch is meant for master only as E.me code had undergone refactoring.
v1.1.0hd and v1.2 have been merged with a dedicated patch in Bug 908057.

Comment 33

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

Verified on v1.1.0hd branch.

Comment 34

[Al Tsai [:atsai]]

I'll be back. :p

Comment 36

[William Hsu [:whsu]]

Hi, all,

Thanks for all your help.
I cannot reproduce this bug now.
Attaching the screenshot.

* Test Build: (Mozilla Build)
 - Gaia:     63cd1d96b5a94e9b22ffc54dfde48ef6d3cf996b
 - Gecko:    http://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/7cd7f30328e1
 - BuildID   20131022042302
 - Version   18.0

Comment 37

[William Hsu [:whsu]]

Attachment: [E.ME] Main page -01

Comment 38

[William Hsu [:whsu]]

Attachment: [E.ME] Main page -02