1140583 - [RTL][Settings] ICCID info revealed in 'More Information' has inconsistent output from mixing numbers & characters/words

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P2)

Description

[Oliver Nelson [:oliverthor]]

Attachment: iccid_formatting_ltr_vs_rtl.png

Description:
When a user observes their ICCID values in Arabic language setting (with only one SIM inserted), they will observe that the text for 'Not available' (in Arabic) stretches from right to left. In all other cases when values are present, the label for the data (SIM1/SIM2) displays to the left of the value then the value correponds on the right. When not available however, it breaks the format by pushing the SIM label to the far right, with 'Not Available' on the left. This formatting looks odd for a display of data, as regardless of LTR, the user is already reading down the page to the left most of each line for the corresponding label name. 


Repro Steps:
1) Update a Flame to 20150306010207
2) Open the 'Settings' app.
3) Scroll down and tap 'Device Information'.
4) Scroll down and tap 'More Information'.
5) Observe ICCID info relating to IMEI.

Actual:
Placeholder text for values not present shift the text from displaying data (LTR) to displaying text (RTL) in an RTL language.

Expected:
Placeholder text for values not present read the same direction as if the data value was there (LTR as values follow this format).


Environmental Variables:
--------------------------------------------------
Device: Flame 3.0
Build ID: 20150306010207
Gaia: 7a91c16bfa348be8b25e09719178efa051512988
Gecko: 0189941a3fd5
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
BuildID: 20150306002519
Gaia: eb86137e247224e86d17ed1a0a133b2a318dce3c
Gecko: a04034e239fb
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
--------------------------------------------------

Repro frequency: 5/5
See attached: 
screenshot

Comment 1

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

RTL triage: P2. Pretty much sure this should be a blocker since this is inconsistent RTL behavior so nominating

Comment 2

[howie [:howie]]

Triage: blocking.

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] EragonJ:bug-1140583 > mozilla-b2g:master

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8575794 [details] [review]
[gaia] EragonJ:bug-1140583 > mozilla-b2g:master

Arthur, in this patch, I tried to introduce two new general CSS rule that can be easily reused in all situations. Can you help me review this patch ? THanks :)

Comment 5

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8575794 [details] [review]
[gaia] EragonJ:bug-1140583 > mozilla-b2g:master

Per the offline discussion, let's add new strings taking parameters instead, or the order might be incorrect in other languages. Thanks.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] EragonJ:bug-1140583-another-try > mozilla-b2g:master

Comment 7

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8575880 [details] [review]
[gaia] EragonJ:bug-1140583-another-try > mozilla-b2g:master

Hi Stas,

in this bug, we are trying to handle the problem that "IMEI 1: 123456789" would be rendered in a wrong way by Gecko. In order to fix this, I got an idea that I have to add RTL/LTR related unicodes in locales for this like what we did before. But after digging into this bug, I think this is a little bit different.

Because in order to make "IMEI 1" and "123456789" all in a right order, the first one should be wrapped as RTL while the second one should be wrapped as LTR. So, in my previous PR, I used two different <span> with pre-defined CSS rules to force them in a right order and this works ! 

But during an offline discussion with Arthur, we think this part may be able to be achieved in locales, so I made another try for it. When writing this new patch, I noticed some problems that needs to be clarified.

1. Is pseudo language abel to render "XXXX.innerHTML" string below ? I did try several times but it looks like peeudo language can't render correctly with these special codes. What I saw is the whole raw string.

deviceInfo-IMEI-with-index.innerHTML=IMEI {{index}}: &#x202b;12345&#x202c;

2. In l10n fallback mechanism, this case would fail no matter how I did here. As you may see, I can't make "IMEI 1" and "123456789" to fit this case. Do you suggest any way to make the fallback work ? or we can just drop rare case ?

If there is anything I understand wrongly, please tell me and I really appreciate it !

Thanks Stas :)

Comment 8

[Staś Małolepszy :stas]

Comment on attachment 8575880 [details] [review]
[gaia] EragonJ:bug-1140583-another-try > mozilla-b2g:master

Hi EJ,  sorry for the wait, I'm still working through the backlog after coming back from vacation.

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #7)

> 1. Is pseudo language abel to render "XXXX.innerHTML" string below ? I did
> try several times but it looks like peeudo language can't render correctly
> with these special codes. What I saw is the whole raw string.
> 
> deviceInfo-IMEI-with-index.innerHTML=IMEI {{index}}: ‫12345‬

Yes, that's a known limitation of pseudolocales which I'll fix in bug 1137094.

> 2. In l10n fallback mechanism, this case would fail no matter how I did
> here. As you may see, I can't make "IMEI 1" and "123456789" to fit this
> case. Do you suggest any way to make the fallback work ? or we can just drop
> rare case ?

We shouldn't force the IMEI and the SIM number to be LTR.  It's valid for numbers to be RTL.

IIUC, the way this should look like for RTL languages is this:

           1234 :SIM 1
  elbaliava toN :SIM 2

The reason why the first case looks different in the screenshot is that all digits and the colon are weak characters [1] and they take the directionality of the text around them, i.e. "SIM".  So the whole thing ends up being LTR:  "SIM 1: 1234".

The way to work around this is to use <bdi>.  As you mention, you can't really use LRE/PDF because what you care about is the English string coming from the fallback mechanism, and it only actually needs the LRE/PDF marks if it's used as fallback, but not when it's used as the first language!

<bdi> is handy because all it does is isolate (hence the "i" in the name) some characters from the surrounding text which makes it inherit the direction of the parent.  It doesn't require you to hardcode any direction explicitly and this makes it a good choice for the fallback scenario.
  
  deviceInfo-ICCID-with-index.innerHTM = SIM 1: <bdi>1234</bdi>

Check out the pen at [2] for examples of this at work.

HTH!

[1] http://en.wikipedia.org/wiki/Bi-directional_text#Unicode_support
[2] http://codepen.io/stasm/pen/VYEXBZ?editors=100

Comment 9

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Staś Małolepszy :stas from comment #8)
> Comment on attachment 8575880 [details] [review]
> [gaia] EragonJ:bug-1140583-another-try > mozilla-b2g:master
> 
> Hi EJ,  sorry for the wait, I'm still working through the backlog after
> coming back from vacation.
> 
> (In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #7)
> 
> > 1. Is pseudo language abel to render "XXXX.innerHTML" string below ? I did
> > try several times but it looks like peeudo language can't render correctly
> > with these special codes. What I saw is the whole raw string.
> > 
> > deviceInfo-IMEI-with-index.innerHTML=IMEI {{index}}: ‫12345‬
> 
> Yes, that's a known limitation of pseudolocales which I'll fix in bug
> 1137094.

Okay, at least this is a known issue because I keep trying to see whether I broke something here on my local.

> > 2. In l10n fallback mechanism, this case would fail no matter how I did
> > here. As you may see, I can't make "IMEI 1" and "123456789" to fit this
> > case. Do you suggest any way to make the fallback work ? or we can just drop
> > rare case ?
> 
> We shouldn't force the IMEI and the SIM number to be LTR.  It's valid for
> numbers to be RTL.
> 
> IIUC, the way this should look like for RTL languages is this:
> 
>            1234 :SIM 1
>   elbaliava toN :SIM 2
> 
> The reason why the first case looks different in the screenshot is that all
> digits and the colon are weak characters [1] and they take the
> directionality of the text around them, i.e. "SIM".  So the whole thing ends
> up being LTR:  "SIM 1: 1234".
> 
> The way to work around this is to use <bdi>.  As you mention, you can't
> really use LRE/PDF because what you care about is the English string coming
> from the fallback mechanism, and it only actually needs the LRE/PDF marks if
> it's used as fallback, but not when it's used as the first language!
> 
> <bdi> is handy because all it does is isolate (hence the "i" in the name)
> some characters from the surrounding text which makes it inherit the
> direction of the parent.  It doesn't require you to hardcode any direction
> explicitly and this makes it a good choice for the fallback scenario.
>   
>   deviceInfo-ICCID-with-index.innerHTM = SIM 1: <bdi>1234</bdi>
> 

Thanks ! After reading that wiki page, I finally understand more about Unicode Bidirectional Algorithm. With weak & strong role, I think this may be the root cause why () would break before. In order to make sure I understand this correctly, I also made another codepen for this part and please check my comment in HTML part : 

http://codepen.io/anon/pen/XJxxNK

And as what you said about <bdi>, I almost forgot that's the reason why we need this kind of tag to make sure its direction can be isolated and be decided solely by browser. By doing so, I don't have to specific hack the direction for these two parts and the fallback mechanism should work.

I just updated the PR and use <bdi> for this special case, any idea for it :) ?

Comment 10

[Staś Małolepszy :stas]

Comment on attachment 8575880 [details] [review]
[gaia] EragonJ:bug-1140583-another-try > mozilla-b2g:master

This looks great, thanks EJ!  And yes, your codepen demonstrates the Bidi mechnic very well.  I added one more example to it to include the trick with LRE/PDF we did in the "WPA (AES)" bug:

http://codepen.io/stasm/pen/WbaaOV?editors=100

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8575880 [details] [review]
[gaia] EragonJ:bug-1140583-another-try > mozilla-b2g:master

Arthur, based on offline discussion with you, can you help me review this patch ? I already got f+ from Stas for it. Hope the comments above can help you when reviewing.

Thanks :)

Comment 12

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8575880 [details] [review]
[gaia] EragonJ:bug-1140583-another-try > mozilla-b2g:master

r=me with the comment addressed, thanks!

Comment 13

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks all, this patch is merged at Gaia/master : https://github.com/mozilla-b2g/gaia/commit/daf32758b3ba8300d711235e8f0caad0801b6945

Comment 14

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8575880 [details] [review]
[gaia] EragonJ:bug-1140583-another-try > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): no
[User impact] if declined: the direction of numbers & strings would be wrong in RTL languages.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low 
[String changes made]: yes

Comment 15

[Sue]

Attachment: moreinfo.png

This issue has been verified successfully on Flame 3.0, the IMEI and ICCID for RTL languages shown as:
1234 :IMEI 1
1234 :IMEI 2
1234 :SIM 1
1234 :SIM 3
See attachment:moreinfo.png
Rate:0/5

Flame 3.0 build: pass
Build ID               20150317160205
Gaia Revision          63d6639acd771f548a2613f07f3e335921e4ac87
Gaia Date              2015-03-17 16:53:50
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/e965a1a534ec
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150317.195927
Firmware Date          Tue Mar 17 19:59:39 EDT 2015
Bootloader             L1TC000118D0

Comment 16

[Sue]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

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

Please remember to add the late-l10n keyword when you land patches on 2.2 with string changes! (too late now)
late-l10n keyword use started at FL...thanks!

Comment 19

[Lancy(Leave from Mozilla)]

Attachment: Verify2_Flame2.2_Pass.png

This issue has been verified successfully on latest Flame 2.2 build.
See attachment: Verify2_Flame2.2_Pass.png
Reproducing rate: 0/10

Flame 2.2 build: 
Build ID               20150322002503
Gaia Revision          44c62060581fde8de1e12e94cf55e9673b401a47
Gaia Date              2015-03-20 19:05:17
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e6140a32902a
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150322.043216
Firmware Date          Sun Mar 22 04:32:27 EDT 2015
Bootloader             L1TC000118D0