1140322 - [RTL][Wallpaper]The Grid of "Change wallpaper" view is not mirrored in RTL language.

Status: VERIFIED FIXED

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

Description

[Lancy(Leave from Mozilla)]

Attachment: Comparion.png

[1.Description]:
[RTL][v2.2&v3.0[Settings]Try to download the failed file again, the error icon is left-aligned and overlaps with refresh icon in download list.
See attachment:Comparion.png

[2.Testing Steps]: 
1. Set your phone language to Arabic. 
2. Launch settings and Select "Display". 
3. Tap the edit icon on the wallpaper. 
4. Tap "Wallpaper"

[3.Expected Result]: 
4. The grid of wallpaper view should be mirrored

[4.Actual Result]: 
4. The grid of wallpaper view is not mirrored.

[5.Reproduction build]: 
Flame 2.2 build:
Build ID               20150305002528
Gaia Revision          89af288bad6751248ff84504fa898206fee127fe
Gaia Date              2015-03-04 18:00:05
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6d8d294aa8f3
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150305.094337
Firmware Date          Thu Mar  5 09:43:49 EST 2015
Bootloader             L1TC000118D0

Flame 3.0 build:
Build ID               20150305072141
Gaia Revision          0017f2bbc63781a5409644b664d80ebaa1543653
Gaia Date              2015-03-05 00:46:59
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/993eb76a8bd6
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150305.160341
Firmware Date          Thu Mar  5 16:03:51 EST 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
15884

Comment 1

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

Note: Lancy your Description is incorrect, but it's clearly that you copy/pasted from your previous bug description and forgot to change that part ;) The testing steps, title of bug and screenshot confirm that.

Triage P1 - Nominating as this is a frequently used screen and totally inconsistent with RTL behavior that is found throughout Gallery, Music, Video

Comment 2

[bhavana bajaj [:bajaj]]

Given this a commonly used feature and this is inconsistent with the spec per comment above, blocking.

Comment 3

[Hema Koka [:hema]]

Punam to the rescue! Assigning to you.

Thanks
Hema

Comment 4

[Punam Dahiya [:pdahiya]]

Thanks Hema, will investigate and submit the fix.

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] punamdahiya:Bug1140322 > mozilla-b2g:master

Comment 6

[Punam Dahiya [:pdahiya]]

Comment on attachment 8576778 [details] [review]
[gaia] punamdahiya:Bug1140322 > mozilla-b2g:master

Hi David

Please review attached patch that mirrors select wallpaper grid in RTL mode. Thanks!

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8576778 [details] [review]
[gaia] punamdahiya:Bug1140322 > mozilla-b2g:master

Thanks for the quick fix, Punam.

Long ago we had a bug in the gallery app where using margins instead of borders to separate the images made scrolling much slower. There was some graphics issue where having the background color showing through transparent margins caused a lot more work for the compositor or something.  So I tested this patch for performance, and it does not seem to affect frame rate or the overdraw number.

I think, though, that this patch replaces black borders with the almost-but-not-quite black background color, so I think you should add background-color:#000 to #wallpapers or change the background color on the head and body.  That will make a difference if we ever have built in wallpapers that do not divide into groups of three. The unoccupied slots will be pure black instead of almost black. I don't know if anyone really cares much about that.

I've got no idea why this app is using position:relative and float:left. The only reason we need the RTL fix is because of the float:left. So if we do more standard layout here, then we don't need a fix. You could try the patch below intead, though it is arguably a riskier patch because it changes more. So for 2.2 maybe your existing patch is better.


diff --git a/apps/wallpaper/style/wallpaper.css b/apps/wallpaper/style/wallpaper.css
index 76fda7a..9b3fe2a 100644
--- a/apps/wallpaper/style/wallpaper.css
+++ b/apps/wallpaper/style/wallpaper.css
@@ -10,7 +10,7 @@ html, body {
   margin: 0;
   padding: 0;
   font-size: 10px;
-  background-color: #26272c;
+  background-color: #000;
   overflow: hidden;
   height: 100%;
 }
@@ -19,21 +19,17 @@ html, body {
 #wallpapers {
   overflow: auto;
   height: calc(100% - 5rem);
+  line-height:0;
 }
 
 .wallpaper {
-  position: relative;
+  display: inline;
   overflow: hidden;
-  float: left;
   width: 33%;
   -moz-margin-end: 0.1rem;
   margin-bottom: 0.1rem;
 }
 
-html[dir="rtl"] .wallpaper {
-  float: right;
-}
-
 #wallpapers .wallpaper:nth-child(3n) {
   -moz-margin-end: 0;
 }

Comment 8

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #7)
> Comment on attachment 8576778 [details] [review]
> [gaia] punamdahiya:Bug1140322 > mozilla-b2g:master
> 
> Thanks for the quick fix, Punam.
> 
> Long ago we had a bug in the gallery app where using margins instead of
> borders to separate the images made scrolling much slower. There was some
> graphics issue where having the background color showing through transparent
> margins caused a lot more work for the compositor or something.  So I tested
> this patch for performance, and it does not seem to affect frame rate or the
> overdraw number.
> 
> I think, though, that this patch replaces black borders with the
> almost-but-not-quite black background color, so I think you should add
> background-color:#000 to #wallpapers or change the background color on the
> head and body.  That will make a difference if we ever have built in
> wallpapers that do not divide into groups of three. The unoccupied slots
> will be pure black instead of almost black. I don't know if anyone really
> cares much about that.
> 
> I've got no idea why this app is using position:relative and float:left. The
> only reason we need the RTL fix is because of the float:left. So if we do
> more standard layout here, then we don't need a fix. You could try the patch
> below intead, though it is arguably a riskier patch because it changes more.
> So for 2.2 maybe your existing patch is better.
> 
> 
> diff --git a/apps/wallpaper/style/wallpaper.css
> b/apps/wallpaper/style/wallpaper.css
> index 76fda7a..9b3fe2a 100644
> --- a/apps/wallpaper/style/wallpaper.css
> +++ b/apps/wallpaper/style/wallpaper.css
> @@ -10,7 +10,7 @@ html, body {
>    margin: 0;
>    padding: 0;
>    font-size: 10px;
> -  background-color: #26272c;
> +  background-color: #000;
>    overflow: hidden;
>    height: 100%;
>  }
> @@ -19,21 +19,17 @@ html, body {
>  #wallpapers {
>    overflow: auto;
>    height: calc(100% - 5rem);
> +  line-height:0;
>  }
>  
>  .wallpaper {
> -  position: relative;
> +  display: inline;
>    overflow: hidden;
> -  float: left;
>    width: 33%;
>    -moz-margin-end: 0.1rem;
>    margin-bottom: 0.1rem;
>  }
>  
> -html[dir="rtl"] .wallpaper {
> -  float: right;
> -}
> -
>  #wallpapers .wallpaper:nth-child(3n) {
>    -moz-margin-end: 0;
>  }

Thanks David for the review. I agree to use existing patch with background color #000 added to body for 2.2. I have created bug 1143060 to update wallpaper app to standard layout using style changes suggested above.

Comment 9

[GitHub Autolander Bot]

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

Comment 10

[Punam Dahiya [:pdahiya]]

Comment on attachment 8576778 [details] [review]
[gaia] punamdahiya:Bug1140322 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Not a regression
[User impact] if declined: In RTL mode, change wallpaper grid will not be mirrored
[Testing completed]: On master
[Risk to taking this patch] (and alternatives if risky): Very low
[String changes made]: None

Comment 11

[Lancy(Leave from Mozilla)]

Attachment: Verify1_Wallpaper_Flame3.0.png

Hi, josh,
This bug has been verified to fail on latest Flame 3.0 version.
Could you help with this bug?
Thank you.

See attachment:Verify1_Wallpaper_Flame3.0.png
Reproducing rate:10/10

Flame 3.0 build:
Build ID               20150315160203
Gaia Revision          d4177902b04b8fedcb7df9a30ae6e9677e03d2d4
Gaia Date              2015-03-13 15:58:35
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/af68c9c0e903
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150315.192711
Firmware Date          Sun Mar 15 19:27:22 EDT 2015
Bootloader             L1TC000118D0

Comment 12

[Lancy(Leave from Mozilla)]

Modify the description to:
[RTL][v2.2&v3.0[Settings]Edit the wallpaper in "Change wallpaper" view, the Grid is not mirrored in RTL language.

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

Comment 13

[(PTO June 19-July 7) Ryan VanderMeulen]

v2.2: https://github.com/mozilla-b2g/gaia/commit/5f9d06333e20cb4bbd00a20e45428af58e6a177a

Comment 14

[Josh Cheng [:josh]]

Lancy,
Can you also try on latest 2.2? It should be also fixed on 2.2 too.

Comment 15

[Lancy(Leave from Mozilla)]

Attachment: Verify2_Wallpaper_Flame2.2_Pass.png

This bug has been verified successfully on latest Flame 2.2 and 3.0 build.

See attachment:Verify2_Wallpaper_Flame2.2_Pass.png and Verify2_Wallpaper_Flame3.0_Pass.png
Reproducing rate:0/10

Flame 2.2 build:
Build ID               20150316162504
Gaia Revision          d0e09d5e6367e558824f9cbf691da99cedf63037
Gaia Date              2015-03-16 17:14:22
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/793d61bb0bd4
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150316.195035
Firmware Date          Mon Mar 16 19:50:48 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 build:
Build ID               20150316160204
Gaia Revision          4868c56c0a3b7a1e51d55b24457e44a7709ea1ae
Gaia Date              2015-03-14 18:50:17
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/436686833af0
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150316.193248
Firmware Date          Mon Mar 16 19:32:58 EDT 2015
Bootloader             L1TC000118D0

Comment 16

[Lancy(Leave from Mozilla)]

Attachment: Verify2_Wallpaper_Flame3.0_Pass.png