Closed Bug 1160080 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

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

VERIFIED FIXED
2.2 S12 (15may)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: yulan.zhu, Assigned: sfoster)

References

Details

(Whiteboard: [2.2-nexus-5-l][systemsfe])

Attachments

(4 files)

[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
QA Whiteboard: [rtl-impact]
Triage: P2;
Putting needinfo on me to investigate this further
Flags: needinfo?(lebedel.delphine)
Priority: -- → P2
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?)
blocking-b2g: --- → 2.2?
Flags: needinfo?(lebedel.delphine) → needinfo?(yulan.zhu)
Seems similar to Bug 1150825
See Also: → 1150825
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/16214/
Flags: needinfo?(yulan.zhu) → in-moztrap+
blocking-b2g: 2.2? → 2.2+
Hi Ben,
Could you please help to check the problem? Thanks!
Flags: needinfo?(bfrancis)
Dear Gregor,
Could you please help to find someone who can help on this bug? 
Thanks!
Flags: needinfo?(anygregor)
Flags: needinfo?(anygregor)
Whiteboard: [2.2-nexus-5-l] → [2.2-nexus-5-l][systemsfe]
We may need to add the left-to-right mark where we create this string
Assignee: nobody → sfoster
Hi Sam. This might be fixed by Bug 1154438 which I am just about to land. I'll let you know.
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.
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?
Flags: needinfo?(gaia-l10n)
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
Flags: needinfo?(bfrancis) → needinfo?(stas)
(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.
(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'});
Flags: needinfo?(stas)
Flags: needinfo?(gaia-l10n)
(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 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.
Attachment #8605943 - Flags: review?(fabrice)
Comment on attachment 8605943 [details] [review]
[gaia] sfoster:net-error-bidi-bug-1160080 > mozilla-b2g:master

Yay, I learned things about bidi!
Attachment #8605943 - Flags: review?(fabrice) → review+
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?
Flags: needinfo?(tclancy)
Keywords: checkin-needed
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.
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.
Flags: needinfo?(tclancy)
Merged to master: https://github.com/mozilla-b2g/gaia/commit/27f6760ceb0c8897bd21461e8b91136d7efff2a1
Status: NEW → RESOLVED
Closed: 5 years ago
Component: Gaia::Browser → Gaia::System::System UI
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S12 (15may)
@Sam, could you help uplift to 2.2 since it is a blocker?
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.
Attachment #8605943 - Flags: approval-gaia-v2.2?
(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?
Flags: needinfo?(tclancy)
> 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.
(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.
Flags: needinfo?(tclancy)
(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?
Flags: needinfo?(tclancy)
> 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.
Flags: needinfo?(tclancy)
Keywords: verifyme
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
Flags: needinfo?(yulan.zhu)
Attachment #8605943 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Attached image 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
QA Whiteboard: [rtl-impact] → [rtl-impact][MGSEI-Triage+]
Attached image 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
Flags: needinfo?(yulan.zhu)
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.