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)
Tracking
(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)
People
(Reporter: yulan.zhu, Assigned: pdahiya)
References
Details
Attachments
(5 files)
[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
Reporter | ||
Updated•9 years ago
|
QA Whiteboard: [rtl-impact]
Comment 1•9 years ago
|
||
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
Updated•9 years ago
|
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.
Comment 2•9 years ago
|
||
Given this a commonly used feature and this is inconsistent with the spec per comment above, blocking.
blocking-b2g: 2.2? → 2.2+
Comment 3•9 years ago
|
||
Punam to the rescue! Assigning to you. Thanks Hema
Assignee: nobody → pdahiya
Assignee | ||
Comment 4•9 years ago
|
||
Thanks Hema, will investigate and submit the fix.
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Keywords: checkin-needed
Comment 9•9 years ago
|
||
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/25a9092ffee8035bc486e1476fe79f5dd4493ea8
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8576778 -
Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
Reporter | ||
Comment 11•9 years ago
|
||
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)
Reporter | ||
Comment 12•9 years ago
|
||
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+
Reporter | ||
Updated•9 years ago
|
QA Whiteboard: [rtl-impact] → [rtl-impact][MGSEI-Triage+]
Comment 13•9 years ago
|
||
v2.2: https://github.com/mozilla-b2g/gaia/commit/5f9d06333e20cb4bbd00a20e45428af58e6a177a
Target Milestone: --- → 2.2 S8 (20mar)
Comment 14•9 years ago
|
||
Lancy, Can you also try on latest 2.2? It should be also fixed on 2.2 too.
Flags: needinfo?(jocheng) → needinfo?(yulan.zhu)
Reporter | ||
Comment 15•9 years ago
|
||
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)
Reporter | ||
Comment 16•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•