Closed Bug 1373061 Opened 7 years ago Closed 7 years ago

require("sdk/l10n").get locale fallback broken

Categories

(Firefox :: Extension Compatibility, defect)

Product:
Firefox
Component:
Extension Compatibility
Version:
55 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: mirh, Assigned: zbraniecki)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1373061 - Fallback on en-US in sdk/l10n.get.
59 bytes, text/x-review-board-request
Pike
: review+
jcristau
: approval-mozilla-beta+
Details 
I just updated to FF 55 beta. 

https://addons.mozilla.org/en-US/firefox/addon/restartfox/
And this add-on button changed name from 'restart' to 'buttonLabel'

I guess that with the new lazy tabs humongous memory improvements I basically won't need it anymore - moreover afaiu in 2 versions it shouldn't work anymore... But still it seems a bug.
got "buttonLabel" with require("sdk/l10n").get('buttonLabel') in Debugging the add-on.
Severity: normal → trivial
Status: UNCONFIRMED → NEW
status-firefox55: --- → affected
Component: Toolbars and Customization → Extension Compatibility
Ever confirmed: true
Summary: Add-on button is now 'buttonLabel' → restartFox button and menu is 'buttonLabel' in Fx55
I cannot reproduce it if using mozregression, need to find out its origin.
Severity: trivial → normal
Has Regression Range: --- → no
Component: Extension Compatibility → Untriaged
Keywords: regressionwindow-wanted
OS: Unspecified → Windows
Hardware: Unspecified → All
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID 	20170615063713

Hi Mirh,

I was unable to reproduce the issue on the latest Release (54.0) and on the latest Nightly (56.0a1, Build ID 20170616030207).
I also tried updating Beta 54b11 to 55b2 and I had no luck.

>moreover afaiu in 2 versions it shouldn't work anymore...
Looks like the add-on is a legacy add-on and indeed it will stop working starting with Nightly 57. As a side note, Firefox has a built-in restart command. Press shift+F2 to bring up the browser toolbar and write "restart" in it. Once you press enter Firefox will restart and you won't lose your tabs.


What beta version were you on at the time of the update? 
Also what Windows OS are you using? 
And lastly, could you please try to reproduce the issue again on your end and provide the results?

Thanks,
Ciprian
Flags: needinfo?(mirh)
At the time of the update I was using latest "beta" (54b99, I think you called it). 

Now I just updated to 55b2 (from b1) and it's still there too. 
Windows 7 x64 (but firefox is 32 bit)

E10s is disabled.
Flags: needinfo?(mirh)
Please report the issue to the extension developer.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=54f983e0eb8143559c35a5d45f32fabd253932b3&tochange=39a903b099a6b50e3921fd6cb2ecc7d2b53b94d3


STR:
1. Set general.useragent.locale to a locale, except en-US, es or ja (they included in restartfox).
2. Install the https://addons.mozilla.org/firefox/addon/restartfox/.
3. See the restartFox toolbar's tooltip, or its menu in File menu.
Blocks: 1346616
Status: RESOLVED → REOPENED
Has Regression Range: no → yes
Has STR: --- → yes
status-firefox56: --- → affected
Component: Untriaged → Extension Compatibility
Keywords: regressionwindow-wanted → regression
OS: Windows → All
Resolution: INVALID → ---
Summary: restartFox button and menu is 'buttonLabel' in Fx55 → require("sdk/l10n").get locale fallback broken
Thank you Yang.
This is breaking Mailvelope too. Any chance you can weigh in, gandalf?
Flags: needinfo?(gandalf)
I would think this is caused by https://hg.mozilla.org/mozilla-central/rev/39a903b099a6, as it changes behavior of getPreferedLocales from what I can tell. It used to always contain en-US, which it no longer does, so the fallback to en-US that extension developers expect (not sure where that was documented, but it was a thing) no longer happens now (see the string lookup in https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/l10n/properties/core.js, which returns null if ti doesn't find a string in a locale from the list returned by getPreferedLocales).
I think that the summary from comment 9 sounds plausible.

I would prefer not to pretend that en-US is in requested if it's not, but rather make the language negotiation fallback on en-US if none of the requested matches.
Flags: needinfo?(gandalf)
Gandalf, how likely are we gonna be able to fix this in the 55 cycle?
Flags: needinfo?(gandalf)
Assignee: nobody → gandalf
Status: REOPENED → ASSIGNED
Flags: needinfo?(gandalf)
This is a regression from bug 1346616. The fix is easy and I think it makes sense to bring back the expected behavior even if only for a couple releases.
Comment on attachment 8884354 [details]
Bug 1373061 - Fallback on en-US in sdk/l10n.get.

https://reviewboard.mozilla.org/r/155274/#review160644

Can this be tested in addon-sdk/source/test/test-l10n-locale.js or the like?
Added the test.
Comment on attachment 8884354 [details]
Bug 1373061 - Fallback on en-US in sdk/l10n.get.

https://reviewboard.mozilla.org/r/155274/#review161210
Attachment #8884354 - Flags: review?(l10n) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97f380543bc7
Fallback on en-US in sdk/l10n.get. r=Pike
https://hg.mozilla.org/mozilla-central/rev/97f380543bc7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Please request Beta approval on this when you get a chance.
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(gandalf)
Flags: in-testsuite+
Comment on attachment 8884354 [details]
Bug 1373061 - Fallback on en-US in sdk/l10n.get.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1346616 
[User impact if declined]: Potential to break extensions code
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: It's a trivial single line change that adds one locale to a list of locales.
[String changes made/needed]: none
Flags: needinfo?(gandalf)
Attachment #8884354 - Flags: approval-mozilla-beta?
Comment on attachment 8884354 [details]
Bug 1373061 - Fallback on en-US in sdk/l10n.get.

addon compat fix, beta55+
Attachment #8884354 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #22)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Zibi Braniecki's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Confirmed fixed in 55.0b9
Thank you all.
sdk/l10n.get gives still different result in 55.0b13 compared to <=54: previously if a region was not supplied, then the fallback was to use the language file (e.g. en-US -> en.properties). With 55 you need to have a separate language file for each possible region (en-US.properties, en-GB.properties, ...).
(In reply to Thomas Oberndörfer from comment #27)
> sdk/l10n.get gives still different result in 55.0b13 compared to <=54:
> previously if a region was not supplied, then the fallback was to use the
> language file (e.g. en-US -> en.properties). With 55 you need to have a
> separate language file for each possible region (en-US.properties,
> en-GB.properties, ...).

The default fallback for SDK extensions generated by JPM has always been en-US.properties and not en.properties. that was a change from cfx to jpm.

If this is not the default fallback, this would be a behaviour change outside of the SDK code base, as I can not see any code change this year in the decision to leave out the regional specifier of a language for properties (there is none, essentially). What may have changed are the contents of the list of prefered locals, since before it included all content locales (where I assume most would have the generic fallback of their regional locale), the UI language etc. Not sure what it now contains.
As :freaktechnik said - we didn't touch the fallbacking, the default is en-US.

There's a new method that AddonSDK could use to better match between requested and available: Services.locale.negotiateLanguages(requested, available, default), which has better matching capabilities [0], but that would be a new feature, rather than fixing any regressions.


[0] http://searchfox.org/mozilla-central/source/intl/locale/LocaleService.cpp#348
I tested again and I think the difference with FF55 is that the language settings in the preferences are not used anymore for sdk/l10n.get. So for example in FF52 if you set general.useragent.locale to "en-US" and you only ship a en.properties file, but you have a "en" entry in the language preferences, then en.properties is picked.
This does not work in FF55, only general.useragent.locale is considered.
What does neither work in FF52 and FF55 is that a "en-US" setting does not fallback to a en.properties file. My expectation would be that if the exact language-region combination is not available, then the fallback should be the available language file.
Obviously for the SDK this does not matter anymore, but I wonder how the WebEx i18n API will behave in this respect.
> Obviously for the SDK this does not matter anymore, but I wonder how the WebEx i18n API will behave in this respect.

:kmag landed bug 1357902 recently which uses Services.locale.negotitateLanguages for WebExt. This will match en-XX to en.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: