Closed Bug 1136859 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

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

VERIFIED FIXED
2.2 S9 (3apr)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: Marty, Assigned: tedders1)

References

Details

(Whiteboard: [3.0-Daily-Testing][systemsfe])

Attachments

(8 files, 7 obsolete files)

97.12 KB, image/png
Details
50.19 KB, image/png
Details
17.98 KB, image/png
Details
38.98 KB, image/png
Details
46 bytes, text/x-github-pull-request
Details | Review
46 bytes, text/x-github-pull-request
Details | Review
40.93 KB, image/png
Details
40.50 KB, image/png
Details
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
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
QA Whiteboard: [QAnalyst-Triage?][rtl-impact]
Flags: needinfo?(pbylenga)
QA Whiteboard: [QAnalyst-Triage?][rtl-impact] → [QAnalyst-Triage+][rtl-impact]
Flags: needinfo?(pbylenga)
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
Flags: needinfo?(gandalf)
Screenshot using mirrored-english. Markup is getting entities escaped so they render as-is (<>& etc.)
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
Flags: needinfo?(gandalf) → needinfo?(stas)
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?
Flags: needinfo?(nefzaoui)
(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.
Flags: needinfo?(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
So, with your jsbin, I still see url broken (both with span and bdi). Is that the correct behavior?
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.
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!
Flags: needinfo?(nefzaoui) → needinfo?(fabien)
Broken URL means broken means this should be a P1. Also, nominating
blocking-b2g: --- → 2.2?
Priority: -- → P1
Whiteboard: [3.0-Daily-Testing] → [3.0-Daily-Testing][systemsfe]
blocking-b2g: 2.2? → 2.2+
Assignee: nobody → tclancy
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.
Flags: needinfo?(stas)
Status: NEW → ASSIGNED
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.
Status: ASSIGNED → NEW
Flags: needinfo?(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.
Attachment #8575864 - Flags: review?(sfoster)
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 on attachment 8575864 [details] [review]
[gaia] stasm:1136859-trailing-slash > mozilla-b2g:master

Stellar workaround :) LGTM.
Attachment #8575864 - Flags: review?(sfoster) → review+
Autolander do your thing plz.
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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)
Attachment #8575864 - Flags: approval-gaia-v2.2?
Attachment #8575864 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Target Milestone: --- → 2.2 S8 (20mar)
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
QA Whiteboard: [QAnalyst-Triage+][rtl-impact] → [QAnalyst-Triage?][rtl-impact][failed-verification]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][rtl-impact][failed-verification] → [QAnalyst-Triage+][rtl-impact][failed-verification]
Flags: needinfo?(ktucker)
(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.
Flags: needinfo?(jmercado)
Attached image 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
Flags: needinfo?(jmercado)
QA Whiteboard: [QAnalyst-Triage+][rtl-impact][failed-verification] → [QAnalyst-Triage+][rtl-impact][failed-verification][MGSEI-Triage+]
Hi Sam,
I have uploaded the screenshot, could you help with it? Thanks!
Flags: needinfo?(sfoster)
OK looks like we need to try again with this.
Assignee: tclancy → sfoster
Status: RESOLVED → REOPENED
Flags: needinfo?(sfoster)
Resolution: FIXED → ---
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.
(In reply to Ted Clancy [:tedders1] from comment #27)
> Hey Sam, is it okay if I take this one?

Sure, go for it.
Assignee: sfoster → tclancy
Attachment #8578716 - Attachment is obsolete: true
Attachment #8578734 - Attachment is obsolete: true
Attachment #8578996 - Attachment is obsolete: true
Attachment #8579307 - Attachment is obsolete: true
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.
Blocks: 1144682
No longer blocks: 1144682
Depends on: 1144682
Comment on attachment 8579380 [details] [review]
[gaia] tedders1:bidi-substitution-fix-2 > mozilla-b2g:master

Autolander, stop doing that.
Attachment #8579380 - Attachment is obsolete: true
Okay, this bug will eventually be taken care of by Bug 1144682, which implements what I was talking about in Comment 33.
Where's this slash coming from if it was removed in my commit?
(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.
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
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/16082/
Flags: in-moztrap+
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.
Attachment #8580572 - Flags: review?(sfoster)
Comment on attachment 8580572 [details] [review]
[gaia] stasm:1136859-trailing-slash-2 > mozilla-b2g:master

Ship it!
Attachment #8580572 - Flags: review?(sfoster) → review+
Attachment #8575864 - Attachment is obsolete: true
Okay, I'm going to see if I can land this for Staś.
Attachment #8580572 - Attachment is obsolete: true
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/29d89416fa808ddd45b27509cbbb8ad2cfef00d9
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: 2.2 S8 (20mar) → 2.2 S9 (3apr)
Please request Gaia v2.2 approval on this patch when you get a chance.
Flags: needinfo?(tclancy)
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
Attachment #8575864 - Flags: approval-gaia-v2.2+
(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 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)
Flags: needinfo?(tclancy)
Attachment #8582743 - Flags: approval-gaia-v2.2?(bbajaj)
(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
Attachment #8582743 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
Attached image 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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+][rtl-impact][failed-verification][MGSEI-Triage+] → [QAnalyst-Triage+][rtl-impact][MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.