1160080 - [RTL][Browser]The format of the website address is shown wrongly in "Serve not found" view.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::System UI, defect, P2)

Description

[Lancy(Leave from Mozilla)]

Attachment: Screenshot_Website address.png

[1.Description]:
[RTL][Flame v2.2 & v3.0][Nexus 5 2.2 & 3.0][Browser]Launc browser and browse the wrong webste, the format of the website address is shown wrong in "Serve not found" view.
See attachment:Screenshot_Website address.png

[2.Testing Steps]: 
1.Set your phone language to Arabic and connect network.
2.Launch Browser.
3.Input the wrong website address and tap search button.

[3.Expected Result]: 
3.The format of the website address should be shown correctly in "Serve not found" view.

[4.Actual Result]: 
3.The format of the website address is shown wrongly in "Serve not found" view.

[5.Reproduction build]: 
Device: Flame 2.2 (affected)
Build ID               20150429002501
Gaia Revision          1b7aa7e60788668ed09abf76022dfa231dbe88d4
Gaia Date              2015-04-28 19:36:06
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d38ff4717f39
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150429.035717
Firmware Date          Wed Apr 29 03:57:29 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 (affected)
Build ID               20150429010205
Gaia Revision          6e35b0948c42a4398b8a5916015de167121683a1
Gaia Date              2015-04-28 16:06:07
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/1ad65cbeb2f4
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150429.043837
Firmware Date          Wed Apr 29 04:38:48 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (affected)
Build ID               20150429002501
Gaia Revision          1b7aa7e60788668ed09abf76022dfa231dbe88d4
Gaia Date              2015-04-28 19:36:06
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d38ff4717f39
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150429.040217
Firmware Date          Wed Apr 29 04:02:34 EDT 2015
Bootloader             HHZ12f

Device: Nexus 5 3.0 (affected)
Build ID               20150429010205
Gaia Revision          6e35b0948c42a4398b8a5916015de167121683a1
Gaia Date              2015-04-28 16:06:07
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/1ad65cbeb2f4
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150429.044325
Firmware Date          Wed Apr 29 04:43:46 EDT 2015
Bootloader             HHZ12f

[6.Reproduction Frequency]: 
Always Recurrence,10/10

[7.TCID]: 
Free Test

Comment 1

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

Triage: P2;
Putting needinfo on me to investigate this further

Comment 2

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

Thought that this was a regression, but can still repro up until March 26 (haven't tried earlier).
This looks pretty bad, can we still get a fix in 2.2?
(Lancy: can you please make sure this was covered by a test case and confirm?)

Comment 3

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

Seems similar to Bug 1150825

Comment 4

[Lancy(Leave from Mozilla)]

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

Comment 5

[Josh Cheng [:josh]]

Hi Ben,
Could you please help to check the problem? Thanks!

Comment 6

[Josh Cheng [:josh]]

Dear Gregor,
Could you please help to find someone who can help on this bug? 
Thanks!

Comment 7

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

We may need to add the left-to-right mark where we create this string

Comment 8

[Ted Clancy [:tedders1]]

Hi Sam. This might be fixed by Bug 1154438 which I am just about to land. I'll let you know.

Comment 9

[Ted Clancy [:tedders1]]

Actually, on second thoughts, I'm not confident of getting Bug 1154438 approved for uplift. (It depends on some problematic things.)

You might be better off using the left-to-right mark for now.

Comment 10

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

This is the server-not-found-error string in apps/system/net_error.properties: 

  server-not-found-error=لم يتم العثور على الخادم في {{name}}.

Options seem to be to either add the LTR mark in the necessary string properties files, or perhaps better, to add it as a prefix in https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/net_error.js#L261. But right now I'm unable to get any of these options to work - as it seems both '\u200E' or '‎' get stripped back out, escaped or ignored. Am I doing it wrong or do you have other suggestions?

Comment 11

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

So this works, but after the discussion in bug 1144682 and bug 1154438 I'm not sure its fixing it in the right place? 

--- a/apps/system/locales/net_error.en-US.properties
+++ b/apps/system/locales/net_error.en-US.properties
@@ -1,7 +1,7 @@
 network-error-in-app=No access to the Internet right now.
 network-error-launching={{name}} requires an Internet connection.
 server-not-found=Server not found
-server-not-found-error=Can’t find the server at {{name}}.
+server-not-found-error=Can’t find the server at <bdi>{{name}}</bdi>.
 file-not-found=File not found
 file-not-found-error=The device can’t display this page because the file cannot
 connection-failed=Unable to connect

Also, clearing ni? for Ben

Comment 12

[Francesco Lodolo [:flod]]

(In reply to Sam Foster [:sfoster] from comment #11)
> So this works, but after the discussion in bug 1144682 and bug 1154438 I'm
> not sure its fixing it in the right place? 

We're 2 weeks past FC, almost done with l10n QA testing, and string freeze (which was already extended from the original date), so I'd say you should not add new strings. 

But, as usual, that's not l10n's call, that's release-driver's choice to make.

Comment 13

[Staś Małolepszy :stas]

(In reply to Sam Foster [:sfoster] from comment #11)

> -server-not-found-error=Can’t find the server at {{name}}.
> +server-not-found-error=Can’t find the server at <bdi>{{name}}</bdi>.

This would be my preferred solution if we weren't past the string freeze.  Keep in mind that you also need to change the string id (server-not-found-error2).

On master, Ted's bug 1154438 will solve this.  On 2.2, in order to avoid breaking the string freeze, we might want to change the JS.  Sam, are you saying that this didn't work?

  localizeElement(this.message, this.messageText, { name: '\u2068' + this.error.u + '\u2069'});

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] sfoster:net-error-bidi-bug-1160080 > mozilla-b2g:master

Comment 15

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

(In reply to Staś Małolepszy :stas from comment #13)

> On master, Ted's bug 1154438 will solve this.  On 2.2, in order to avoid
> breaking the string freeze, we might want to change the JS.  Sam, are you
> saying that this didn't work?
> 
>   localizeElement(this.message, this.messageText, { name: '\u2068' +
> this.error.u + '\u2069'});

I'm still seeing this issue on master, so I don't think bug 1154438 covers this adequately. For my patch, I guess that was one permutation I didn't try :) Works great, I'll get the patch in for review.

Comment 16

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

Comment on attachment 8605943 [details] [review]
[gaia] sfoster:net-error-bidi-bug-1160080 > mozilla-b2g:master

Fabrice, you seem to have done a lot of the reviews in this file, would you mind taking a look at this - or forwarding as necessary? 

We don't have a good way to test these bidi issues currently, so I'm not adding tests in the patch. Testing manually - use a RTL language like Arabic and attempt to load from a non-existent domain in the browser - this does the right thing. Its a workaround however so I'll file a follow-up to either fix the strings on master, or do this in the l10n library.

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 8605943 [details] [review]
[gaia] sfoster:net-error-bidi-bug-1160080 > mozilla-b2g:master

Yay, I learned things about bidi!

Comment 18

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

Ted, this patch is a workaround that we can uplift, but I want to file a follow-up - should this have been fixed by bug 1160080?

Comment 19

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 20

[Ted Clancy [:tedders1]]

Hey Sam. 

Yeah, Bug 1154438 should have fixed this.

Are you using a recent version of gecko from master too? Bug 1154438 relies on some gecko stuff which has landed on master but not on the B2G v2.2 branch.

Comment 21

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

Merged to master: https://github.com/mozilla-b2g/gaia/commit/27f6760ceb0c8897bd21461e8b91136d7efff2a1

Comment 22

[Hermes Cheng (inactive after July 27, 2015)]

@Sam, could you help uplift to 2.2 since it is a blocker?

Comment 23

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

Comment on attachment 8605943 [details] [review]
[gaia] sfoster:net-error-bidi-bug-1160080 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Bidi content in net error pages
[User impact] if declined: URLs in RTL locale network error pages may be truncated oddly and the trailing slash misplaced
[Testing completed]: Tested on device in multiple languages
[Risk to taking this patch] (and alternatives if risky): V. low, patch just wraps unicode isolation characters around the string
[String changes made]: None, change made in the js.

Comment 24

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

(In reply to Ted Clancy [:tedders1] from comment #20)
> Hey Sam. 
> 
> Yeah, Bug 1154438 should have fixed this.

Wait, I'm concerned. If bug 1154438 should have fixed this on master, and it doesn't work, shouldn't we investigate instead of landing this patch here?

Comment 25

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

> Wait, I'm concerned. If bug 1154438 should have fixed this on master, and it
> doesn't work, shouldn't we investigate instead of landing this patch here?

I've filed bug 1165443 to remove the workaround on master. Bug 1154438 had not fixed it for me, but I can't be 100% sure which gecko I was testing with at the time. I (or Ted?) can test again without this patch and put a PR on bug 1165443 if it looks good.

Comment 26

[Ted Clancy [:tedders1]]

(In reply to Sam Foster [:sfoster] from comment #25)
> I (or Ted?) can test again without this patch

Just tested it. It works fine for me with just the patch for Bug 1165443 (and without the workaround patch Sam just made).

I think we can go ahead with Bug 1165443.

Comment 27

[Hermes Cheng (inactive after July 27, 2015)]

(In reply to Ted Clancy [:tedders1] from comment #26)
> (In reply to Sam Foster [:sfoster] from comment #25)
> > I (or Ted?) can test again without this patch
> 
> Just tested it. It works fine for me with just the patch for Bug 1165443
> (and without the workaround patch Sam just made).
> 
> I think we can go ahead with Bug 1165443.

Did you mean that we have to wait until bug 1165443 is resolved and uplift to v2.2?

Comment 28

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

> Did you mean that we have to wait until bug 1165443 is resolved and uplift
> to v2.2?

No, attachment #8605943 [details] [review] should get uplifted as-is - it is not needed on master but *is* currently needed on v2.2. We're still waiting for uplift approval but should have that in an ready for test this week.

Comment 29

[Josh Cheng [:josh]]

Comment on attachment 8605943 [details] [review]
[gaia] sfoster:net-error-bidi-bug-1160080 > mozilla-b2g:master

Lancy,
Please verify on master and 2.2 after this patch land there.
thanks

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Sue]

Attachment: verify_v3.0_pass.png

This issue has been verified as pass on latest nightly build of Flame 3.0 and Nexus 5 3.0 by STRs in comment 0.
Result:The format of the website address is shown correctly.
See attachment:verify_v3.0_pass.png
Rate:0/5

Device: Flame 3.0 (pass)
Build ID               20150518010206
Gaia Revision          afea16de7a76c3b6d15c35fb4c37bac71c8ddc6a
Gaia Date              2015-05-17 03:33:40
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/35918b0441b4
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150518.042019
Firmware Date          Mon May 18 04:20:30 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 3.0 (pass)
Build ID               20150518010206
Gaia Revision          afea16de7a76c3b6d15c35fb4c37bac71c8ddc6a
Gaia Date              2015-05-17 03:33:40
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/35918b0441b4
Gecko Version          41.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150518.043911
Firmware Date          Mon May 18 04:39:27 EDT 2015
Bootloader             HHZ12f

Comment 32

[Sue]

Attachment: verify_v2.2_pass.png

This issue has been verified as pass on latest nightly build of Flame 2.2 and Nexus 5 2.2 by STRs in comment 0.
Result:The format of the website address is shown correctly.
See attachment:verify_v2.2_pass.png
Rate:0/5

Device: Flame 2.2 (pass)
Build ID               20150519162501
Gaia Revision          63e9eeec3032318f8a240f80b6a184fa4b50b6e1
Gaia Date              2015-05-19 17:52:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4e078e1364d3
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150519.200807
Firmware Date          Tue May 19 20:08:18 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (pass)
Build ID               20150519162501
Gaia Revision          63e9eeec3032318f8a240f80b6a184fa4b50b6e1
Gaia Date              2015-05-19 17:52:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4e078e1364d3
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150519.195445
Firmware Date          Tue May 19 19:55:01 EDT 2015
Bootloader             HHZ12f