Closed
Bug 1100399
Opened 10 years ago
Closed 10 years ago
Incorrectly reused strings in patch for bug 1086958 (back out default browser prompting change)
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
People
(Reporter: stef, Assigned: Gijs)
References
Details
Attachments
(1 file)
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•10 years ago
|
||
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?
Reporter | ||
Comment 2•10 years ago
|
||
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.
Reporter | ||
Comment 3•10 years ago
|
||
Comment 4•10 years ago
|
||
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?
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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?
Assignee | ||
Comment 6•10 years ago
|
||
(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.
Reporter | ||
Comment 7•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
(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.
Reporter | ||
Comment 10•10 years ago
|
||
(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?
Assignee | ||
Comment 11•10 years ago
|
||
(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•10 years ago
|
||
(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.
Reporter | ||
Comment 13•10 years ago
|
||
(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.
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(gavin.sharp)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 14•10 years ago
|
||
What are you needinfo'ing for?
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 15•10 years ago
|
||
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).
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 16•10 years ago
|
||
(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?
Flags: needinfo?(gijskruitbosch+bugs)
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Reporter | ||
Comment 19•10 years ago
|
||
Reminder: this is still not fixed in trunk/central/36
Assignee | ||
Comment 20•10 years ago
|
||
Don't much care who reviews this, but we should get this in before string freeze.
Attachment #8546617 -
Flags: review?(mconley)
Attachment #8546617 -
Flags: review?(jaws)
Attachment #8546617 -
Flags: review?(gavin.sharp)
Attachment #8546617 -
Flags: feedback?(splewako)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Reporter | ||
Comment 21•10 years ago
|
||
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).
Attachment #8546617 -
Flags: feedback?(splewako)
Attachment #8546617 -
Flags: feedback?(francesco.lodolo)
Attachment #8546617 -
Flags: feedback+
Comment 22•10 years ago
|
||
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.
Attachment #8546617 -
Flags: feedback?(francesco.lodolo) → feedback+
Comment 23•10 years ago
|
||
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? :)
Attachment #8546617 -
Flags: review?(mconley) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8546617 -
Flags: review?(jaws)
Attachment #8546617 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Iteration: --- → 37.2
Points: --- → 2
Flags: qe-verify+
Flags: in-testsuite-
Flags: firefox-backlog+
Updated•10 years ago
|
Iteration: 37.2 → 37.3 - 12 Jan
Comment 25•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Updated•10 years ago
|
QA Contact: camelia.badau
Assignee | ||
Comment 27•10 years ago
|
||
(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! :-(
Flags: needinfo?(gavin.sharp)
Reporter | ||
Comment 28•10 years ago
|
||
(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).
You need to log in
before you can comment on or make changes to this bug.
Description
•