Make built-in widgets use localization strings

RESOLVED FIXED in Firefox 28

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mconley, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M6])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
No longer blocks: 770135
Whiteboard: [Australis:M7]

Updated

6 years ago
Whiteboard: [Australis:M7] → [Australis:M6]
(Reporter)

Updated

6 years ago
Blocks: 872617
(Reporter)

Updated

6 years ago
Assignee: nobody → mdeboer
Created attachment 756556 [details] [diff] [review]
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.
Attachment #756556 - Flags: review?(mconley)

Comment 2

6 years ago
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.
(Reporter)

Comment 6

6 years ago
(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.
(Reporter)

Comment 7

6 years ago
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. :)
Attachment #756556 - Flags: review?(mconley) → feedback+
(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.
Created attachment 757377 [details] [diff] [review]
Make widgets use localization strings

Updated patch, with comments addressed.
Attachment #756556 - Attachment is obsolete: true
Attachment #757377 - Flags: review?(mconley)
(Reporter)

Comment 10

6 years ago
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...
Attachment #757377 - Flags: review?(mconley) → review-
(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...
(Reporter)

Comment 13

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

Ok, worth digging deeper into this.
Attachment #757377 - Flags: review- → review?(mconley)
(Reporter)

Comment 14

6 years ago
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.
Attachment #757377 - Flags: review?(mconley) → review+
Created attachment 757877 [details] [diff] [review]
Make widgets use localization strings

Updated patch, carrying over r=mconley.
Attachment #757377 - Attachment is obsolete: true
Attachment #757877 - Flags: review+

Updated

6 years ago
Whiteboard: [Australis:M6] → [Australis:M6][fixed-in-ux]
Blocks: 887911

Comment 17

5 years ago
https://hg.mozilla.org/mozilla-central/rev/7a4f2f5b3c6b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.