Closed Bug 1140322 Opened 9 years ago Closed 9 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

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

VERIFIED FIXED
2.2 S8 (20mar)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: yulan.zhu, Assigned: pdahiya)

References

Details

Attachments

(5 files)

Attached image 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
QA Whiteboard: [rtl-impact]
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
blocking-b2g: --- → 2.2?
Priority: -- → P1
Component: Gaia::Settings → Gaia::Wallpaper
Summary: [RTL][Settings]The Grid of "Change wallpaper" view is not mirrored in RTL language. → [RTL][Wallpaper]The Grid of "Change wallpaper" view is not mirrored in RTL language.
Given this a commonly used feature and this is inconsistent with the spec per comment above, blocking.
blocking-b2g: 2.2? → 2.2+
Punam to the rescue! Assigning to you.

Thanks
Hema
Assignee: nobody → pdahiya
Thanks Hema, will investigate and submit the fix.
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!
Attachment #8576778 - Flags: review?(dflanagan)
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;
 }
Attachment #8576778 - Flags: review?(dflanagan) → review+
(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.
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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
Attachment #8576778 - Flags: approval-gaia-v2.2?(bbajaj)
Attachment #8576778 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
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
Flags: needinfo?(jocheng)
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/
Flags: in-moztrap+
Keywords: verifyme
QA Whiteboard: [rtl-impact] → [rtl-impact][MGSEI-Triage+]
Lancy,
Can you also try on latest 2.2? It should be also fixed on 2.2 too.
Flags: needinfo?(jocheng) → needinfo?(yulan.zhu)
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
Flags: needinfo?(yulan.zhu)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: