987418 - Update copy in fxa system app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FxA, defect, P2)

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

This covers copy changes visible in the 960130 spec in the fxa system app.

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Github PR 17613

Comment 2

[Francesco Lodolo [:flod]]

Sorry for reiterating the comment over 3 bugs, but I want to be sure reviewers see it.
https://developer.mozilla.org/en-US/docs/Making_String_Changes

This rule applies to Gaia as well.

So, please, get the copy checked before landing code. Everyone will be happier.

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Francesco Lodolo [:flod] from comment #2)
> Sorry for reiterating the comment over 3 bugs, but I want to be sure
> reviewers see it.
> https://developer.mozilla.org/en-US/docs/Making_String_Changes
> 
> This rule applies to Gaia as well.

I wasn't aware of this rule--thanks for letting me know.

> 
> So, please, get the copy checked before landing code. Everyone will be
> happier.

Not totally clear what you mean by "get the copy checked before landing code." Is your concern that the names of the l10n-ids should be updated to reflect changes in the meaning of the text?

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8396834 [details] [review]
Github PR 17613

unsetting r? while I wait on clarification from :flod

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Ah, I see the comment was made over here: https://bugzilla.mozilla.org/show_bug.cgi?id=987417#c2

Comment 6

[Francesco Lodolo [:flod]]

Basically: once a string lands on master, we need a new ID every time the content of that string changes, unless it's a minor typo/fix. This invalidates any existing localization, and that's the only way to make sure localizers see the change and take action updating their work. 

Localizers are not happy because they need to localize things twice, developers are not happy either because they often see this passage as an unnecessary burden. If copy gets reviewed before landing code for the first time, this saves us at least one step.

Comment 7

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Cool, thanks for the info. We'll keep it in mind for the next sprint :-)

Comment 8

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8396834 [details] [review]
Github PR 17613

l10n keys changed, resubmitting.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8396834 [details] [review]
Github PR 17613

It seems that you need to update the tests to make Travis happy.

Also shouldn't we revert the changes from bug 961009?

I'm afraid that I am not the best reviewer for this. I'd prefer someone else with real knowledge of the l10n process to review these changes.

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

ferjm: thanks for the feedback! do you know who would be a good reviewer for the l10n stuff?

Comment 11

[Francesco Lodolo [:flod]]

Not much to review from the l10n side: all changed strings have been renamed, and that's the only requirement we have.

+fxa-use-features= Now you can use features like Find My Device and apps like the Firefox Marketplace (bug 981909).

Is this copy reviewed? Because we've just fixed a bug asking to rename "the Firefox Marketplace" to "Firefox Marketplace" on mozilla.org

Comment 12

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Thanks for jumping in, flod!

(In reply to Francesco Lodolo [:flod] from comment #11)
> Not much to review from the l10n side: all changed strings have been
> renamed, and that's the only requirement we have.
> 
> +fxa-use-features= Now you can use features like Find My Device and apps
> like the Firefox Marketplace (bug 981909).
> 
> Is this copy reviewed? Because we've just fixed a bug asking to rename "the
> Firefox Marketplace" to "Firefox Marketplace" on mozilla.org

The copy was reviewed several weeks ago, perhaps the guidelines have changed since then.

Do you happen to have the bug number? I will ask product/UX about this.

Comment 13

[Francesco Lodolo [:flod]]

Sorry, I thought I added the bug number this morning but clearly forgot. It's bug 981909.

Comment 14

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Ah, it turns out that whoever filed bug 981909 was wrong, and we *do* always want to say "The Firefox Marketplace". See that bug for related discussion.

So, the l10n stuff is fine (per flod in comment 11).

I've reverted the changes from bug 961009, and I've also changed the locale file name per the comment in that bug[1].

ferjm, are you good to review this? If not, could you suggest a better reviewer?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=961009#c1

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Francesco Lodolo [:flod] from comment #11)
> Not much to review from the l10n side: all changed strings have been
> renamed, and that's the only requirement we have.
> 

I don't think the string rename was needed in any case, since I think this copy never made it to the l10n system because of bug 961009.

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #14)
> Ah, it turns out that whoever filed bug 981909 was wrong, and we *do* always
> want to say "The Firefox Marketplace". See that bug for related discussion.
> 
> So, the l10n stuff is fine (per flod in comment 11).
> 
> I've reverted the changes from bug 961009, and I've also changed the locale
> file name per the comment in that bug[1].
> 
> ferjm, are you good to review this? If not, could you suggest a better
> reviewer?
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=961009#c1

Yes, but please make sure that Travis is happy with the changes.

Comment 17

[Francesco Lodolo [:flod]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #15)
> I don't think the string rename was needed in any case, since I think this
> copy never made it to the l10n system because of bug 961009.

They were merged about 2 weeks ago
https://github.com/mozilla-b2g/gaia/commit/e7c75d3d41719ea5ac20184777874c230b61b3b7

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Those are for the Settings app, not the System app.

Comment 19

[Francesco Lodolo [:flod]]

Argh, I always mix system and settings, the fact that the other bug was this-1 didn't help my mind (bug 987417) :-\

You're right, those strings weren't exposed yet to l10n (changing ID doesn't hurt though).

Comment 20

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8396834 [details] [review]
Github PR 17613

Hi Fernando - Finally got back around to this bug, tests are green. Mind having a look?

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8396834 [details] [review]
Github PR 17613

Thanks Jared. Squash the commits before merging, please.

Comment 22

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Master: https://github.com/mozilla-b2g/gaia/commit/3d4b4b06475d2376bad23ac46da185cd48a68d17

Comment 23

[Francesco Lodolo [:flod]]

Sorry to reopen, but when I checked this patch before I saw only some changed strings, not all the strings I found this morning in the repo (the entire fxa.properties). I guess you enabled that file for localization :-\

I might sound repetitive, but was this ever copy reviewed? 

Titles have mixed cases, e.g. "Unable to connect" vs "Cannot Create Account".

Is the name "Firefox Account" or "Firefox Accounts", because I'm extremely confused about it: Firefox uses the first form, I might be wrong but it's the first time I see "Firefox Accounts" used.

Also some strings like "signed in Firefox." or "Unable to connect to Firefox" are even more confusing to me. Isn't the name "Firefox Account(s)"? 

> fxa-already-signed-in-message = There is an user is already signed in Firefox.
Not really English

> fxa-invalid-password-message = You have used an invalid password. Please try again with a valid one.
The "with a valid one" sounds weird in a password message error. It seems like a bad copy/paste from the email error message below.

Why landing all that lorem impsum stuff (fxa-tos-header, fxa-pp-header)?

Make sure you don't have duplicate string IDs (e.g. fxa-back).

Considering the amount of mess in these strings, we either backout the patch or temporarily disable localization again.

Comment 24

[Matej Novak [:matej]]

Looks like there are definitely some issues here. I'm flagging Stephany to make sure this gets covered in the copy audit.

Are there any specific strings that I can help with immediately?

As for Firefox Accounts, since it isn't a brand name, it can be singular or plural. The easiest comparison is how we treat add-ons (which means that "account" and "accounts" also get localized).

Comment 25

[Francesco Lodolo [:flod]]

Not sure, these strings seem all but final, and that's the reason why they weren't exposed to localization a few weeks ago.

(In reply to Matej Novak [:matej] from comment #24)
> As for Firefox Accounts, since it isn't a brand name, it can be singular or
> plural. The easiest comparison is how we treat add-ons (which means that
> "account" and "accounts" also get localized).

And when you talk about the feature in general, is it "Firefox Account" or "Firefox Accounts"? As I said, I don't think I've seen the plural form used elsewhere.

Comment 26

[Matej Novak [:matej]]

(In reply to Francesco Lodolo [:flod] from comment #25)
> Not sure, these strings seem all but final, and that's the reason why they
> weren't exposed to localization a few weeks ago.
> 
> (In reply to Matej Novak [:matej] from comment #24)
> > As for Firefox Accounts, since it isn't a brand name, it can be singular or
> > plural. The easiest comparison is how we treat add-ons (which means that
> > "account" and "accounts" also get localized).
> 
> And when you talk about the feature in general, is it "Firefox Account" or
> "Firefox Accounts"? As I said, I don't think I've seen the plural form used
> elsewhere.

In general, I'd say Firefox Accounts, but it really depends on usage.

We could say both "Welcome to Firefox Accounts" and "Create a Firefox Account."

Does that help?

Comment 27

[Francesco Lodolo [:flod]]

(In reply to Matej Novak [:matej] from comment #26)
> In general, I'd say Firefox Accounts, but it really depends on usage.
> 
> We could say both "Welcome to Firefox Accounts" and "Create a Firefox
> Account."
> 
> Does that help?

Yes, it's definitely clearer.

Comment 28

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey Francesco,

Uggh, I didn't look closely enough at the strings either, this is a bummer.

We have had final copy review: as mentioned in comment 0, the spec is attached to bug 960130[1].

The lorum ipsum is a placeholder because we don't have ToS/PP from legal yet. That is captured separately in bug 949065.

I will back this patch out, make sure all the strings match the finalized spec, and flag you for feedback when I resubmit. That way, you can do a single pass, offer your feedback, and we can get this fixed properly.

Does that sound good?

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8395908

Comment 29

[Francesco Lodolo [:flod]]

(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #28)
> Does that sound good?

Thanks Jared, sounds definitely good to me.

Comment 30

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Reverted in master: https://github.com/mozilla-b2g/gaia/commit/69c64170f3a78cce518d81adc8ff094d6a894b25

Comment 31

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Github PR 18392

Comment 32

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8407911 [details] [review]
Github PR 18392

OK, Francesco, I figured out what happened.

Every single possible error in the Gecko code was mapped to a WIP Gaia error message when this was initially implemented. However, only a few error messages were actually in the spec--and I didn't delete the others.

I've filed bug 997493 to go over the rest of the error messages with UX (I have a meeting with jgruen tomorrow, we'll work it out then). I've also added a TODO with the bug number, and commented those funky error messages out.

The TOS/PP are waiting on legal approval. The tracking bug for that is bug 949065, and I've added a TODO indicating that as well.

Comment 33

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

What I forgot to say in c32 is that, actually, most of the strings were fine--it was those unspec'd error messages that really looked bad. So fixing this was not as much work as I feared :-)

Comment 34

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8407911 [details] [review]
Github PR 18392

Hey Fernando, mind reviewing (again)? I've kept the already r+ commit separate from the minor changes made since, to make it easier to see what needs another pass.

Comment 35

[Francesco Lodolo [:flod]]

Comment on attachment 8407911 [details] [review]
Github PR 18392

This time I'm looking at the entire .properties file
https://github.com/6a68/gaia/blob/resubmit-bug-987418-fxa-system-app-copy/apps/system/fxa/locales/fxa.en-US.properties

1. Can we agree on the case for titles fixed before landing? I still see mixed title/sentence case (and "Create A Password" seems just wrong)

2. Just don't land lorem ipsum placeholders, you don't need them exposed in the .properties file, hard-code them if you really need. By exposing them in the .properties file, you force localizers to "localize" these strings.

Besides that, has someone talked with legal about this? In some cases legal takes ownership of these localizations (e.g. see FxOS privacy).

3. Do we really need to keep those commented out strings? We always have history in case we need to recover them, I found them extremely confusing like this.

4. Last but MOST important. My understanding is that every single spec has to undergo copy review before coding, this is clearly not happening here (i.e. TODOs). Is there a specific reason? Am I misunderstanding the entire process? I hope Stephany can answer this.

Landing not finalized strings is a potential waste of localization resources, and we're already thin on that side.

Comment 36

[Matej Novak [:matej]]

(In reply to Francesco Lodolo [:flod] from comment #35)
> Comment on attachment 8407911 [details] [review]
> Github PR 18392
> 
> This time I'm looking at the entire .properties file
> https://github.com/6a68/gaia/blob/resubmit-bug-987418-fxa-system-app-copy/
> apps/system/fxa/locales/fxa.en-US.properties
> 
> 1. Can we agree on the case for titles fixed before landing? I still see
> mixed title/sentence case (and "Create A Password" seems just wrong)

Yes, either way that's wrong. It should be "Create a Password" or "Create a password" (though I believe the former is what we decided for titles).

Comment 37

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

OK, just met up with jgruen (fxa UX) to iron stuff out. (To be clear, I inserted the TODOs as reminders of things to cover with jgruen today.)

- We do have TOS/PP from legal, I could use some help with the process for adding these. I will remove the lorem ipsum from this patch and we can address TOS/PP in bug 998012.

- jgruen will get a generic error message to replace the many weirdly technical error messages currently commented-out in the pull request. He thinks there's a bug somewhere in the github fxa-content-server repo with copy that's already been discussed with Matej (if I understood correctly in the meeting). The extra error screen will be copy-reviewed and added to the spec in bug 960130.

- "Create a Password" is indeed how it's written in the spec, my mistake there.

- The "authenticating" and "registering" strings will be removed in favor of using "connecting" in all the overlays. We're keeping things simple as our audience is not technical.

This bug is blocked on getting a generic error message approved. In the meantime, I can clear out the other TODOs from the l10n file, and I'll need to edit the error display logic and tests, too.

Comment 38

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8407911 [details] [review]
Github PR 18392

I'll need to make additional changes, clearing r? for now

Comment 41

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

there are a lot of error cases that are missing from the spec, so I'm removing any unspec'd error messages in this bug, and we will get UX/product/copy input on anything missing in a separate bug.

This lets the copy changes finally land without overlooking the fact that the error handling UX is not good enough.

Bug 1004209 will track improvements in error handling UX.

Comment 42

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8407911 [details] [review]
Github PR 18392

Hey Francesco,

I've made changes as outlined in c37 and c41. Mind taking another look?

Cheers,

Jared

Comment 43

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8407911 [details] [review]
Github PR 18392

Hey Fernando,

Mind taking another quick look at this? I've made minor changes since your last r+, but it's enough of a delta that I'd appreciate a quick look.

Thanks!

Jared

Comment 44

[Francesco Lodolo [:flod]]

Comment on attachment 8407911 [details] [review]
Github PR 18392

Left comments on Github. My main concern is to hide as much HTML code as possible from strings.

Example:

fxa-will-send-email = We will send an email to: <strong id="fxa-summary-email">email</strong>

This can be easily replaced by 
fxa-will-send-email = We will send an email to: {{email}}

And do whatever you want with {{email}} in the code. This way less chances to break things.

Also, I'm still seeing the mixed cases (comment 36), but that's not a l10n issue.

Comment 45

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Thanks for the feedback.

(In reply to Francesco Lodolo [:flod] from comment #44)
> 
> Also, I'm still seeing the mixed cases (comment 36), but that's not a l10n
> issue.

Can you please list exactly where you are seeing mixed cases?

We have now done several passes since the code was merged and reverted. It would be very helpful if you could be as complete and specific in your feedback as possible, so we can get this bug finished.

For instance, the use of HTML in the strings wasn't mentioned in any of the previous comments in this bug, and it is not mentioned in the MDN page[1]. If there were clear expectations, like a checklist of l10n requirements, I would be happy to address them before submitting the code.

[1] https://developer.mozilla.org/en-US/docs/Making_String_Changes

Comment 46

[Francesco Lodolo [:flod]]

First example I find

> fxa-connection-error-title = Unable to Connect
> fxa-reset-password-error-title = Unable to reset password

Why is the case different? Aren't they both titles?

About the HTML, my bad for not spotting it before, I realized it only after seeing the gigantic link. There are no strict rules for this (besides "no concatenations"), so you could just land the strings as they are, but let me comment some of the cases.

> fxa-coppa-failure-error-message = You must meet certain age requirements to create a Firefox Account. <a href='http://www.ftc.gov/news-events/media-resources/protecting-consumer-privacy/kids-privacy-coppa'>Learn more</a>

Why do you need to expose the URL to localizers? Also: if the URL changes, you need to update the entire string with a new ID.

Possible alternatives

> fxa-coppa-failure-error-message = You must meet certain age requirements to create a Firefox Account. <a href='#'>Learn more</a>

And change the href at run-time. Or drop the HTML entirely

> fxa-coppa-failure-error-message = You must meet certain age requirements to create a Firefox Account. {{learn-more}}
> fxa-coppa-learn-more = Learn more

Why this? Because it reduces the chances to mess up when localizing.

> fxa-hello-user = Hello, <a id="fxa-user-email">[e-mail]</a>
> fxa-will-send-email = We will send an email to: <strong id="fxa-summary-email">email</strong>

You don't need to localize the actual link, since it will be replaced at run-time (I assume), or change the mark up, so just use a placeholder instead.

> fxa-hello-user = Hello, {{email}}
> fxa-will-send-email = We will send an email to: {{email}}

Again: zero chances to break things, less confusion.

> fxa-agree-to-ff-tos-pp = By proceeding, I agree to the <a href="#fxa-tos">Terms of Service</a> and <a href="#fxa-pp">Privacy Notice</a> of Firefox cloud services.

This would turn in 3 strings, so I might understand the reason for not going for placeholders.

As I said, I'm sorry because I should have noticed these during previous checks, instead of focusing only on string content. The only ones I would definitely push for are those with email links (fxa-hello-user, fxa-will-send-email).

Comment 47

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8407911 [details] [review]
Github PR 18392

Clearing the r? until Francesco's feedback is addressed.

Comment 48

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8407911 [details] [review]
Github PR 18392

Hi Francesco,

Do you have time to take another look at this code? All the html has been removed from the properties file :-)

I've had to break up some of the strings in order to build them up with JS, but I've inserted comments to make it clear to localizers how the individual pieces will fit together.

The code is not quite final yet (I will need to make some changes once 980638 lands), but the l10n properties file and related code should be working and ready to go.

Comment 49

[Francesco Lodolo [:flod]]

Comment on attachment 8407911 [details] [review]
Github PR 18392

Left some comments on Github.

Unfortunately concatenation is worse than having HTML code in the string, so we should avoid that. Also, never hard-code characters (in this case blank spaces).

One other nit: for comments referencing one specific string, it's good to use this format

# LOCALIZATION NOTE(STRINGID): comment etc.

Some tools rely on this structure to read the comment and display it together the string to translate.

Comment 50

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Marked as blocking-b2g, since fxa is a dependency for find my device, both of which are shipping in 2.0

Comment 51

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Francesco Lodolo [:flod] from comment #49)
> Comment on attachment 8407911 [details] [review]
> Github PR 18392
> 
> Left some comments on Github.

Saw that-thanks.

> 
> Unfortunately concatenation is worse than having HTML code in the string, so
> we should avoid that. Also, never hard-code characters (in this case blank
> spaces).

Cool, I've tried another approach in the latest patch: I use localize(), but leave the placeholders in there, then manually search/replace in JS, replacing them with element strings that get innerHTML'd. Seems to work, the code isn't *too* ugly to look at, and this should totally avoid putting html in the strings.

> 
> One other nit: for comments referencing one specific string, it's good to
> use this format
> 
> # LOCALIZATION NOTE(STRINGID): comment etc.
> 
> Some tools rely on this structure to read the comment and display it
> together the string to translate.

Awesome, thanks. I've updated the comments to use that format.

Comment 52

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8407911 [details] [review]
Github PR 18392

Hey Francesco,

I've pushed an updated diff to github, mind having a look?

Jared

Comment 53

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8407911 [details] [review]
Github PR 18392

(doh! missed a field, set feedback for myself).

Hey Francesco,

I've pushed an updated diff to github, mind having a look?

Jared

Comment 54

[Francesco Lodolo [:flod]]

Comment on attachment 8407911 [details] [review]
Github PR 18392

Thanks Jared, strings look great now.

Comment 55

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Strings are approved and copy is approved, clearing ni for swilkes.

Comment 56

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8407911 [details] [review]
Github PR 18392

Hi Fernando,

Here's a rather large review, though most of the diff is related to l10n changes. Do you have time to take a look? Tests are all green.

Cheers,

Jared

Comment 57

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8407911 [details] [review]
Github PR 18392

Thanks Jared. LGTM. I'd like to understand why did you remove the error cases though.

Comment 58

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Thanks very much, Fernando!

I talked over the error cases with UX, and the suggestion was not to show users more detail than is necessary, especially since many of our target users won't have much computer experience. So, in most of the removed cases, we'll just show the unknown error message, which invites them to try again later.

Comment 59

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Master: https://github.com/mozilla-b2g/gaia/commit/f78a5ae520f2c4161d72cbcdb73e355569256e81