1136859 - [RTL][FTE] Internet Connection error message for Privacy Policy has an improperly formatted Firefox OS URL

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect, P1)

Description

[Martin Shuman [:Marty]]

Attachment: Error-Message-URL-Screenshot.png

Description:
If the user attempts to access the Firefox OS privacy policy without an internet connection, they are presented with an error message that displays the URL with the privacy policy location.
This URL has a line break in it, and in RTL, the second line has a slash ('/') character misplaced.

The second line of the URL appears like so: 
"//firefox-os"

Repro Steps:
1) Update a Flame to 20150225010244
2) In the FTE, change the language to Arabic ()
3) Do not connect to a WiFi or Data network
4) Progress to the About Firefox OS page in the FTE and open the Privacy Policy

Actual:
The resultant error message has an improperly formatted URL.

Expected:
The URL is formatted properly in the error message.

Environmental Variables:
Device: Flame 3.0
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

Repro frequency: 5/5
See attached: screenshot

Comment 1

[Martin Shuman [:Marty]]

This issue DOES occur in Flame 2.2
The error message has an improperly formatted URL.

Environmental Variables:
Device: Flame 2.2
Build ID: 20150223002503
Gaia: 389542b71c89253c0d176d3b0bfb54e275c19bf1
Gecko: 9fd3441c8983
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

[Sam Foster [:sfoster] (he/him)]

Using the mirrored english pseudo-locale, the markup is dumped out to the screen so I'm struggling to diagnose this. I thought we had this worked out using the best practices for localized content with embedded links. Does the markup & code look ok Zibi?

https://github.com/mozilla-b2g/gaia/blob/master/apps/ftu/js/navigation.js#L237,L243

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: ftu-about-b2g-mirrored.png

Screenshot using mirrored-english. Markup is getting entities escaped so they render as-is (<>& etc.)

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Hah, it's an interesting bug. It seems that pseudol10n is a bit too aggressive with HTML tags.

I'll keep debugging this bug, but NI'ing stas for this:

 - open FTE
 - switch to pseudolocale
 - go to About Firefox OS

Current result:
all HTML tags are displayed as text and pseudolocalized

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

CC'ing Ahmed.

It seems that something is weird with Gecko RTL. The Gaia code is good here.

I copied arabic string and created minimized testcase.

STR:

1) Launch http://jsbin.com/lalarutexi/1/edit?html,console,output
2) Resize the output panel to be ~300px
3) Notice how the link is broken

Ahmed, what's the solution here?

Comment 6

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf] from comment #4)

> Current result:
> all HTML tags are displayed as text and pseudolocalized

Yea, it's a known limitation that I have been meaning to fix in a while but was concerned about the performance.  Now that we landed bug 1101632, I can finally fix this the proper way.  I filed bug 1137094.

Comment 7

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf] from comment #5)

> Ahmed, what's the solution here?

It's not a bug in Gecko but rather the consequence of how the bidi algorithm handles the slash, which is a weak character meaning it doesn't have its own directionality property and instead inherits from characters around it.

http://en.wikipedia.org/wiki/Bi-directional_text#Unicode_support

This is similar to bug 1102158 comment 6 and 1106835 comment 11.  The solution is to either use <bdi> or <span dir="ltr"> combined with innerHTML in the properties file.

http://jsbin.com/yasicoxice/2/edit
http://jsbin.com/yasicoxice/3/edit

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screen Shot 2015-02-26 at 12.55.11 AM.png

So, with your jsbin, I still see url broken (both with span and bdi). Is that the correct behavior?

Comment 9

[Staś Małolepszy :stas]

What do you mean by broken?  The final slash is now correctly placed at the end of the URL.  Or do you mean the way it's broken into two lines?  I think that's how it should be, I don't see any other way this could work.

Comment 10

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

Is this in any way related to the issue filed in Bug 1137021?
Also, changing the flag to Kaze as Ahmed will be focusing on 2.0M now. thanks!

Comment 11

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

Broken URL means broken means this should be a P1. Also, nominating

Comment 12

[Ted Clancy [:tedders1]]

Even though this ticket was assigned to "nobody", it looks like :stas already has a solution to this and it just needs to be landed.

Stas, can you land this? It is blocking B2G 2.2.

Comment 13

[Staś Małolepszy :stas]

I don't think I should be the person fixing this, but since it's blocking 2.2, let's not let it wait any longer.  I'll attach a pull request shortly which has a much simpler fix than the one I suggested in comment 7.

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] stasm:1136859-trailing-slash > mozilla-b2g:master

Comment 15

[Staś Małolepszy :stas]

Comment on attachment 8575864 [details] [review]
[gaia] stasm:1136859-trailing-slash > mozilla-b2g:master

Sam, it's a simple workaround that seems acceptable in this case of the LTR text always being a URL.

Comment 16

[Sam Foster [:sfoster] (he/him)]

Yeah I get it, we could also drop a span or something with direction: ltr in there, right? Either way, I do want to get through the exercise of seeing this running localized in Arabic though, which I've not done yet so apologies for the delay in review.

Comment 17

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8575864 [details] [review]
[gaia] stasm:1136859-trailing-slash > mozilla-b2g:master

Stellar workaround :) LGTM.

Comment 18

[Sam Foster [:sfoster] (he/him)]

Autolander do your thing plz.

Comment 19

[GitHub Autolander Bot]

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

Comment 20

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8575864 [details] [review]
[gaia] stasm:1136859-trailing-slash > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): FTE / RTL
[User impact] if declined: The URL reported in connection error screen when trying to load the privacy policy is garbled
[Testing completed]: Testing on device w. Arabic locale, green Gaia-Try
[Risk to taking this patch] (and alternatives if risky): Trivial 1-character patch, removing the trailing '/' on the privacy-policy URL ensures it displays correctly in error screens
[String changes made]: None (this URL is hardcoded in FTU index.html)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue still reproduces on the latest Nightly Flame 3.0 and 2.2 builds as written in the description.

Actual Results: The url is still split up with text in between when viewed in RTL languages.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150313010238
Gaia: eabe35cf054d47087b37c1ca7db8143717fbd7f3
Gecko: 42afc7ef5ccb
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20150313002507
Gaia: 4aefc3f6f30a40ac67fdf841b7c90cd648b85369
Gecko: 049713f3b0ed
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 23

[Sam Foster [:sfoster] (he/him)]

(In reply to Jayme Mercado [:JMercado] from comment #22)
> This issue still reproduces on the latest Nightly Flame 3.0 and 2.2 builds
> as written in the description.
> 
> Actual Results: The url is still split up with text in between when viewed
> in RTL languages.

Could you add a screenshot of what you see? I know I tested this on master with Arabic locale, and with the trailing slash removed the URL looked correct to me at that time.

Comment 24

[Sue]

Attachment: error_url.png

This bug has been verified fail on latest build of Flame 2.2/3.0, the second line of the URL appears like: "//firefox-os" and it should be "/firefox-os/"
See attachment:error_url.png
Rate:3/3

Flame 2.2 build: fail
Build ID               20150315162500
Gaia Revision          a6b2d3f8478ec250beb49950fecbb8a16465ff6f
Gaia Date              2015-03-15 14:33:22
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/18619f8f6c5c
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150315.195030
Firmware Date          Sun Mar 15 19:50:42 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 build: fail
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

Comment 25

[Sue]

Hi Sam,
I have uploaded the screenshot, could you help with it? Thanks!

Comment 26

[Sam Foster [:sfoster] (he/him)]

OK looks like we need to try again with this.

Comment 27

[Ted Clancy [:tedders1]]

Hey Sam, is it okay if I take this one?

It was assigned to me last week. The only reason I didn't work on it then is because :stas already had a patch.

I think I know what to do to make this work.

Comment 28

[Sam Foster [:sfoster] (he/him)]

(In reply to Ted Clancy [:tedders1] from comment #27)
> Hey Sam, is it okay if I take this one?

Sure, go for it.

Comment 29

[GitHub Autolander Bot]

Attachment: [gaia] tedders1:bidi-substitution-fix > mozilla-b2g:master

Comment 30

[GitHub Autolander Bot]

Attachment: [gaia] tedders1:bidi-substitution-fix-1 > mozilla-b2g:master

Comment 31

[GitHub Autolander Bot]

Attachment: [gaia] tedders1:bidi-substitution-fix-1 > mozilla-b2g:master

Comment 32

[GitHub Autolander Bot]

Attachment: [gaia] tedders1:bidi-substitution-fix-2 > mozilla-b2g:master

Comment 33

[Ted Clancy [:tedders1]]

Okay, the solution here is that the URL needs to be surrounded by Unicode bidi isolation characters. (Specifically, FSI U+2068 and PDI U+2069 are the ones we want.) 

Now, there are different ways we could achieve that. I could easily write a patch that changes the string in the Arabic version of ftu.properties to contain "\u2068{{ url }}\u2069" instead of just "{{ url }}".

But that would be the wrong approach, and we'd keep encountering this problem again and again in other places. 

You see, this problem occurs whenever a substituted string might have a different directionality from the surrounding string. It happens in this case with a LTR URL being substituted into an RTL Arabic sentence, but the same problem could occur almost anywhere. 

Consider an English sentence like "Hello {{name}}, you have new mail". That won't display properly if the user's name is in Hebrew, unless you insert Unicode bidi isolation characters.

I'm of the opinion that the Unicode bidi isolation characters should be inserted automatically by the substitution system (navigator.mozL10n). I don't think we should require the programmer or translators to insert them manually, since (a) they'd have to be put almost everywhere, (b) most programmers and translators wouldn't know what they are, and (c) even the programmers who do know will forget. I think the traditional view has been that the Arabic/Hebrew/Farsi/Urdu/Yiddish translators should take of this themselves, but as the example in the previous paragraph shows, this isn't just a problem when the UI is in an RTL language; it's a problem in any Unicode environment.

Now, it's easy to get mozL10n to insert Unicode bidi isolation characters automatically, but there's a catch: Sometimes you *don't* want the Unicode bidi isolation characters. This happens when the resulting string is intended for a machine rather than a human. For example, if you're constructing a URL, you don't want a the bidi isolation characters.

So we need a way to tell mozL10n when the substitution is intended for a machine or a human. Luckily, I have a patch that adds that functionality! But, I'm going to put it into a different Bugzilla ticket.

Comment 34

[GitHub Autolander Bot]

Attachment: [gaia] tedders1:bidi-substitution-fix-2 > mozilla-b2g:master

Comment 35

[Ted Clancy [:tedders1]]

Comment on attachment 8579380 [details] [review]
[gaia] tedders1:bidi-substitution-fix-2 > mozilla-b2g:master

Autolander, stop doing that.

Comment 36

[Ted Clancy [:tedders1]]

Okay, this bug will eventually be taken care of by Bug 1144682, which implements what I was talking about in Comment 33.

Comment 37

[Staś Małolepszy :stas]

Where's this slash coming from if it was removed in my commit?

Comment 38

[Ted Clancy [:tedders1]]

(In reply to Staś Małolepszy :stas from comment #37)
> Where's this slash coming from if it was removed in my commit?

The string that needed to be changed is in ftu/js/external_links.js.

Comment 39

[Staś Małolepszy :stas]

Ah, I see.  I figured FTU was getting the URL from the HTML [1] but I didn't realize those URL were first inserted into the HTML by JS using ftu/js/external_links.js.

[1] https://github.com/mozilla-b2g/gaia/blob/3e3989820c47e611b81e4ed9a6d4b5a6912bf8db/apps/ftu/js/navigation.js#L140-L155

Comment 40

[GitHub Autolander Bot]

Attachment: [gaia] stasm:1136859-trailing-slash-2 > mozilla-b2g:master

Comment 41

[Lancy(Leave from Mozilla)]

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

Comment 42

[Ted Clancy [:tedders1]]

Comment on attachment 8580572 [details] [review]
[gaia] stasm:1136859-trailing-slash-2 > mozilla-b2g:master

Hey Staś. I think you left the r=sfoster out of the commit message, so Autolander didn't flag this for review.

I'll flag it for you.

Comment 43

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8580572 [details] [review]
[gaia] stasm:1136859-trailing-slash-2 > mozilla-b2g:master

Ship it!

Comment 44

[Ted Clancy [:tedders1]]

Okay, I'm going to see if I can land this for Staś.

Comment 45

[GitHub Autolander Bot]

Attachment: [gaia] tedders1:bug-1136859-trailing-slash-2 > mozilla-b2g:master

Comment 46

[GitHub Autolander Bot]

Attachment: [gaia] stasm:1136859-trailing-slash-2 > mozilla-b2g:master

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/29d89416fa808ddd45b27509cbbb8ad2cfef00d9

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Please request Gaia v2.2 approval on this patch when you get a chance.

Comment 49

[Sue]

Attachment: URL_in_error_message.png

Hi Ted,
I have tested this issue on latest build of Flame 3.0 with the same steps in comment 0. The second line of the URL appears like: "/firefox-os". Could you help to confirm whether it is right or not? Thanks!
See attachment:URL_in_error_message.png

Comment 50

[Ted Clancy [:tedders1]]

(In reply to Sue from comment #49)
> Created attachment 8584934 [details]
> URL_in_error_message.png
> 
> Hi Ted,
> I have tested this issue on latest build of Flame 3.0 with the same steps in
> comment 0. The second line of the URL appears like: "/firefox-os". Could you
> help to confirm whether it is right or not? Thanks!
> See attachment:URL_in_error_message.png

Hi Sue,

Yes, that's fine. The URL is too long to fit on one line, so it's broken across two lines. That's not a bug.

Comment 51

[Ted Clancy [:tedders1]]

Comment on attachment 8582743 [details] [review]
[gaia] tedders1:bug-1136859-trailing-slash-2 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): FTE / RTL
[User impact] if declined: The URL reported in connection error screen when trying to load the privacy policy is garbled
[Testing completed]: Testing on device w. Arabic locale, green Gaia-Try
[Risk to taking this patch] (and alternatives if risky): Trivial 1-character patch, removing the trailing '/' on the privacy-policy URL ensures it displays correctly in error screens
[String changes made]: None (this URL is hardcoded in FTU index.html)

Comment 52

[Sue]

(In reply to Ted Clancy [:tedders1] from comment #50)
> Hi Sue,
> 
> Yes, that's fine. The URL is too long to fit on one line, so it's broken
> across two lines. That's not a bug.

Thanks Ted.
Per comment 49 and comment 50, this issue has been verified pass on Flame 3.0

Comment 53

[Ryan VanderMeulen [:RyanVM]]

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

Comment 54

[Sue]

Attachment: URL_v2.2_verify.png

This bug has been verified pass on latest build of Flame 2.2 with same steps in comment 0, the second line of the URL appears like: "/firefox-os"
See attachment:URL_v2.2_verify.png
Rate:0/3

Device: Flame 2.2 (pass)
Build ID               20150402002500
Gaia Revision          1ceca464053dee4a8bf10ea5abeef724d68c2ff2
Gaia Date              2015-04-01 09:49:30
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/427b4da96714
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150402.035057
Firmware Date          Thu Apr  2 03:51:09 EDT 2015
Bootloader             L1TC000118D0