1100399 - Incorrectly reused strings in patch for bug 1086958 (back out default browser prompting change)

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Stefan Plewako [:stef]]

https://hg.mozilla.org/mozilla-central/rev/7d3e942ee884
https://hg.mozilla.org/mozilla-central/rev/7f9adc144d57

Besides other problems with above changes, the old but new default browser prompt incorrectly reuses strings form the previous new default browser prompt bar.

In general strings should not be reused (without changing string id) if right from the beginning it is not clear the they are shared.

This applies to changing context (rewording strings that they depend on), changing UI elements significantly that contain the string and so on… remember that Firefox is localized and the fact that in en-US something looks the same doesn't mean that it will be true in every one of hundred localizations.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Thanks for filing this.

I always remember that Firefox is localized :)

It would be helpful if you could describe how the re-using in this particular case was problematic in more detail. What issues did it cause for the Polish localization?

Comment 2

[Stefan Plewako [:stef]]

Keep in mind that I didn't yet localized changes for bug 1086958 and the prompt already looks like https://bug1086958.bugzilla.mozilla.org/attachment.cgi?id=8523422 

localization != translation and "Zapytaj później" is not equivalent of "Not now" button label, not even close (but made good UI in previous-bar version) that's why all strings used in new interface should have new ids.

Currently this dialog is confusing in pl and is not clear besides when actually one wants to set Firefox as default. So much in fact, that I consider this as blocker for any new sign-offs for pl.

Comment 3

[Stefan Plewako [:stef]]

Also https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Stefan, thanks for the link, but I am intimately familiar with the principles behind the ID change requirement - no need to clarify that. Not changing the string ID on trunk was a mistake, and I will fix it.

At the same time, I'm trying to understand the problem this poses for Polish specifically, in order to measure the impact that our half-measure for Firefox 35/Firefox 36 in bug 1086958 will have. It sounds like you're saying that the "Not now" string needs to vary between the notification bar button context and the dialog checkbox context in your locale? Are there any other similar issues that lead to potential user confusion?

Comment 5

[Stefan Plewako [:stef]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #4)
> Not changing the string ID on trunk was a mistake, and I will fix it.

The situation is different on other branches?

> At the same time, I'm trying to understand the problem this poses for Polish
> specifically, in order to measure the impact that our half-measure for
> Firefox 35/Firefox 36 in bug 1086958 will have.

If strings would get new ids there would be no problem with wording.

> It sounds like you're saying
> that the "Not now" string needs to vary between the notification bar button
> context and the dialog checkbox context in your locale? Are there any other
> similar issues that lead to potential user confusion?

"Not now" was localized as sth closer to "Ask later" in pl, also "Options" is not directly translated but I'm not sure if it is used in new version - what difference does it make?

Comment 6

[:Gijs (he/him)]

(In reply to Stefan Plewako [:stef] from comment #5)
> "Not now" was localized as sth closer to "Ask later" in pl, also "Options"
> is not directly translated but I'm not sure if it is used in new version -
> what difference does it make?

Because we've already landed backout-ish changes for the notification bar for 34 and 35 that reuse existing strings (because beta/aurora do not really let us do much else), and so if the issues are very severe we may need to rethink changes that we've made there, perhaps only for non-en-US.

Comment 7

[Stefan Plewako [:stef]]

(In reply to :Gijs Kruitbosch from comment #6)
> because beta/aurora do not really let us do much else

That is new to me, esp for aurora, do you think about string freeze only or something else too?

> perhaps only for non-en-US.

I'm afraid that we still do not understand each other.

Comment 8

[Francesco Lodolo [:flod]]

I think that we agree on fixing this properly on Firefox 36.

Having said that, are these strings currently used only in the new modal window? Is the notification bar completely gone?

> That is new to me, esp for aurora, do you think about string freeze only or something else too?

We need to balance the needs of 80 or so locales, so yes, string freeze is one important aspect to consider. String freeze is ignored only in extreme cases.

Comment 9

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #8)
> I think that we agree on fixing this properly on Firefox 36.
> 
> Having said that, are these strings currently used only in the new modal
> window?

Yes.

> > That is new to me, esp for aurora, do you think about string freeze only or something else too?
> 
> We need to balance the needs of 80 or so locales, so yes, string freeze is
> one important aspect to consider. String freeze is ignored only in extreme
> cases.

Indeed. It's late in aurora, I don't think anyone would appreciate landing a bunch of string-freeze-breaking changes. Hence the importance of understanding the impact, in order to weight cost/benefits to various solutions. This could include, say, reverting the revert and going back to the notification bar only on non-en-US, or something - but the cost/benefit ratio would have to be dramatically different from what we previously thought, which I expect is why Gavin was asking to be sure about what the impact of the current solution is.

Comment 10

[Stefan Plewako [:stef]]

(In reply to Francesco Lodolo [:flod] from comment #8)
> > That is new to me, esp for aurora, do you think about string freeze only or something else too?
> 
> We need to balance the needs of 80 or so locales, so yes, string freeze is
> one important aspect to consider. String freeze is ignored only in extreme
> cases.

Yes and we have no idea if any other/how many of them will be affected if we do nothing.

I simply don't buy into string freeze breaks reserved for implementing Mozilla ideas and disallowed for fixing implementations of those… and statement about extreme cases sadly doesn't match the reality.

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Francesco Lodolo [:flod] from comment #8)
> > I think that we agree on fixing this properly on Firefox 36.
> > 
> > Having said that, are these strings currently used only in the new modal
> > window?
> 
> Yes.

Reading the patch, https://hg.mozilla.org/mozilla-central/rev/7d3e942ee884 this doesn't sound true but I'm not an expert and there could be something else involved - could you explain this?

Comment 11

[:Gijs (he/him)]

(In reply to Stefan Plewako [:stef] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > (In reply to Francesco Lodolo [:flod] from comment #8)
> > > I think that we agree on fixing this properly on Firefox 36.
> > > 
> > > Having said that, are these strings currently used only in the new modal
> > > window?
> > 
> > Yes.
> 
> Reading the patch, https://hg.mozilla.org/mozilla-central/rev/7d3e942ee884
> this doesn't sound true but I'm not an expert and there could be something
> else involved - could you explain this?

This is on Nightly; AFAIK the notification bar is completely gone on aurora/beta.

Comment 12

[Francesco Lodolo [:flod]]

(In reply to Stefan Plewako [:stef] from comment #10)
> Yes and we have no idea if any other/how many of them will be affected if we
> do nothing.

True. The best thing we can do is send out a mail to dev-l10n, warn localizers and check how many locales are actually affected. I might be optimistic but I don't think there will be many.

> I simply don't buy into string freeze breaks reserved for implementing
> Mozilla ideas and disallowed for fixing implementations of those… and
> statement about extreme cases sadly doesn't match the reality.

You're clearly free to not trust me, but I'm aware of how many cases we forced developers to create string-free patches for those branches. So no, string freeze is not something we break carelessly.

If these strings are used only in the modal dialog, locales still have the chance to update the localization. Not ideal but not as broken as if both dialog forms were still used.

Comment 13

[Stefan Plewako [:stef]]

(In reply to :Gijs Kruitbosch from comment #11)
> This is on Nightly; AFAIK the notification bar is completely gone on
> aurora/beta.

Ahh, I see now, OK then, if we can agree that this notification bar will not be added to 34 and 35 then I could fix this on pl side - that would simplify things but I would like to see such assurance.

(In reply to Francesco Lodolo [:flod] from comment #12)
> > I simply don't buy into string freeze breaks reserved for implementing
> > Mozilla ideas and disallowed for fixing implementations of those… and
> > statement about extreme cases sadly doesn't match the reality.
> 
> You're clearly free to not trust me, but I'm aware of how many cases we
> forced developers to create string-free patches for those branches. So no,
> string freeze is not something we break carelessly.

For me, it is not about trust but rather about judgment. Not breaking carelessly is not exactly the same as breaking in extreme cases; shipping hardcoded strings, pushing chrome strings to other repos (with much lower standards) or such hybrid string reusing like in bug 1086958 is even worse and contradicts the idea of string freeze.

Comment 14

[:Gijs (he/him)]

What are you needinfo'ing for?

Comment 15

[Stefan Plewako [:stef]]

Follow-ups for fixing important issues are useless…

> if we can agree that this notification bar will not
> be added to 34 and 35 then I could fix this on pl side - that would simplify
> things but I would like to see such assurance.

34 is already lost… I can fix this in 35 and 36 on pl side and it still needs to be fixed on central (37).

Comment 16

[:Gijs (he/him)]

(In reply to Stefan Plewako [:stef] from comment #15)
> Follow-ups for fixing important issues are useless…

I don't really know what you mean by this.


> > if we can agree that this notification bar will not
> > be added to 34 and 35 then I could fix this on pl side - that would simplify
> > things but I would like to see such assurance.
> 
> 34 is already lost… I can fix this in 35 and 36 on pl side and it still
> needs to be fixed on central (37).

IMO we should late-l10n a fix for 36. It's only been 3 days. But we should do that today. Gavin?

Comment 17

[Francesco Lodolo [:flod]]

(In reply to :Gijs Kruitbosch from comment #16)
> IMO we should late-l10n a fix for 36. It's only been 3 days. But we should
> do that today. Gavin?

The only locale that reported this issue is Polish, and this can be fixed by changing the string on the localization's side. As harsh as this might sound, I think that this should be fixed only on trunk at this point.

Comment 18

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #17)
> (In reply to :Gijs Kruitbosch from comment #16)
> > IMO we should late-l10n a fix for 36. It's only been 3 days. But we should
> > do that today. Gavin?
> 
> The only locale that reported this issue is Polish, and this can be fixed by
> changing the string on the localization's side. As harsh as this might
> sound, I think that this should be fixed only on trunk at this point.

34 and 35 use only one of the two methods of displaying, so updating the localization makes sense; the pref implementation was landed for 36, so there we now reuse the string... although I have no idea if anyone is going to flip the pref for that train at any point. In any case, I'm not the person to make a call here - you and Gavin can do that.

Comment 19

[Stefan Plewako [:stef]]

Reminder: this is still not fixed in trunk/central/36

Comment 20

[:Gijs (he/him)]

Attachment: use existing strings and add more unique strings to ensure modal prompt and notification bar don't share strings,

Don't much care who reviews this, but we should get this in before string freeze.

Comment 21

[Stefan Plewako [:stef]]

Comment on attachment 8546617 [details] [diff] [review]
use existing strings and add more unique strings to ensure modal prompt and notification bar don't share strings,

Review of attachment 8546617 [details] [diff] [review]:
-----------------------------------------------------------------

> # LOCALIZATION NOTE (setDefaultBrowserTitle, setDefaultBrowserMessage, setDefaultBrowserDontAsk):
> # These strings are used as an alternative to the ones above, in a modal dialog.
> # %S will be replaced by brandShortName
> setDefaultBrowserTitle=Default Browser
> setDefaultBrowserMessage=%S is not currently set as your default browser. Would you like to make it your default browser?
> setDefaultBrowserDontAsk=Always perform this check when starting %S.
> 
> +# LOCALIZATION NOTE (setDefaultBrowserAlertConfirm.label, setDefaultBrowserAlertNotNow.label):
> +# These strings are used as an alternative to the ones above, in a modal dialog.
> +# %S will be replaced by brandShortName
> +setDefaultBrowserAlertConfirm.label=Use %S as my default browser
> +setDefaultBrowserAlertNotNow.label=Not now

I would just use l10n note for setDefaultBrowserTitle, setDefaultBrowserMessage, setDefaultBrowserDontAsk and not add second one (add setDefaultBrowserAlertConfirm.label, setDefaultBrowserAlertNotNow.label ids to existing modal dialog note). Otherwise it looks good to me (from just reading the patch).

Comment 22

[Francesco Lodolo [:flod]]

Comment on attachment 8546617 [details] [diff] [review]
use existing strings and add more unique strings to ensure modal prompt and notification bar don't share strings,

Review of attachment 8546617 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I'd keep the strings together with the previous group and use a single localization comment, but that's just a nit.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8546617 [details] [diff] [review]
use existing strings and add more unique strings to ensure modal prompt and notification bar don't share strings,

Review of attachment 8546617 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sane. Re-use is always so nice in theory, isn't it? :)

Comment 24

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/8194656eca34

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8194656eca34

Comment 26

[Stefan Plewako [:stef]]

Thx!

Comment 27

[:Gijs (he/him)]

(In reply to Stefan Plewako [:stef] from comment #26)
> Thx!

I do hope you realize that I accidentally got the logic for the checkbox the wrong way around when considering the string next to it... I reversed the logic (but kept the string!) in bug 1120421 - I'll get that approved and uplifted to 37 ASAP. Please keep it in mind when localizing. Sorry! :-(

Comment 28

[Stefan Plewako [:stef]]

(In reply to :Gijs Kruitbosch from comment #27)
> I do hope you realize that I accidentally got the logic for the checkbox the
> wrong way around when considering the string next to it... I reversed the
> logic (but kept the string!) in bug 1120421 - I'll get that approved and
> uplifted to 37 ASAP. Please keep it in mind when localizing. Sorry! :-(

Yes, no problem, I didn't catch it too when reading the patch and while translating. With fix from bug 1120421 applied, pl should be good (like en-US).