1115213 - [RTL][Download] Downloads list, icons overlap text

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P1)

Description

[pete siphantong [:Petes]]

Attachment: 2014-12-23-16-22-48.png

Description:
When the phone is on landscape and tapping a download notification, the downloads screen does not fit the screen


Repro Steps:
1) Update a Flame to 20141223010202
2) Go to Browser >  rotate phone to landscape
3) Download a large file from https://owd.tid.es/dm/
4) Tap on the download start notification 


Actual:
the downloads screen does not fit the screen


Expected:
the downloads screen displays correctly 


Environmental Variables:
Device: Flame 2.2 (319mb)(Kitkat Base)(Full Flash)
Build ID: 20141223010202
Gaia: c2da2bafd4e809317e2ca70c9bf5c11136a32818
Gecko: 0532f2509f3f
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


Repro frequency:100%
See attached: screenshot

Comment 1

[Delphine Lebédel [:delphine - Use needinfo]]

Decided during triage this morning that this would block 2.2

Comment 2

[bhavana bajaj [:bajaj]]

Blocking per RTL triage for 2.2

Comment 3

[Dylan Oliver [:doliver]]

RTL update: marking required bugs as feature-b2g:2.2+ (and removing blocking flags)

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi Pete,

the screen does fit now, can you try with latest 2.2 to verify this bug again ? Thanks !!


== my build info ==
Gaia-Rev        46b590648007d51a0406b21b1d6f98eba8e3898e
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b03afde7e699
Build-ID        20150128162504
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150120.192939
FW-Date         Tue Jan 20 19:29:50 EST 2015
Bootloader      L1TC10011800
==

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

But it seems that icons are overlapped with text and the style looks a little bit weird, if we are going to fix this instead of the screen fitting problem, we should change the bug description / title or file another bug and ni? related RTL visual for this part.

Thanks !

Comment 6

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: demo.png

Hi Stephany,

currently if users are using RTL locales and navigating to Settings > Download Manager, they will see something like this demo image (icons are overlapped with strings)

For me, this looks really weird and may need to be fixed. I did try to find some specs for this part but with no luck. Do you have any further ideas or latest spec for this case ? Thanks :)

Comment 7

[Delphine Lebédel [:delphine - Use needinfo]]

I believe what you now see is a different bug since it's not that the downloads screen doesn't fit anymore. Please file a separate one for the issue you describe.
Concerning the specs you're asking for on 2.2: you can find the appropriate behaviors that I have identified from your screenshot explained on p.11 (position of buttons), p.9 (spinner), p.15 (progress bar), p.11 (headers).

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

Let's just continue to use this bug since the STR are still valid and we already have a nice screenshot.

Comment 9

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Michael Henretty [:mhenretty] from comment #8)
> Let's just continue to use this bug since the STR are still valid and we
> already have a nice screenshot.

Yes, I thought we can still use this bug to track this issue because the STR & screenshot are all here. Thanks for taking this bug, Michael ! :)

Comment 10

[Michael Henretty [:mikehenrty][:mhenretty]]

Stealing.

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] mikehenrty:bug-1115213-rtl-stop-resume-dl > mozilla-b2g:master

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8564552 [details] [review]
[gaia] mikehenrty:bug-1115213-rtl-stop-resume-dl > mozilla-b2g:master

Part 2, from bug 1131486. EJ, can you have a look here too?

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] tedders1:bug-1115213-fix > mozilla-b2g:master

Comment 14

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8564552 [details] [review]
[gaia] mikehenrty:bug-1115213-rtl-stop-resume-dl > mozilla-b2g:master

This patch looks nice to me ! please ask Helen for ui review by the way.

And because this is part 2 of bug 1131486, it would be better to take a screenshot with part 1 landed. Otherwise, the screenshot may look weird xD

Thanks :)

Comment 15

[Ted Clancy [:tedders1]]

Stealing back.

Mike, can you give me a heads up before stealing one of my bugs? I've been working on this a couple days already and was in the process of landing a patch.

Comment 16

[Ted Clancy [:tedders1]]

If you look at the attachment called demo.png, you'll see there are multiple other problems with this screen, apart from just the icon overlapping the text.

1) The progress text says "3.96 ميجابايت من 96.95 ميجابايت" instead of "3.96 ميجابايت من 96.95 ميجابايت". I reported this as Bug 1132777, but it makes more sense to fix it here.

2) The check mark icon in the upper right corner is reversed.

The reason for number 1 is because of code in apps/settings/style/downloads.css which specifies that the download list should be treated as LTR even in an RTL context. I think that's a mistake, and so my patch removes that code. This has the side effect of fixing the icon that overlaps with the text too.

The reason for number 2 is because of code in shared/style/buttons.css that flips header icons in RTL contexts. I think that's a naïve mistake, and my patch removes it. I guess the intention is that prev/next arrows should be reversed in RTL, but there are lots of reasons not to do this.
* Flipping graphics doesn't work if the arrows have shadows, or are otherwise asymetrical.
* Our current UI specs actually say NOT to swap forward/back arrows in RTL.
* Flipping anything that isn't an arrow is probably a mistake.
* Where we do need to swap forward/back arrows in RTL, we usually have special handling.

Comment 17

[Ted Clancy [:tedders1]]

Ugh. Bugzilla really doesn't handle Bidi text well.

Number 1 should say:
The progress text says "ميجابايت من 96.95 ميجابايت 3.96", instead of "‏3.96 ميجابايت من 96.95 ميجابايت".

Comment 18

[Ted Clancy [:tedders1]]

Attachment: fixed.png

This is a screenshot of what the Downloads screen looks like with my fix. Compare to demo.png.

Note the correctly oriented checkmark in the upper-right corner, and the correct order of the progress text.

Comment 19

[Ted Clancy [:tedders1]]

Comment on attachment 8564839 [details] [review]
[gaia] tedders1:bug-1115213-fix > mozilla-b2g:master

Assigning :kgrandon to review the change to shared/style/buttons.css, and :arthurcc to review the change to apps/settings/style/download.css.

Comment 20

[Ted Clancy [:tedders1]]

Green try run: https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=d3a80c22c704

Comment 21

[Kevin Grandon :kgrandon]

Comment on attachment 8564839 [details] [review]
[gaia] tedders1:bug-1115213-fix > mozilla-b2g:master

I don't really know why we have the transform() like this, but it looks like it was introduced in bug 1076759.  

I think it would be good to get a review from :paco on this. Thanks for the patch!

Comment 22

[Paco Rampas [:paco]]

Hello Kevin, 

That transform() is for making the mirroring image. In this case there isn't any change because the image is simetric, but for example with arrows the image should flip. Then, I would leave the transform.

Thanks ;)

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8564839 [details] [review]
[gaia] tedders1:bug-1115213-fix > mozilla-b2g:master

EJ, could you help check this? Thanks.

Comment 24

[Michael Henretty [:mikehenrty][:mhenretty]]

Ted, 

Ah sorry, the reason I stole this bug is because I was working on bug 1131486 which is in the same area, so I figured I could fix this quickly. This hadn't moved in two weeks, so it would have been helpful if you had uploaded a WIP patch so people know that this is indeed moving since we are only 1 week from the deadline now.


(In reply to Ted Clancy [:tedders1] from comment #16)
> 1) The progress text says "3.96 ميجابايت من 96.95 ميجابايت" instead of "3.96
> ميجابايت من 96.95 ميجابايت". I reported this as Bug 1132777, but it makes
> more sense to fix it here.

Yeah, this is bug 1131486. That one already has an r+ and also fixes the alignment of the checkboxes, so let's fix this problem in that bug.


> 2) The check mark icon in the upper right corner is reversed.

Was this not fixed by bug 1130307? In master, I see the icon displaying properly. If it's not fixed, please re-open that bug instead of fixing it here.


Also, I don't think your patch completely fixes the pause/resume icons. In order to correctly mirror the download list from LTR, the pause/resume icon should display at far end of the list item, which in RTL mode would be the left side of the screen. Let's get clarification from UX to be sure.

Stephany, which side of the screen should the pause (white X) and resume (white circle) icons display on in RTL mode for the download list?

Comment 25

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Michael Henretty [:mhenretty] from comment #24)
> This hadn't moved in two weeks, so it would have been helpful if you had
> uploaded a WIP patch so people know that this is indeed moving since we are
> only 1 week from the deadline now.

Yeah, thumbs up for this idea. There must be at least one WTP, otherwise the others may get confused especially RTL support is quite late and urgent.

> (In reply to Ted Clancy [:tedders1] from comment #16)
> > 1) The progress text says "3.96 ميجابايت من 96.95 ميجابايت" instead of "3.96
> > ميجابايت من 96.95 ميجابايت". I reported this as Bug 1132777, but it makes
> > more sense to fix it here.
> 
> Yeah, this is bug 1131486. That one already has an r+ and also fixes the
> alignment of the checkboxes, so let's fix this problem in that bug.
> 

Ted & Michael,

I don't mind who is the owner who would fix this problem, let's just fix it :)

> 
> > 2) The check mark icon in the upper right corner is reversed.
> 
> Was this not fixed by bug 1130307? In master, I see the icon displaying
> properly. If it's not fixed, please re-open that bug instead of fixing it
> here.
> 

I think this should be fixed in my bug 1130307.

What Ted did here is trying to fix the problem from gaia-header instead of downloads.css. Based on Paco's comment 22, I am not quite sure about the main reason why we need transform here, so I would this part to him.

> 
> Also, I don't think your patch completely fixes the pause/resume icons. In
> order to correctly mirror the download list from LTR, the pause/resume icon
> should display at far end of the list item, which in RTL mode would be the
> left side of the screen. Let's get clarification from UX to be sure.
> 
> Stephany, which side of the screen should the pause (white X) and resume
> (white circle) icons display on in RTL mode for the download list?

Base on page 15 in latest RTL spec (https://mozilla.app.box.com/s/bcm3s5i2v6js5uk0ws3tsywse8bgncgo), I think we should put the pause/resume icon in the end (LTR: right, RTL: left). Let's wait for Stephany's comment to double confirm this part.

So for me, I think there are still some parts that needs to be classified (gaia-header mirroring problems, spec definition). I'll remove the r? first :)

Thanks all !

Comment 26

[Ted Clancy [:tedders1]]

Alrighty, Mike. You can have this back. Go ahead and land it.

I still think it's a mistake to flip the header icons by default and then override it in almost every place we have a header icon. (And if we ever switch to icons which aren't reversable, like with a shadow or bevel, it will be 100% useless.)

Comment 27

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [screenshot] pause/resume icons

Let's just let Stephany do a UI review of the new screen.

Comment 28

[Stephany Wilkes]

Comment on attachment 8565428 [details]
[screenshot] pause/resume icons

Ui-review+ with a caveat. :) The icons (per page 15, bottom row, screen on the right) look like they're in the correct place, which is on the left side. The graphic assets shown for the icons do not look correct, however, so let's make sure we're using the correct x and refresh icon assets.

Comment 29

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Stephany Wilkes from comment #28)
> Comment on attachment 8565428 [details]
> [screenshot] pause/resume icons
> 
> Ui-review+ with a caveat. :) The icons (per page 15, bottom row, screen on
> the right) look like they're in the correct place, which is on the left
> side. The graphic assets shown for the icons do not look correct, however,
> so let's make sure we're using the correct x and refresh icon assets.

These are indeed the correct assets (ie. they are the same as LTR mode). I agree they look kind of out of place, but they have been like this since I believe we implemented the download list. We should revisit the look of these icons, but probably not in this bug.

Comment 30

[Michael Henretty [:mikehenrty][:mhenretty]]

Rebased after bug 1131486 landed. Just waiting for the try run:
https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=8469eac6fd2e

Comment 31

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/404bbb2cefb34241e0e4aa1b1fea2ddd51c0d7bc

Comment 32

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8564552 [details] [review]
[gaia] mikehenrty:bug-1115213-rtl-stop-resume-dl > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
RTL feature.

[User impact] if declined:
Inconsistent display for download list between RTL and LTR, ie. bad UX.

[Testing completed]:
Manually tested with UX review.

[Risk to taking this patch] (and alternatives if risky):
CSS only change only effecting RTL. Low risk.

[String changes made]: none.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/35edbcd3cf420295929295310bcbdb1d06f1c7f6

Comment 34

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue is verified fixed on the latest Nightly  Flame KK 3.0 and 2.2 builds.

Actual Results: The downloads page text and icons do not overlap.

Environmental Variables:
Device: Flame 3.0 KK (Full Flash) (319 MB)
BuildID: 20150223010224
Gaia: a6881205deae450757a8d1e1ed65e5e5be0ec633
Gecko: 86d2bb8bb1c9
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Environmental Variables:
Device: Flame 2.2 KK (Full Flash) (319 MB)
BuildID: 20150223002503
Gaia: 389542b71c89253c0d176d3b0bfb54e275c19bf1
Gecko: 9fd3441c8983
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 35

[Lancy(Leave from Mozilla)]

Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15744/