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

VERIFIED FIXED in 2.2 S11 (1may)

Status

defect
P1
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Marty, Assigned: pdahiya)

Tracking

unspecified
2.2 S11 (1may)
ARM
Gonk (Firefox OS)
Bug Flags:
in-moztrap +

Firefox Tracking Flags

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

Details

(Whiteboard: [3.0-Daily-Testing])

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
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
QA Whiteboard: [QAnalyst-Triage?][rtl-impact]
Flags: needinfo?(pbylenga)
Whiteboard: [3.0-Daily-Testing][rtl-impact] → [3.0-Daily-Testing]
(Reporter)

Updated

4 years ago
Blocks: gallery-rtl
QA Whiteboard: [QAnalyst-Triage?][rtl-impact] → [QAnalyst-Triage+][rtl-impact]
Flags: needinfo?(pbylenga)
Triage: P1. Nominating since this isn't how it should look like in Arabic
blocking-b2g: --- → 2.2?
Priority: -- → P1

Updated

4 years ago
Assignee: nobody → pdahiya
blocking-b2g: 2.2? → 2.2+
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
(Assignee)

Comment 5

4 years ago
(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!
(Assignee)

Updated

4 years ago
Attachment #8596740 - Flags: review?(dflanagan)
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!
Attachment #8596740 - Flags: review?(dflanagan) → review-
(Assignee)

Comment 7

4 years ago
(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!
(Assignee)

Comment 8

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

Setting review flag on updated patch. Please review. Thanks!
Attachment #8596740 - Flags: review- → review?(dflanagan)
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.
Attachment #8596740 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 10

4 years ago
(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.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

4 years ago
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
Attachment #8596740 - Flags: approval-gaia-v2.2?(bbajaj)
Attachment #8596740 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+

Comment 15

4 years ago
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

Updated

4 years ago
QA Whiteboard: [QAnalyst-Triage+][rtl-impact] → [QAnalyst-Triage+][rtl-impact][MGSEI-Triage+]

Comment 16

4 years ago
Posted image 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

Updated

4 years ago
Status: RESOLVED → VERIFIED

Comment 17

4 years ago
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15916/
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.