1134436 - [RTL][Gallery] Exposure slider is not mirrored

Status: VERIFIED FIXED

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

Description

[Yeojin Chung [:YeojinC]]

Attachment: ExplosureSlider.png

Description:
The exposure slider is not mirrored during the edit mode. According to the Bidirectional (BiDi) Guidelines (https://mozilla.app.box.com/s/bcm3s5i2v6js5uk0ws3tsywse8bgncgo), "Sliders are mirrored with maxi-mums at left and minimums at right" (p.18).

Pre-requisite: Have at least 1 photo in the device. 

Repro Steps:
1) Update a Flame to 20150218010226.
2) Set the device language in Arabic under Settings > Language.
3) Open Gallery > Select any photo > Select the edit icon > Select exposure icon.

Actual:
The slider is not mirrored. The minimum value is on the far left, and the maximum value is on the far right.

Expected:
The slider is mirrored. The minimum value is on the far right, and the maximum value is on the fart left.

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150218010226
Gaia: 82f286f10a41aab84a0796c89fbefe67b179994b
Gecko: 9696d1c4b3ba
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

Repro frequency: 3/3
See attached: screenshot

Comment 1

[Yeojin Chung [:YeojinC]]

This issue also reproduces on Flame 2.2.

Result: The slider on the Exposure edit screen is not mirrored.

Device: Flame 2.2 (KK, 319mb, full flash)
Build ID: 20150218002515
Gaia: da509caa7395d3d090ce973e8de082b4680a590d
Gecko: 96da179a7d3a
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 2

[GitHub Autolander Bot]

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

Comment 3

[Punam Dahiya [:pdahiya]]

Comment on attachment 8566802 [details] [review]
[gaia] punamdahiya:Bug1134436 > mozilla-b2g:master

Hi David
Please review attached patch that mirrors exposure slider for RTL locales. Thanks!

Comment 4

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

This is a regression
Triage as P1 as this touches an important screen and RTL needs to be consistent across screens

Comment 5

[Punam Dahiya [:pdahiya]]

(In reply to Delphine Lebédel [:delphine - use need info] from comment #4)
> This is a regression
> Triage as P1 as this touches an important screen and RTL needs to be
> consistent across screens

Hi Delphine
AFAIK this RTL bug has always been there, is the regression flag set by mistake?

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8566802 [details] [review]
[gaia] punamdahiya:Bug1134436 > mozilla-b2g:master

r- because there is a problem, I think, with the way you call the ready() function each time the user starts editing. I think you'll need to switch to listening for 'localized' events because you can remove the event listener in that case.  Or, since there is only one global exposureSlider, you could just move the ready() call out to the global scope so it only gets called once.

I haven't tested this, but otherwise it looks fine to me.  I wonder if you can position things with margin-start instead of left or right and avoid having to explicitly handle the language direction.  You might try and see if that works, but if it isn't easy, then this is fine the way it is.

Comment 7

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

I might be wrong, but I'm almost sure this wasn't the case before... sliders used to be mirrored correctly

Comment 8

[Punam Dahiya [:pdahiya]]

Comment on attachment 8566802 [details] [review]
[gaia] punamdahiya:Bug1134436 > mozilla-b2g:master

Hi David
I have updated patch to use add and remove event listeners for the 'localized' event and -moz-margin-start to position slider label and thumb. Please review. Thanks!

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8566802 [details] [review]
[gaia] punamdahiya:Bug1134436 > mozilla-b2g:master

Looks good to me. Thanks.

Comment 10

[GitHub Autolander Bot]

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

Comment 11

[Punam Dahiya [:pdahiya]]

Comment on attachment 8566802 [details] [review]
[gaia] punamdahiya:Bug1134436 > mozilla-b2g:master

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame Master and 2.2.

Result: The exposure slider is mirrored.
 
Device: Flame Master (KK, 319mb, full flash)
Build ID: 20150225010244
Gaia: f6bfd854fe4746f21bc006eac145365e85f98808
Gecko: 0a8b3b67715a
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Device: Flame 2.2 (KK, 319mb, full flash)
Build ID: 20150225002505
Gaia: ca64f2fe145909f31af266b1730874051ba76c78
Gecko: 16804008c29f
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 14

[Alissa（leave from Mozilla）]

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