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

VERIFIED FIXED in 2.2 S6 (20feb)

Status

defect
P1
normal
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: psiphantong, Assigned: mikehenrty)

Tracking

unspecified
2.2 S6 (20feb)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(feature-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [systemsfe])

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

5 years ago
Posted image 2014-12-23-16-22-48.png (obsolete) —
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
Reporter

Updated

5 years ago
Blocks: settings-rtl
QA Whiteboard: [QAnalyst-Triage?][rtl-impact]
Flags: needinfo?(dharris)
QA Whiteboard: [QAnalyst-Triage?][rtl-impact] → [QAnalyst-Triage+][rtl-impact]
Flags: needinfo?(dharris)
Decided during triage this morning that this would block 2.2
blocking-b2g: --- → 2.2?
Priority: -- → P1
Blocking per RTL triage for 2.2
blocking-b2g: 2.2? → 2.2+
RTL update: marking required bugs as feature-b2g:2.2+ (and removing blocking flags)
blocking-b2g: 2.2+ → ---
feature-b2g: --- → 2.2+
Whiteboard: [systemsfe]
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
==
Flags: needinfo?(psiphantong)
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 !
Posted image 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 :)
Flags: needinfo?(swilkes)
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).
Flags: needinfo?(swilkes)
Status: NEW → ASSIGNED
QA Contact: tclancy
Assignee: nobody → tclancy
QA Contact: tclancy
Let's just continue to use this bug since the STR are still valid and we already have a nice screenshot.
Flags: needinfo?(psiphantong)
Summary: [RTL][Download] Tapping a download notification, the downloads screen does not fit the screen → [RTL][Download] Downloads list, icons overlap text
Attachment #8541019 - Attachment is obsolete: true
(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 ! :)
Target Milestone: --- → 2.2 S6 (20feb)
Stealing.
Assignee: tclancy → mhenretty
Component: Gaia::System::Window Mgmt → Gaia::Settings
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?
Attachment #8564552 - Flags: review?(ejchen)
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 :)
Attachment #8564552 - Flags: review?(ejchen) → review+
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.
Assignee: mhenretty → tclancy
Attachment #8564552 - Attachment is obsolete: true
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.
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 ميجابايت".
Posted image fixed.png (obsolete) —
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 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.
Attachment #8564839 - Flags: review?(kgrandon)
Attachment #8564839 - Flags: review?(arthur.chen)
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!
Attachment #8564839 - Flags: review?(pacorampas)
Attachment #8564839 - Flags: review?(kgrandon)
Attachment #8564839 - Flags: feedback+
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 on attachment 8564839 [details] [review]
[gaia] tedders1:bug-1115213-fix > mozilla-b2g:master

EJ, could you help check this? Thanks.
Attachment #8564839 - Flags: review?(arthur.chen) → review?(ejchen)
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?
Flags: needinfo?(swilkes)

Updated

4 years ago
Attachment #8564839 - Flags: review?(pacorampas)
(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 !
Attachment #8564552 - Attachment is obsolete: false
Attachment #8564839 - Attachment is obsolete: true
Attachment #8564862 - Attachment is obsolete: true
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.)
Assignee: tclancy → mhenretty
Flags: needinfo?(mhenretty)
Let's just let Stephany do a UI review of the new screen.
Flags: needinfo?(swilkes)
Flags: needinfo?(mhenretty)
Attachment #8565428 - Flags: ui-review?(swilkes)

Comment 28

4 years ago
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.
Attachment #8565428 - Flags: ui-review?(swilkes) → ui-review+
(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.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
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.
Attachment #8564552 - Flags: approval-gaia-v2.2?
Attachment #8564552 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+][rtl-impact] → [QAnalyst-Triage?][rtl-impact]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][rtl-impact] → [QAnalyst-Triage+][rtl-impact]
Flags: needinfo?(ktucker)
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15744/
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.