Closed Bug 1036030 Opened 10 years ago Closed 10 years ago

[B2G][2.0][l10n][FxA] Some translations for fxa-hello-user string are wrongly displayed by Firefox OS

Categories

(Firefox OS Graveyard :: FxA, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: AdamA, Assigned: jhirsch)

References

Details

(Whiteboard: LocRun2.0)

Attachments

(12 files)

Attached image Screenshot
Description:
When creating a new firefox account there will be an exclamation mark(!) that appears before the email address on the "Create a password" screen.

Repro Steps:
1) Update a Flame to BuildID: 20140630000201
2) Change language to Hungarian (Magyar) 
3) Restart device
4) Select "Settings"
5) Select "Firefox Accounts"
6) Choose to create a new account
7) Proceed to the "Create a Password" screen
8) Observe Email address

Actual:
An exclamation mark appears before the email address

Expected:
It is expected that there are no exclamation marks before email

v2.0 Environmental Variables:
Device: Flame v2.0 MOZ ril
BuildID: 20140707000200
Gaia: ef67af27dff3130d41a9139d6ae7ed640c34d922
Gecko: f53099796238
Version: 32.0a2
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32

Keywords: FxA

Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/13719/
See attached: Screenshot

-------------------------------------------------------

This issue does not occur on 1.4 Flame.

Environmental Variables:
Device: Flame 1.4
BuildID: 20140630000201
Gaia: aa896d5db1b4929f3bf31a0f4bb7de50530222a8
Gecko: 8cba60bc12ef
Version: 30.0 (1.4) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

String did not exist on v1.4.
Blocks: 1032262
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Summary: [B2G][2.0][l10n][Settings] hu: An exclamation mark is appearing before the email address on the password screen when creating a new Firefox account → [B2G][2.0][l10n][Settings] Hungarian: An exclamation mark is appearing before the email address on the password screen when creating a new Firefox account
Whiteboard: LocRun2.0
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached image 2014-07-08-13-46-27.png
Screenshot2
This issue is also occurring when signing into a previously created firefox account. attached a new screenshot.
No longer blocks: 1032262
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
I see it, but this is strange: the exclamation mark should be after the address:

gabor@gabor-laptop:~/src/fsfhu-scripts/gaia-l10n-hu/gaia-l10n-hu$ grep -r Szia *
apps/system/fxa/fxa.properties:fxa-hello-user = Szia, {{email}}!

Can be there a stray RTL mark somewhere? I have no idea how could this happen.
This is actually not the fault of the Hungarian team, but a weird Firefox OS bug.

Tried with Japanese (which also have text after {{email}}), we have the same issue.
From what I understand: 
The OS is moving all the text after the variable for unknown reasons and putting it before the variable. This shouldn't happen.

This is causing some translations to be totally wrong. At least hu, ja, ko. But also potentially all RTL locales (needs to be confirmed, thought).
blocking-b2g: --- → 2.0?
Component: hu / Hungarian → FxA
Product: Mozilla Localizations → Firefox OS
Summary: [B2G][2.0][l10n][Settings] Hungarian: An exclamation mark is appearing before the email address on the password screen when creating a new Firefox account → [B2G][2.0][l10n][FxA] Some translations for fxa-hello-user string are wrongly displayed by Firefox OS
Just tested it, it seems like yes, happening in RTL locales too.
Since RTL languages start from the right, the exclamation mark should be at the far left.
blocking-b2g: 2.0? → 2.0+
Thanks for reporting this, guys!

Looking now.
Assignee: nobody → 6a68
Flags: needinfo?(6a68)
The English locale file doesn't have an exclamation mark at the end of the fxa-hello-user string[1]. 

I did not know it was expected for localizers to alter/insert punctuation in approved strings. This could certainly cause problems, because l10n.js requires totally different approaches to translate text with an html element at the end, vs text containing an html element in the middle.

Is it possible to just not insert punctuation in this case? That seems simplest to me, given how close we are to feature complete.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/fxa/locales/fxa.en-US.properties#L76
Flags: needinfo?(tchevalier)
Flags: needinfo?(francesco.lodolo)
(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #9)
> The English locale file doesn't have an exclamation mark at the end of the
> fxa-hello-user string[1]. 
> 
> I did not know it was expected for localizers to alter/insert punctuation in
> approved strings. This could certainly cause problems, because l10n.js
> requires totally different approaches to translate text with an html element
> at the end, vs text containing an html element in the middle.
> 
> Is it possible to just not insert punctuation in this case? That seems
> simplest to me, given how close we are to feature complete.
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/fxa/locales/fxa.
> en-US.properties#L76

It's absolutely not about punctuation, the point of having a variable in a string is that localizers must be able to move the it anywhere.
Take a look at Japanese for instance, the language requires the variable to be at the beginning of the sentence.
http://transvision.mozfr.org/?sourcelocale=en-US&locale=ja&repo=gaia&search_type=entities&recherche=apps/system/fxa/fxa.properties:fxa-hello-user

On a side note about punctuation, localizers can totally use a different punctuation than English, we shouldn't make assumptions on that. I can assure you that in French for instance, there is a lot of cases where we must add a period whereas it's totally correct not to add one in English. Each language has its own rules ;)
Flags: needinfo?(tchevalier)
Flags: needinfo?(francesco.lodolo)
Thanks for the explanation! Makes sense.

I'll go ahead and make all the placeholder bits more flexible, following the approach that seems to have worked for inserting links into the tos/pp paragraph: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/fxa/js/screens/fxam_enter_email.js#L65-L75
Hey Theo - two questions:

1. Does the terms/privacy text look ok in all RTL languages? If so, I can reuse that approach as mentioned in my previous comment. (I'm doing this now.)

2. How can I display the Hungarian or Japanese locales when I am doing development work in the gaia repo? I want to be sure I've fixed this locally before I submit a patch.
Flags: needinfo?(tchevalier)
Figured out how to get this working beautifully with the 'mirrored english' locale, will update the bug once I get tests fixed up.
Flags: needinfo?(tchevalier)
(In reply to Francesco Lodolo [:flod] from comment #13)
> For testing you can use a multi-locale build
> https://developer.mozilla.org/en-US/Firefox_OS/Building#Building_multilocale

Thanks for that, will be good to know in the future. Turns out there are no locales yet for hungarian for fxa, so I wound up using mirrored english instead.
OK, I've got a patch which fixes this bug. It updates every place where fxa inserts inline html into a localized string. This mostly occurs in the system app, but there's 1 occurrence in settings also. To avoid copy-pasting a util into multiple places, I added an L10nHelper to shared/js/utilities.js for now. Maybe we can figure out whether/how to integrate this into l10n.js in a separate bug.

flod, I am guessing that this approach is broadly ok, since it has gotten your f+ in the past, but now I'm being uniform in using it, and have extracted it into a helper.

I am happy to open a followup bug to add this to l10n.js, but since we are right up against feature complete deadline for 2.0, I'd prefer we file a followup bug and hash it out after 2.0 ships.

------

I have verified this patch works by adding exclamation marks after the placeholders that didn't have trailing punctuation in English, and making sure every html replacement looks correct in the 'Mirrored English' locale. I'm not going to commit those exclamation marks, just wanted to convince myself I've fixed the bug.

I'll attach screenshots of all affected screens, showing that they correctly handle trailing '!' in the Mirrored English locale.

The affected screens are:
  - settings: the unverified panel, shown after creating, but before verifying, an account.
    the string says "don't see an email? resend." the resend link is the html that should be positioned before the period.
  All others are in the fxa system app)
  - tos/pp string in the enter email screen
  - coppa failure error message, triggered by creating a new account with birth year 2010
     - I manually added an '!' at the end of this one to verify it's correct
  - the 'hello, user' string in the set password (sign up flow) and enter password (sign in flow) screens.
  - the 'we will send email' string in the success screen shown at the end of sign up flow
     - I manually added an '!' at the end of this string, too

------

A little bit more about l10n.js, for reviewers:

l10n.js recursively localizes text, but it pushes any inline html elements to the end of the string[1], and inserts text via el.textContent, not el.innerHTML. This means that 
  "<p l10n-id="x">hello <a>user</a> goodbye</p>"
becomes 
  "<p>hello goodbye <a>user</a></p>"
which is definitely not what we want, and there's no simple/clean way to handle this kind of case in the library as currently written.

The workaround is to (1) use JS string replacement to swap out l10n placeholders for inline elements, then (2) use innerHTML to insert the translated strings with correctly-placed inline els. This is the logic I've moved to the shared L10nHelper.

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L1571-L1585
Comment on attachment 8454054 [details] [review]
Github PR 21598

Hey all,

Do you mind reviewing a 2.0 blocker patch? The fix is relatively simple, but the explanation is a bit complex; see comment 17 for an overview.

Gaia-try is green.

I'm on PTO Mon-Wed of next week, and would like to actually not work during that time ;-), so I'm hoping to land this patch by Friday evening my time--if you don't think you'll have time to review within the next day, I would really appreciate it if you could find another reviewer with time to spare.

Thanks very much,

Jared
Attachment #8454054 - Flags: review?(ferjmoreno)
Attachment #8454054 - Flags: review?(arthur.chen)
Attachment #8454054 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8454054 [details] [review]
Github PR 21598

I'll pass this to someone actually working on l10n.js

While I think this is generally OK for 2.0, we should probably be able to do better on 2.1 with mutation observers & C.
Attachment #8454054 - Flags: feedback?(francesco.lodolo) → feedback?(stas)
Comment on attachment 8454054 [details] [review]
Github PR 21598

r=me for the settings part, thanks.
Attachment #8454054 - Flags: review?(arthur.chen) → review+
Comment on attachment 8454054 [details] [review]
Github PR 21598

Flod, thanks for forwarding this to me.  Jared, in the future, please CC or needinfo me (:stas) or Zibi (:gandalf) if you're planning to work around the behavior of l10n.js.  Ever since the new library landed in early April, we've been busy making the API better and more helpful, and cleaning up bad coding patterns.  There might already be a bug filed :)

setTextContent [1] is broken.  We inherited it from the previous version of the l10n.js library;  it was added in bug 902363 and I'm not sure it was needed.  It makes mozL10n.localize surprise developers and localizers alike.

We have a much better replacement for setTextContent ready for Vivien's review in bug 994357.  See bug 994357 comment 16 for an example of this new approach (interestingly, it's taken from fxa).

Consequently, I'm against building complex workarounds, and abstracting them into classes in shared/js.  Instead, I suggest that the fix be as simple as possible: 

  var hello = _('fxa-hello-user');
  hello = hello.replace(
    '{{email}}',
    '<a id="fxa-user-email">' + this.email + '</a>'
  );
  this.fxHelloUser.innerHTML = hello;

Once bug 994357 lands, we'll grep all ".innerHTML =" and we will start replacing them with the new approach.

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L1571-L1585
Attachment #8454054 - Flags: feedback?(stas) → feedback-
Comment on attachment 8454054 [details] [review]
Github PR 21598

Jared, could you address :stas' feedback and r? me again, please?
Attachment #8454054 - Flags: review?(ferjmoreno)
(In reply to Staś Małolepszy :stas from comment #27)
> Comment on attachment 8454054 [details] [review]
> Github PR 21598
> 
> Flod, thanks for forwarding this to me.  Jared, in the future, please CC or
> needinfo me (:stas) or Zibi (:gandalf) if you're planning to work around the
> behavior of l10n.js.  Ever since the new library landed in early April,
> we've been busy making the API better and more helpful, and cleaning up bad
> coding patterns.  There might already be a bug filed :)

Awesome! I'll definitely ni? you next time I hit l10n.js weirdness.


> 
> setTextContent [1] is broken.  We inherited it from the previous version of
> the l10n.js library;  it was added in bug 902363 and I'm not sure it was
> needed.  It makes mozL10n.localize surprise developers and localizers alike.

loool, tell me about it. it also makes l10n-related reviews needlessly complex.

> 
> We have a much better replacement for setTextContent ready for Vivien's
> review in bug 994357.  See bug 994357 comment 16 for an example of this new
> approach (interestingly, it's taken from fxa).
> 
> Consequently, I'm against building complex workarounds, and abstracting them
> into classes in shared/js.  Instead, I suggest that the fix be as simple as
> possible: 
> 
>   var hello = _('fxa-hello-user');
>   hello = hello.replace(
>     '{{email}}',
>     '<a id="fxa-user-email">' + this.email + '</a>'
>   );
>   this.fxHelloUser.innerHTML = hello;
> 
> Once bug 994357 lands, we'll grep all ".innerHTML =" and we will start
> replacing them with the new approach.

Sure, that works for me. I actually was doing the innerHTML inline prior to this patch; the helper was just to make things a bit cleaner, and also in hopes of getting this kind of l10n discussion started.

Thanks for the feedback, stas!
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Comment on attachment 8454054 [details] [review]
Github PR 21598

OK, I've updated the bug to not use a shared helper.

The patch now does not change anything in settings (sorry about that, Arthur, but thanks for the review anyway ^_^).

Changes to fxa system app are pretty straightforward.
Attachment #8454054 - Flags: review?(ferjmoreno)
Attachment #8454054 - Flags: feedback?(stas)
Comment on attachment 8454054 [details] [review]
Github PR 21598

Thanks Jared, LGTM
Attachment #8454054 - Flags: review?(ferjmoreno) → review+
Comment on attachment 8454054 [details] [review]
Github PR 21598

f=me.  Thanks, Jared!
Attachment #8454054 - Flags: feedback?(stas) → feedback+
Awesome, thanks all.

Now that gaia's open again, I've merged to master:

Master: https://github.com/mozilla-b2g/gaia/commit/ed77e1cef9947e0f13efa9e7b6b0e488f56f1bc9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S6 (18july)
I flipped the wrong bit, apologies.
Verified fixed on Flame 2.0 with japanese

Gaia      5ba22d458fdb63bd72c59de53c701d0efe35c1e2
Gecko     https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/6fbc60a80c6d
BuildID   20140806000200
Version   32.0
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014
B1TC00011230
Status: RESOLVED → VERIFIED
Attached image Flame2.0 screenshot
This issue has been verified successfully on Flame 2.0 and 2.1
See attachment: Flame2.0_screenshot.png & Flame2.1_screenshot.png
Reproducing rate: 0/2

Flame 2.0 build:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/29222e215db8
Build-ID        20141203000201
Version         32.0

Flame 2.1 build:
Gaia-Rev        dbaf3e31c9ba9c3436e074381744f2971e15c7bf
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebce587d2194
Build-ID        20141203001205
Version         34.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: