868433 - Make built-in widgets use localization strings

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

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.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Make widgets use localization strings

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 2

[:Gijs (he/him)]

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...

Comment 3

[Mike de Boer [:mikedeboer]]

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?

Comment 4

[Mike de Boer [:mikedeboer]]

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 5

[Dão Gottwald [:dao]]

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.

Comment 6

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

(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 7

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

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. :)

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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.

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Make widgets use localization strings

Updated patch, with comments addressed.

Comment 10

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

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...

Comment 11

[Mike de Boer [:mikedeboer]]

(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.

Comment 12

[Mike de Boer [:mikedeboer]]

* not used and _can_ be to...

Comment 13

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

Comment on attachment 757377 [details] [diff] [review]
Make widgets use localization strings

Ok, worth digging deeper into this.

Comment 14

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

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.

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Make widgets use localization strings

Updated patch, carrying over r=mconley.

Comment 16

[:Gijs (he/him)]

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

Comment 17

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/7a4f2f5b3c6b