about:addons uses extensions.properties for some UI strings. Those should be converted to use fluent, too.

Hi, i would be interested in working on this bug, how would I get started on this? Thanks!

(In reply to Ben Armstrong from comment #1) > Hi, i would be interested in working on this bug, how would I get started on > this? Thanks! OK! This bug is about converting some code to use our new localization framework called Fluent. There's a guide to fluent here: https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_tutorial.html . Migrating to this new framework happens in 4 parts: 1) copy strings to the new format (in this case, to aboutAddons.ftl, which already exists in https://searchfox.org/mozilla-central/source/toolkit/locales/en-US/toolkit/about/aboutAddons.ftl ). 2) update the JS that uses the strings to use the new ones instead. 3) write a python migration that maps the old strings to the new strings. This should basically create the same FTL contents that you add in (1), based on the old content in the .properties file. We need the script to update the other ~100 languages (mozilla-central only contains en-US, other people will run the script with all the other languages). 4) remove the old, now unused, strings from the .properties file. For this bug, we're interested in the uses of `extensions.properties` in the file https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js . It's defined as a bundle in https://searchfox.org/mozilla-central/rev/2edebf41a2b2e624859bf15ed5a98bdd002f73b4/toolkit/mozapps/extensions/content/extensions.js#72 . As you can see, this defines the `ext` property on `gStrings`, so elsewhere in this file it's accessed as `gStrings.ext`. As part of fixing this bug, I'm hoping that we can convert all of these uses and remove `gStrings.ext` completely. According to 'find in page', there are some 27 accesses. I've categorized them: - the ones in https://searchfox.org/mozilla-central/rev/2edebf41a2b2e624859bf15ed5a98bdd002f73b4/toolkit/mozapps/extensions/content/extensions.js#2816 updateState(). Here you can replace all the calls that change the contents of `warningLink` or "detail-error" or "detail-warning" etc. with ones that call `document.l10n.setAttributes()`, passing the node, the identifier of the string, and potentially (as the third argument) a JS object with a named variable to insert (like the name of the addon). You'd use effectively the same thing as the bottom example in https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_tutorial.html#runtime-localization , but you'd obviously adapt the name of the variable in the string (ie $addonName instead of $tabCount ). - the ones in the `getTooltip()` methods. Here it looks like the logic could just move into the `updateCommand` method elsewhere in the file. For the commands that have a `getTooltip` method, I'd just define a `tooltipL10nId` property. In `updateCommand`, you'd check if such a property exists and if `aAddon` is truthy, and if both conditions are met, call `document.l10n.setAttributes(cmdElt, cmd.tooltipL10nId)`, otherwise call `cmdElt.removeAttribute("data-l10n-id"); cmdElt.removeAttribute("tooltiptext");`. - the ones in setSearchLabel() ( https://searchfox.org/mozilla-central/rev/2edebf41a2b2e624859bf15ed5a98bdd002f73b4/toolkit/mozapps/extensions/content/extensions.js#281-291 ) and loadViewInternal should be straightforward to update to use document.l10n.setAttributes() instead of assigning to textContent (in the case where we use strings rather than reset to ""). You'll probably need to also remove the `data-l10n-id` attribute in the `else` case where we clear the textContent to ensure we re-translate correctly if the code re-runs and we take the other branch. I think that's most of the JS stuff. For the fluent side of things, hopefully the tutorial helps. Generally, where you're updating a node and the string ends up as `textContent` or innerHTML or similar, you can just define a string ID: foopy-string = My fancy string and use it with document.l10n.setAttributes(). Where we're using other attributes (like tooltiptext for the getTooltip() cases, and maybe "label" or "value" in other places), you'll want to define those in fluent using: foopy = .tooltiptext = My fancy tooltip etc. This part should be relatively straightforward. Let's start there - if you want to see an example of the migration scripts, see e.g. https://hg.mozilla.org/mozilla-central/file/tip/python/l10n/fluent_migrations/bug_1491677_extensions.py You'll want to create your own file in that directory with the bug number for this bug. As you can see there are a bunch of helper operations that should make this fairly straightforward - but I recognize that it might look a bit intimidating if you've not seen any of this before. Let me know if you have questions or if you get stuck anywhere!

Ah, hm, and the gStrings.ext property seems to also be used from: https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.xml#423 and a number of other callsites there. In principle, the same techniques should apply to convert those callsites.

Ok, cool! Thank you for the in-depth information on this bug and how everything works/relates to one another. I'll be giving it a go and I'll be sure to mesage you with any queries I have. Thanks!

Hi, I am just trying to wrap my head around this and have a few questions about everything. As far as my understanding of how the conversion process for this bug goes, if we take the first 'gstrings.ext' usage from extensions.js as an example which is searchLabel .textContent = gStrings.ext.GetStringFromName(`searchLabel.${type}`); We need to remove the reference to 'gStrings.ext' and convert it to use the fluent format which from my understanding for this case is done like so: document.l10n.setAttributes(searchLabel, "string reference defined in aboutAddons.ftl") And if takes the else branch we remove `data-l10n-id` attribute, as you mentioned, which from my understanding would happen via: searchLabel.removeAttribute("data-l10n-id"); Hopefully I have understood everything and I'm along the right lines of how to do this, but what I am completely unsure about is in part 1 where we have to copy the strings to aboutAddons.ftl so they can be referenced. How would it be done with this example: searchLabel .textContent = gStrings.ext.GetStringFromName(`searchLabel.${type}`); As we don't know the string name for the search label to insert into aboutAddons.ftl, do I need to dig into the 'GetStringFromName' function and find out what it would return to create the reference? Or have I misunderstood this? I'm very new to this, so thank you very much for your help and patience!

Can I also be assigned to this bug? Thanks!

(In reply to Ben Armstrong from comment #5) > Hi, I am just trying to wrap my head around this and have a few questions > about everything. > > As far as my understanding of how the conversion process for this bug goes, > if we take the first 'gstrings.ext' usage from extensions.js as an example > which is > > searchLabel > .textContent = gStrings.ext.GetStringFromName(`searchLabel.${type}`); > > We need to remove the reference to 'gStrings.ext' and convert it to use the > fluent format which from my understanding for this case is done like so: > > document.l10n.setAttributes(searchLabel, "string reference defined in > aboutAddons.ftl") > > And if takes the else branch we remove `data-l10n-id` attribute, as you > mentioned, which from my understanding would happen via: > > searchLabel.removeAttribute("data-l10n-id"); Yep! > Hopefully I have understood everything and I'm along the right lines of how > to do this, but what I am completely unsure about is in part 1 where we have > to copy the strings to aboutAddons.ftl so they can be referenced. > > How would it be done with this example: > > searchLabel > .textContent = gStrings.ext.GetStringFromName(`searchLabel.${type}`); > > As we don't know the string name for the search label to insert into > aboutAddons.ftl, do I need to dig into the 'GetStringFromName' function and > find out what it would return to create the reference? Or have I > misunderstood this? This case uses a JS template string ( https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals ). The result is passed to `GetStringFromName`, so there (fortunately!) isn't a need to dig into that function. Basically, the contents of any `${}` are evaluated as JS, so `type` is a variable/argument that is in scope at the point where the string is defined, and gets substituted in. You can try this out in the devtools web console, e.g. by evaluating: let kind = "apple"; console.log(`fruit-${kind}`) // logs "fruit-apple" In practice, it looks like `type` here can only ever be "theme" or "extension", given that there are only strings defined for those 2 values in https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties#114-115 . So you'd migrate those 2 strings to fluent, and could use the same call type from JS with the fluent API: document.l10n.setAttributes(searchLabel, `searchLabel-${type}`); (note that fluent id style is dash-separated-lower-case-only) > I'm very new to this, so thank you very much for your help and patience! No worries, let me know if this doesn't quite clarify things.

Thank you! This was very helpful once again. I think the part that was confusing me before was not knowing about the template_literals file which stored the possible strings for searchLabel. So if I were to convert these to the fluent format in aboutAddons.ftl I would do: search-label = .extension = Find more extensions .theme = Find more themes And then the js would be updated like so: document.l10n.setAttributes(searchLabel, `search-label.${type}`) I just want to make sure I understand it all before I start making changes.

(In reply to Ben Armstrong from comment #8) > Thank you! This was very helpful once again. > > I think the part that was confusing me before was not knowing about the > template_literals file which stored the possible strings for searchLabel. > > So if I were to convert these to the fluent format in aboutAddons.ftl I > would do: > > search-label = > .extension = Find more extensions > .theme = Find more themes > > And then the js would be updated like so: > > document.l10n.setAttributes(searchLabel, `search-label.${type}`) Not quite. In .properties files, '.' is an arbitrary separator. As in, I could define a string like: browser.part1.part2.part3.part4.somethingelse.foopy ... and where the 'foopy' part ends up being '.label' or '.accesskey', it's more or less only by convention - there's no semantic meaning. In fluent, we *only* use the `.foo` bits to specify attributes for DOM nodes. So if you had, say, an `<a>` HTML node (ie a link) in variable `myLink`, you could specify a `title` attribute (ie tooltip) by having this fluent: my-link-to-somewhere = .title = Some text for the tooltip and using JS as follows: document.l10n.setAttributes(myLink, "my-link-to-somewhere"); and Fluent will take care of setting the attributes (as defined with the '.foo' syntax) as attributes on the HTML node. So in this case, the 'searchLabel.extensions' and 'searchLabel.theme' are just 2 separate, arbitrary strings that happen to use a `.` as a separator in them, but in fluent you'd just use '-': search-label-extension = Find more extensions search-label-theme = Find more themes This will update the contents of the element when used with document.l10n.setAttributes. If, as a hypothetical example, you wanted to update their `label` attributes, the .ftl / fluent syntax would look like: search-extensions = .label = Find more extensions Does that make sense?

Ahh ok, that makes sense thanks! Originally I had it like this: search-label-extension = Find more extensions search-label-theme = Find more themes But I think I confused myself by not knowing about the '.' being an arbitrary separator and using the .properties file for comparison.

Hi, I've been working on this and have a just a few scenarios left (around 8) that I'm not quite sure how I should solve: For example scenarios like these: let name = gStrings.ext.GetStringFromName(`type.${haveUnsigned ? "unsupported" : "legacy"}.name`); this._categoryItem.setAttribute("name", name); this._categoryItem.tooltiptext = name; I can understand how to update "this._categoryItem.tooltiptext = name;" to use fluent but setAttribute is a method and uses the retrieved string as variable. This also occurs in other places like here for example: fp.init(window, gStrings.ext.GetStringFromName("installFromFile.dialogTitle"), nsIFilePicker.modeOpenMultiple); Where again it is used in a method, the docs say that you can just retrieve the string doing this: let [ msg ] = await document.l10n.formatValues([ {id: "remove-containers-description"} ]); alert(msg); But it notes that it is heavily discouraged. How would you recommend going about this? Also 'getToolTip()' methods have the same problem but you gave me a solution for that, forgive me but I don't fully understand your solution. You say to define a 'tooltipL10nId' property in 'getTooltip()' the current code for 'getToolTip()' is: getTooltip(aAddon) { if (!aAddon) return ""; return gStrings.ext.GetStringFromName("enableAddonTooltip"); }, What confuses me about this is how I set the property in the 'getTooltip()' functions? getTooltip(aAddon) { if (!aAddon) return; //don't set property so in updateCommand() logic we removeAttribute this.tooltipL10nId = "string reference"; //set property return; }, 'getTooltip()' is a method of cmd so my thought process is 'this.tooltipL10nId' would let me set the property on cmd but I feel like this is wrong. And again I am unsure of how to just get the string reference related to my previous question. One other quick question, are there unit tests I can run for this file to make sure I haven't made mistakes in my conversion? Thank you very much for your help!

(In reply to Ben Armstrong from comment #11) > Hi, I've been working on this and have a just a few scenarios left (around > 8) that I'm not quite sure how I should solve: > > For example scenarios like these: > > let name = gStrings.ext.GetStringFromName(`type.${haveUnsigned ? > "unsupported" : "legacy"}.name`); > > this._categoryItem.setAttribute("name", name); > > this._categoryItem.tooltiptext = name; > > I can understand how to update "this._categoryItem.tooltiptext = name;" to > use fluent but setAttribute is a method and uses the retrieved string as > variable. This also occurs in other places like here for example: In this particular case, because the `_categoryItem` is a DOM node, you can do the following: // This line allows fluent to mess with attributes that aren't in the default list of localizable // attributes. That list is necessary to avoid localizers / fluent being able to change e.g. the "id" // attribute on elements, thus breaking/changing behaviours of Firefox. In this case, the `name` attribute // is just used as a label, so having localization/fluent changing it is fine: this._categoryItem.setAttribute("data-l10n-attrs", "name"); // then just use setAttributes to set both the `name` and `tooltiptext` attributes: document.l10n.setAttributes(this._categoryItem, `addon-category-type-${haveUnsigned ? "unsupported" : "legacy"}`); In fluent, you'd do: addon-category-type-unsupported = .tooltiptext = Whatever the old tooltip was .name = { addon-category-type-unsupported.tooltiptext } Note that in the .ftl, I'm using a reference (the thing in {} ) to another string to avoid duplicating its contents. > > fp.init(window, > > > gStrings.ext.GetStringFromName("installFromFile.dialogTitle"), > > nsIFilePicker.modeOpenMultiple); > > Where again it is used in a method, the docs say that you can just retrieve > the string doing this: > > let [ msg ] = await document.l10n.formatValues([ > {id: "remove-containers-description"} > ]); > > alert(msg); > > But it notes that it is heavily discouraged. How would you recommend going > about this? It is indeed discouraged when the strings end up in the DOM, like they do in the `_categoryItem` case (as that's just an element in the about:addons / addon manager page). This is because when you use setAttributes, fluent has the semantic information it needs to switch languages on the fly - it can retranslate the DOM in a different language using the metadata that's in the DOM. If we instead just "manually" assign into attributes / element.textContent or similar, that's not possible, and the string will "stick out" if the user changes the Firefox language. For the filepicker case though, the file picker is an OS-native dialog, so it isn't possible to change its DOM or to have fluent change its content while it's up, or anything like that. So using `formatValues()` is the right approach. Note that to use `await` you will need to make the enclosing function async (ie inside the object at https://searchfox.org/mozilla-central/rev/dc3585e58b427c3a8a7c01712fe54ebe118a3ce2/toolkit/mozapps/extensions/content/extensions.js#1257 instead of `doCommand() { ... }` you'd use: `async doCommand() { ... }`. > Also 'getToolTip()' methods have the same problem but you gave me a solution > for that, forgive me but I don't fully understand your solution. You say to > define a 'tooltipL10nId' property in 'getTooltip()' the current code for > 'getToolTip()' is: > > getTooltip(aAddon) { > > if (!aAddon) > > return ""; > > return gStrings.ext.GetStringFromName("enableAddonTooltip"); > > }, > > What confuses me about this is how I set the property in the 'getTooltip()' > functions? > > getTooltip(aAddon) { > > if (!aAddon) > > return; //don't set property so in updateCommand() logic we > removeAttribute > > this.tooltipL10nId = "string reference"; //set property > return; > > }, > > 'getTooltip()' is a method of cmd so my thought process is > 'this.tooltipL10nId' would let me set the property on cmd but I feel like > this is wrong. And again I am unsure of how to just get the string reference > related to my previous question. Sure, I wasn't very clear. The entire command object for the example you cited looks like this: cmd_enableItem: { isEnabled(aAddon) { if (!aAddon) return false; let addonType = AddonManager.addonTypes[aAddon.type]; return (!(addonType.flags & AddonManager.TYPE_SUPPORTS_ASK_TO_ACTIVATE) && hasPermission(aAddon, "enable")); }, doCommand(aAddon) { if (aAddon.isWebExtension && !aAddon.seen && WEBEXT_PERMISSION_PROMPTS) { let perms = aAddon.userPermissions; if (perms.origins.length > 0 || perms.permissions.length > 0) { let subject = { wrappedJSObject: { target: getBrowserElement(), info: { type: "sideload", addon: aAddon, icon: aAddon.iconURL, permissions: perms, resolve() { aAddon.markAsSeen(); aAddon.enable(); }, reject() {}, }, }, }; Services.obs.notifyObservers(subject, "webextension-permission-prompt"); return; } } aAddon.enable(); }, getTooltip(aAddon) { if (!aAddon) return ""; return gStrings.ext.GetStringFromName("enableAddonTooltip"); }, }, I'd remove the `getTooltip` method on that object entirely (keeping `isEnabled` and `doCommand` ), and just define a separate static property on that object: cmd_enableItem: { // other stuff tooltipL10nId: "command-enable-addon", }, then in https://searchfox.org/mozilla-central/rev/dc3585e58b427c3a8a7c01712fe54ebe118a3ce2/toolkit/mozapps/extensions/content/extensions.js#1408-1421 , instead of: if ("getTooltip" in cmd) { let tooltip = cmd.getTooltip(aAddon); if (tooltip) cmdElt.setAttribute("tooltiptext", tooltip); else cmdElt.removeAttribute("tooltiptext"); } you could do: if ("tooltipL10nId" in cmd) { if (aAddon) { document.l10n.setAttributes(cmdElt, cmd.tooltipL10nId); } else { cmdElt.removeAttribute("data-l10n-id"); cmdElt.removeAttribute("tooltiptext"); } } Does that make sense? With that code, updateCommand should take care of adding/removing the tooltip, based on the l10n id specified by the command in question (if indeed the command specified an l10n id at all, much like the code as it is checks if the command has a `getTooltip` method). > One other quick question, are there unit tests I can run for this file to > make sure I haven't made mistakes in my conversion? This is a great question. Unit tests, I don't think so because the page pretty much requires a browser + rendered DOM to work, but there are some integration tests, so you could try running: ./mach mochitest toolkit/mozapps/extensions/test/browser . Note that I'm not super familiar with these tests - failures might not necessarily be the fault of your patch. So if there are any, commit your changes, and then update the working dir to a previous commit, rebuild (use `./mach build faster` to save time!) and re-run the tests - you can then verify if any issues are actually related to your patch. You can obviously also pass individual test paths to `./mach mochitest` to avoid having it run a bunch of passing tests that you're not interested in. > Thank you very much for your help! Thanks for working on this!

Hi Ben, did Gijs' comment help or do you still have some questions? Is there anything that we can help you with?

Hi, his comment was very helpful, unfortunately I came back home for the holidays from university and don't have access to my pc until the 8th where I can continue working on it again. I hope this is ok?

Yes, this is perfectly fine. Enjoy your break and let us know of any questions you have when you resume working on this.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6783c31ce412 Migrate messages from mozapps/extensions/extensions.properties to Fluent. r=Gijs,fluent-reviewers,robwu