Right now we've got hard-coded English for our built-in widgets in browser/components/customizableui/CustomizableUI.jsm. Clearly that's not going to fly.

In this patch I added a new .properties file in the locale dir, which I *assumed* was the thing needed to be done here.

Comment on attachment 756556 [details] [diff] [review] Make widgets use localization strings Driveby, sorry about that: > shortcut: "Ctrl+S", >- description: "Save this page", >+ description: kWidgetsBundle.GetStringFromName("savePageButton.description"), Shortcuts need to be localizable, too. Also, is there an easy possibility for reuse here? Quite some of these strings are in our product already somewhere...

Right, that's something I wanted to flag for feedback on in this bug... thanks for pointing that out! So, how do we localize the shortcuts in this case? What was the original idea behind this?

Since Mike will also drive by for a review, I'm sure he'll be able to answer the question in comment 3 as well, right Mike? ;)

Comment on attachment 756556 [details] [diff] [review] Make widgets use localization strings > const CustomizableWidgets = [{ > id: "history-panelmenu", > type: "view", > viewId: "PanelUI-history", >- name: "History...", >- description: "History", >+ name: kWidgetsBundle.GetStringFromName("historyButton.label"), >+ description: kWidgetsBundle.GetStringFromName("historyButton.description"), If all objects in this array need names and descriptions, then it seems like they could be read from customizableWidgets.properties automatically based on the id (e.g. id + ".label" => "history-panelmenu.label" in the above case). ".description" should probably be ".tooltiptext", since seemingly that's what those strings are used for.

(In reply to :Gijs Kruitbosch from comment #2) > Comment on attachment 756556 [details] [diff] [review] > Make widgets use localization strings > > Driveby, sorry about that: > > > shortcut: "Ctrl+S", > >- description: "Save this page", > >+ description: kWidgetsBundle.GetStringFromName("savePageButton.description"), > > > Also, is there an easy possibility > for reuse here? Quite some of these strings are in our product already > somewhere... While it's possible to get .dtd translation files loaded and manipulated with JS, it's super hacky and not something I'd advise. I think straight-copying them over to .properties instead is the better plan. > So, how do we localize the shortcuts in this case? What was the original > idea behind this? That's a good question. I don't know what the original plan was here - currently, we copy the contents of the shortcut string into an acceltext attribute to the created XUL node. Acceltext, according to MDN, is an attribute used by menu items, and generally not by toolbar items. For now, let's replace the shortcut key properties with strings from customizableWidgets.properties, but we should file a follow-up to figure out how keyboard shortcuts are supposed to work here.

Comment on attachment 756556 [details] [diff] [review] Make widgets use localization strings Review of attachment 756556 [details] [diff] [review]: ----------------------------------------------------------------- Just a few light suggestions, but this is definitely the right idea. Thanks! ::: browser/components/customizableui/src/CustomizableWidgets.jsm @@ +15,5 @@ > > const kNSXUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > const kPrefCustomizationDebug = "browser.uiCustomization.debug"; > > +const kWidgetsBundle = Services.strings.createBundle( Let's make this lazy-load - we can do that with: XPCOMUtils.defineLazyGetter(this, "gWidgetsBundle", function() { const kUrl = "chrome://browser/locale/customizableui/customizableWidgets.properties"; return Services.strings.createBundle(kUrl); }); And then access it via gWidgetsBundle. See - https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/XPCOMUtils.jsm#defineLazyGetter%28%29 @@ +129,1 @@ > shortcut: "Ctrl+S", Let's just put all shortcut strings into the string bundle as well for now. I think we know this is something we want to localize - we just need to find a mechanism to do that, which we can handle in a later bug. ::: browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties @@ +2,5 @@ > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +historyButton.label = History > +# LOCALIZATION NOTE (historyButton.description): Use the unicode ellipsis char, MAYDAY MAYDAY - trailing whitespace throughout this file. :)

(In reply to Dão Gottwald [:dao] from comment #5) > If all objects in this array need names and descriptions, then it seems like > they could be read from customizableWidgets.properties automatically based > on the id (e.g. id + ".label" => "history-panelmenu.label" in the above > case). Agreed - this would remove a *lot* of boilerplate code. If the property is missing, we should attempt to get a localized string automatically. > ".description" should probably be ".tooltiptext", since seemingly > that's what those strings are used for. Agreed. I originally used "description" because at the time it wasn't clear what all the use cases of the property would be. But now days, in practice, its being used solely for tooltiptext.

Updated patch, with comments addressed.

Comment on attachment 757377 [details] [diff] [review] Make widgets use localization strings Review of attachment 757377 [details] [diff] [review]: ----------------------------------------------------------------- A lot of this is fine, but I'm concerned about buildWidget just assumes that the built widget has strings available in gWidgetsBundle. We cannot assume this to be true, since we might be building widgets from add-ons. ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +740,5 @@ > if (aWidget.disabled) { > node.setAttribute("disabled", true); > } > node.setAttribute("removable", aWidget.removable); > + node.setAttribute("label", this.getLocalizedProperty(aWidget, "label")); What about widgets that are built from add-ons? If we do this, we're forcing them to somehow get their strings into gWidgetsBundle. @@ +783,5 @@ > aWidget.instances.set(aDocument, node); > return node; > }, > > + getLocalizedProperty: function(aWidget, aProp, aFormatArgs, aDef) { For clarity, let's call that aDefault. But it doesn't even look like this is ever used...

(In reply to Mike Conley (:mconley) from comment #10) > > What about widgets that are built from add-ons? If we do this, we're forcing > them to somehow get their strings into gWidgetsBundle. > Not true, the string may be defined on the widgets' struct, using `name` and `tooltiptext` properties, which take precedence over strings defined in the properties file. > > For clarity, let's call that aDefault. But it doesn't even look like this is > ever used... True, it is not used can _can_ be to return something else the the default empty string from `getLocalizedProperty`. Apart from the above, I vote for renaming the 'name' property of a widget to 'label', to mirror the button node property names.

* not used and _can_ be to...

Comment on attachment 757377 [details] [diff] [review] Make widgets use localization strings Ok, worth digging deeper into this.

Comment on attachment 757377 [details] [diff] [review] Make widgets use localization strings Review of attachment 757377 [details] [diff] [review]: ----------------------------------------------------------------- Ok, I understand. You're right - widgets that supply their own strings are fine. I'm also fine with renaming "name" to "label" - please file a follow-up bug for that.

Updated patch, carrying over r=mconley.

Pushed: https://hg.mozilla.org/projects/ux/rev/7a4f2f5b3c6b