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.

Screenshot2

This issue is also occurring when signing into a previously created firefox account. attached a new screenshot.

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).

This is one weird bug https://github.com/mozilla-b2g/gaia/blob/master/apps/system/fxa/elements/fxa-enter-password.html#L5 I have the feeling it has something to do with this (cache DOM elements) https://github.com/mozilla-b2g/gaia/blob/master/apps/system/fxa/js/screens/fxam_enter_password.js#L77 And the fact that the element #fxa-hello-user is initialized with an empty email https://github.com/mozilla-b2g/gaia/blob/master/apps/system/fxa/js/screens/fxam_enter_password.js#L128-L130

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.

Thanks for reporting this, guys! Looking now.

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

(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 ;)

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.

For testing you can use a multi-locale build https://developer.mozilla.org/en-US/Firefox_OS/Building#Building_multilocale

Figured out how to get this working beautifully with the 'mirrored english' locale, will update the bug once I get tests fixed up.

(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] 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

Comment on attachment 8454054 [details] 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.

Comment on attachment 8454054 [details] Github PR 21598 r=me for the settings part, thanks.

Comment on attachment 8454054 [details] 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

Comment on attachment 8454054 [details] Github PR 21598 Jared, could you address :stas' feedback and r? me again, please?

(In reply to Staś Małolepszy :stas from comment #27) > Comment on attachment 8454054 [details] > 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!

Comment on attachment 8454054 [details] 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.

Comment on attachment 8454054 [details] Github PR 21598 Thanks Jared, LGTM

Comment on attachment 8454054 [details] Github PR 21598 f=me. Thanks, Jared!

Awesome, thanks all. Now that gaia's open again, I've merged to master: Master: https://github.com/mozilla-b2g/gaia/commit/ed77e1cef9947e0f13efa9e7b6b0e488f56f1bc9

I flipped the wrong bit, apologies.

v2.0: https://github.com/mozilla-b2g/gaia/commit/b40d54189b0f1ff4f0a3d4ab751fda23f4e0358f

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

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