1156915 - [RTL][Gallery] Positive / Negative (+/-) signs appear on the right side of numbers on the Exposure Brightness edit screen

Status: VERIFIED FIXED

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

Description

[Martin Shuman [:Marty]]

Attachment: RTL-Gallery-Screenshot.png

Description:
The signed numbers on the Exposure Brightness edit screen display the '+' and '-' signs on the right side of the numbers. These numbers should appear on the left side.

Prerequisite: Have at least one image in the gallery on the device.

Repro Steps:
1) Update a Flame to 20150421010201
2) Change the Language to Arabic ('عربي') and restart the device.
3) Open the Gallery and select an image
4) Enter Edit Mode and select the Exposure Brightness mode

Actual:
The number labels for the brightness slider have their positive/negative signs improperly on the right side

Expected:
The number labels for the brightness slider have their positive/negative signs properly on the left side

Environmental Variables:
Device: Flame 3.0 (319MB)(Full Flash)
Build ID: 20150421010201
Gaia: a8e4f95dce9db727dda5d408b038f97fb4296557
Gecko: 7b823253d9f2
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Repro frequency: 8/8
See attached: Screenshot

Comment 1

[Martin Shuman [:Marty]]

This issue DOES occur on Flame 2.2 builds.
The number labels for the brightness slider have their positive/negative signs improperly on the right side

Environmental Variables:
Device: Flame 2.2 (319MB)(Full Flash)
Build ID: 20150421002501
Gaia: 828dd03a0e3b140d74b2e49355197df4d91d9227
Gecko: 36f72a3efb9b
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
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 2

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

Triage: P1. Nominating since this isn't how it should look like in Arabic

Comment 3

[David Flanagan [:djf]]

Punam: as with so many recent RTL bugs, I suspect that the fix here is to just put all the labels in <bdi></bdi> tags

Comment 4

[GitHub Autolander Bot]

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

Comment 5

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #3)
> Punam: as with so many recent RTL bugs, I suspect that the fix here is to
> just put all the labels in <bdi></bdi> tags

That's correct, putting the labels in bdi tag forces +/- sign to always shows up on left. I have attached the patch with the fix. Please review. Thanks!

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8596740 [details] [review]
[gaia] punamdahiya:Bug1156915 > mozilla-b2g:master

This patch doesn't actually work for me. If I set the language to 'runtime mirrored', I still see strings like "2+" instead of "+2".

Strangely, if I select the Arabic looking language near the beginning of the list, the gallery app doesn't seem to be localized. I get lots of english strings.  And these numbers look right, but it appears that the app isn't actually localized, so that doesn't count.

I don't know if just the CSS doesn't work, or if the bdi wouldn't have worked either.

It occurs to me now, however, that the easier thing would be to just force a dir=ltr on the element.  I forget what the CSS to do that is, but it I think that would work.  Since you know the text is numeric and will always be ltr, it seems fine to do it that way.  That would be simpler than bdi!

Comment 7

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #6)
> Comment on attachment 8596740 [details] [review]
> [gaia] punamdahiya:Bug1156915 > mozilla-b2g:master
> 
> This patch doesn't actually work for me. If I set the language to 'runtime
> mirrored', I still see strings like "2+" instead of "+2".
> 
It's my bad , I tested before rebasing the patch. Rebased patch doesn't work for me also.

> Strangely, if I select the Arabic looking language near the beginning of the
> list, the gallery app doesn't seem to be localized. I get lots of english
> strings.  And these numbers look right, but it appears that the app isn't
> actually localized, so that doesn't count.
> 
> I don't know if just the CSS doesn't work, or if the bdi wouldn't have
> worked either.
> 
I have noticed after make install-gaia, selecting Arabic language doesn't localize the page.  
Selecting mirroredRuntime as language sets the correct locale and works to test RTL bugs.

> It occurs to me now, however, that the easier thing would be to just force a
> dir=ltr on the element.  I forget what the CSS to do that is, but it I think
> that would work.  Since you know the text is numeric and will always be ltr,
> it seems fine to do it that way.  That would be simpler than bdi!

Setting direction:ltr and unicode-bidi as -moz-isolate forces content to stay ltr. I will update patch and submit again for review. Thanks!

Comment 8

[Punam Dahiya [:pdahiya]]

Comment on attachment 8596740 [details] [review]
[gaia] punamdahiya:Bug1156915 > mozilla-b2g:master

Setting review flag on updated patch. Please review. Thanks!

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8596740 [details] [review]
[gaia] punamdahiya:Bug1156915 > mozilla-b2g:master

I didn't test it myself. Do you really need both direction:ltr and unicode-bidi: -moz-isolate?  I would have thought that just direction:ltr would be enough.  If you do need both, then r+ to land it this way. But if direction:ltr is enough, then I'd prefer that you remove the unicode-bidi property.

Comment 10

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #9)
> Comment on attachment 8596740 [details] [review]
> [gaia] punamdahiya:Bug1156915 > mozilla-b2g:master
> 
> I didn't test it myself. Do you really need both direction:ltr and
> unicode-bidi: -moz-isolate?  I would have thought that just direction:ltr
> would be enough.  If you do need both, then r+ to land it this way. But if
> direction:ltr is enough, then I'd prefer that you remove the unicode-bidi
> property.

I tested and we do need both direction:ltr and unicode-bidi: -moz-isolate for the +/- signs to stay left of number.

Comment 11

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#Sg0gkcrtSlGVM697tIHR1w

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 12

[GitHub Autolander Bot]

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

Comment 13

[Punam Dahiya [:pdahiya]]

Comment on attachment 8596740 [details] [review]
[gaia] punamdahiya:Bug1156915 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Not a regression
[User impact] if declined: Exposure slider label will have +/- signs show up on right side of number in RTL mode
[Testing completed]: On master
[Risk to taking this patch] (and alternatives if risky): Very low
[String changes made]: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Sue]

Attachment: verified_v3.0_pass.png

This issue has been verified passed on latest build of Flame 3.0 with the same steps in comment 0.
Result: The number labels for the brightness slider have their positive/negative signs properly on the left side
See attachment:verified_v3.0_pass.png
Rate:0/3

Device: Flame 3.0 (pass)
Build ID               20150427160201
Gaia Revision          0636405f0844bf32451a375b2d61a2b16fe33348
Gaia Date              2015-04-27 16:42:28
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/caf25344f73e
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150427.192938
Firmware Date          Mon Apr 27 19:29:49 EDT 2015
Bootloader             L1TC000118D0

Comment 16

[Sue]

Attachment: verified_passed.png

This issue has been verified passed on latest build of Flame 2.2 and Nexus 5 2.2/3.0 with the same steps in comment 0.
Result: The number labels for the brightness slider have their positive/negative signs properly on the left side.
See attachment:verified_passed.png
Rate:0/5

Device: Flame 2.2 (pass)
Build ID               20150428002500
Gaia Revision          9f6b1b9082662ba2c14168fc66bb02b4df3141e5
Gaia Date              2015-04-27 20:41:33
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e79c19bf19bf
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150428.042318
Firmware Date          Tue Apr 28 04:23:30 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (pass)
Build ID               20150428002500
Gaia Revision          9f6b1b9082662ba2c14168fc66bb02b4df3141e5
Gaia Date              2015-04-27 20:41:33
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e79c19bf19bf
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150428.041949
Firmware Date          Tue Apr 28 04:20:07 EDT 2015
Bootloader             HHZ12f

Device: Nexus 5 3.0 (pass)
Build ID               20150428010206
Gaia Revision          0636405f0844bf32451a375b2d61a2b16fe33348
Gaia Date              2015-04-27 16:42:28
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/caf25344f73e
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150428.043749
Firmware Date          Tue Apr 28 04:38:06 EDT 2015
Bootloader             HHZ12f

Comment 17

[Sue]

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