1272130 - Command description is not localized

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P5)

Description

[Will Bamberg [:wbamberg]]

Attachment: commands-l10n.zip

It should be possible for developers to provides localized versions of the "description" field in commands.

The "description" field for commands should be localized if it's referenced with a string of the form "__MSG_commandDescription__", and "commandDescription" exists in a messages.json file under the _locales directory.

For example, I've attached a zip containing an add-on with a _locales directory a containing messages.json that contains:

  "commandDescription": {
    "message": "Does stuff",
    "description": "Thingy."
  }

Then in the manifest.json the commands key is:

  "commands": {
    "toggle-feature": {
      "suggested_key": { "default": "Ctrl+Shift+U" },
      "description": "__MSG_commandDescription__"
    }
  }

If you install the add-on you'll see output like this in the Browser Console:

    __MSG_commandDescription__

In Chrome you get:

    Does stuff

This seems quite important, since AFAICT the only purpose of "description" is to show the user.

Comment 1

[Kris Maglione [:kmag]]

This is definitely something we should fix, but I'm not sure it's especially important at the moment. The command description definitely exists to be shown to the user, but at the moment, we don't actually show it anywhere. In Chrome, there's a configuration UI for extension commands where the field is used. We don't have anything similar, at the moment.

Comment 2

[:shell escalante]

we don't use descriptions yet - they are shown in prefferences where you can reassign key bindings.

Comment 3

[Will Bamberg [:wbamberg]]

It's true that Firefox doesn't have a native UI for assigning shortcuts, but an add-on could build its own UI for them.

Enabling an add-on to have its own UI for shortcuts is the only reason I can imagine that the description is accessible to add-ons via the JS API at all.

(just to clarify this, not to argue with your triage.)

Comment 4

[anejaalisha]

Hi,

If nobody is working on this, can I take up this bug?

Comment 5

[Kris Maglione [:kmag]]

Sure! Let me know if you have any questions

Comment 6

[anejaalisha]

Apologies for such a basic question. But how to exactly reproduce this bug?

Comment 7

[Tomislav Jovanovic :zombie]

This is more like an "enhancement" than a "bug", so there are no easy steps to "reproduce".

The point is that that we don't localize the command descriptions the way Will described in comment #0.  So you would need to modify the loadCommandsFromManifest() method [1], in order to also read the localized descriptions (if present).  Also, you would need to add a test for new functionality in [2].

1) http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-commands.js#70
2) http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_getAll.js

Comment 8

[Caitlin Neiman [:caitmuenster]]

Hey @anejaalish! Just checking in to see how you are doing with this bug. :)

Comment 9

[anejaalisha]

Hi, sorry was a bit caught up on university assignments. Yup I am again starting to look into resolving this. Thanks for checking in though.

Comment 10

[anejaalisha]

How do I check if the localization for a command exists?

Comment 11

[Tomislav Jovanovic :zombie]

Oh, it seems that my comment 7 wasn't exactly correct.  Looks like you should be able to simply add "preprocess": "localize" to the schema for the command description, like for browser_action [1], so this bug could be only about testing that it works as expected in [2].

1) http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/browser_action.json#19
2) http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_getAll.js

Comment 12

[anejaalisha]

'preprocess' needs to be added in commands.json file line 78?

Comment 13

[Tomislav Jovanovic :zombie]

Yes, that looks about right.

Comment 14

[anejaalisha]

Attachment: Bug1272130.patch

Comment 15

[Tomislav Jovanovic :zombie]

Comment on attachment 8877657 [details] [diff] [review]
Bug1272130.patch

Review of attachment 8877657 [details] [diff] [review]:
-----------------------------------------------------------------

This look good.

You can ask for review (and also add :mixedpuppy) if the tests pass on Try run.

::: browser/components/extensions/test/browser/browser_ext_commands_getAll.js
@@ +99,5 @@
> +          command = commands.find(c => c.name == "with-localization");
> +
> +          browser.test.assertEq("Localized description", command.description, "The description should match the localized version of what is provided in the manifest");
> +
> +          browser.test.assertEq("Ctrl+Shift+Y", command.shortcut, "The shortcut should match the default shortcut provided in the manifest");

nit: this is already tested, don't think we need it here again.

Comment 16

[anejaalisha]

Attachment: Bug1272130.patch

Kindly review the patch.

Comment 17

[Tomislav Jovanovic :zombie]

Comment on attachment 8880685 [details] [diff] [review]
Bug1272130.patch

Review of attachment 8880685 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't look like it would work in this state because of the `files` key.  Did you run the test, either locally or on Try?

::: browser/components/extensions/test/browser/browser_ext_commands_getAll.js
@@ +35,5 @@
> +          "suggested_key": {
> +            "default": "Ctrl+Shift+Y",
> +          },
> +          "description": "__MSG_commandDescription__",
> +          "default_locale": "en",

Shouldn't this key be at the root of the manifest?

@@ +36,5 @@
> +            "default": "Ctrl+Shift+Y",
> +          },
> +          "description": "__MSG_commandDescription__",
> +          "default_locale": "en",
> +          "files": {

And I think `files` only works as a key outside of the manifest, part of the object passed into `loadExtension()`.

Comment 18

[Shane Caraveo (:mixedpuppy)]

As zombie mentioned, looks like the test wont work yet.

Comment 19

[anejaalisha]

Attachment: Bug1272130.patch

I think I have made the correct changes, but the tests still don't pass :/

Comment 20

[Tomislav Jovanovic :zombie]

Comment on attachment 8883072 [details] [diff] [review]
Bug1272130.patch

What's the error reported?  Does it throw or fail a test check?  Can you give more details?

Comment 21

[anejaalisha]

8 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_commands_getAll.js | getAll should return an array of commands - Expected: 6, Actual: 5 - 
Stack trace:
    @moz-extension://68875325-d927-4e6b-b732-4f9155a8bcad/%7B26aa2a81-49ed-4a7d-86d2-421037acc110%7D.js:5:11
    Async*@moz-extension://68875325-d927-4e6b-b732-4f9155a8bcad/%7B26aa2a81-49ed-4a7d-86d2-421037acc110%7D.js:3:9

The test fails.

Comment 22

[Tomislav Jovanovic :zombie]

Well, you obviously added another command, so the length of the array returned from commands.getAll() is not 5 anymore, but 6 instead.  You should be able to just update the expected value for the check here:

http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_getAll.js#41

Comment 23

[anejaalisha]

Attachment: Bug1272130.patch

Comment 24

[Tomislav Jovanovic :zombie]

Comment on attachment 8884618 [details] [diff] [review]
Bug1272130.patch

Review of attachment 8884618 [details] [diff] [review]:
-----------------------------------------------------------------

The code itself seems good, but the diff looks weird, you have one file twice, and the review part should be "r?zombie" until you get a r+.

Please add :mixedpuppy for final review after you fix the diff.

Comment 25

[anejaalisha]

Attachment: Bug1272130.patch

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8886871 [details] [diff] [review]
Bug1272130.patch


>     background: function() {
>       browser.test.onMessage.addListener((message, additionalScope) => {
>         browser.commands.getAll((commands) => {
>           let errorMessage = "getAll should return an array of commands";
>-          browser.test.assertEq(commands.length, 5, errorMessage);
>+          browser.test.assertEq(commands.length, 6, errorMessage);
> 
>           let command = commands.find(c => c.name == "with-desciption");

Please add a test for the with-localization command to verify the description is correct.

Comment 27

[Tomislav Jovanovic :zombie]

Comment on attachment 8886871 [details] [diff] [review]
Bug1272130.patch

Review of attachment 8886871 [details] [diff] [review]:
-----------------------------------------------------------------

As Shane said, you're missing the test, and you already had it in an earlier patch: 

https://bugzilla.mozilla.org/attachment.cgi?id=8877657&action=diff#a/browser/components/extensions/test/browser/browser_ext_commands_getAll.js_sec3

Comment 28

[Shane Caraveo (:mixedpuppy)]

anejaalisha, Just checking in since this is still in my review queue.

Comment 29

[anejaalisha]

Yes, I am submitting the updated patch.

Comment 30

[anejaalisha]

Attachment: Bug1272130.patch

Comment 31

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8891289 [details] [diff] [review]
Bug1272130.patch

># HG changeset patch
># User Alisha <[e-mail]>
>Bug 1272130 - Command description is not localized. r=zombie

>-add_task(async function() {
>+add_task(function* () {

>-  await extension.startup();
>-  await extension.awaitMessage("ready");
>+  yield extension.startup();
>+  yield extension.awaitMessage("ready");
>   extension.sendMessage("additional-scope", {platform: AppConstants.platform});
>-  await extension.awaitFinish("commands");
>-  await extension.unload();
>+  yield extension.awaitFinish("commands");
>+  yield extension.unload();


Lets keep this using async/await.  r+ once that is done.

Comment 32

[Tomislav Jovanovic :zombie]

Comment on attachment 8891289 [details] [diff] [review]
Bug1272130.patch

Review of attachment 8891289 [details] [diff] [review]:
-----------------------------------------------------------------

What Shane said.  Hm, was that on purpose, or did you apply your patch on top of an old commit?

Comment 33

[anejaalisha]

Attachment: Bug1272130.patch

Comment 34

[Tomislav Jovanovic :zombie]

Hi Alisha, feel free to add "checking-needed" keyword if/once the Try push looks ok.

Comment 35

[anejaalisha]

(In reply to Tomislav Jovanovic :zombie from comment #34)
> Hi Alisha, feel free to add "checking-needed" keyword if/once the Try push
> looks ok.

How it is to be done?

Comment 36

[Tomislav Jovanovic :zombie]

Sorry I didn't see this earlier.  Here is how you push to try:

https://wiki.mozilla.org/ReleaseEngineering/TryServer

ans since you might not have commit access, I pushed for you:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f191c86dab4c03cbdba58acc4b4b3f94d068ae49

Comment 37

[Tomislav Jovanovic :zombie]

This is failing on Try.  Did you run the test locally?

You seem to have switched the localized `message` and `description`.  Also, there's a eslint failure because your "files" key is wrongly indented.

Comment 38

[anejaalisha]

I will check it again and submit the updated patch.

Comment 39

[anejaalisha]

Where are the 'message' and 'description' switched?

Comment 40

[Tomislav Jovanovic :zombie]

here is the error message from the above Try run:

    The description should match the localized version of what is provided in the 
    manifest - Expected: localized description, Actual: Does stuff 

That means that the `message` part of the localized description is used instead of the description, so you need to put your expected value there (or change the expected value to what is in the `message`).

Comment 41

[anejaalisha]

Attachment: Bug1272130.patch

Comment 42

[anejaalisha]

Attachment: Bug1272130.patch

Comment 43

[Tomislav Jovanovic :zombie]

Comment on attachment 8903154 [details] [diff] [review]
Bug1272130.patch

Review of attachment 8903154 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/test/browser/browser_ext_commands_getAll.js
@@ +46,5 @@
> +            "message": "Does stuff",
> +            "description": "localized description",
> +          },
> +        },
> +      },

This still doesn't look properly indentented.  Did you run eslint (and the test) locally, or submitted to Try?

Comment 44

[Caitlin Neiman [:caitmuenster]]

Hey anejaalisha! How's it going with this bug?

Comment 45

[anejaalisha]

Hey Caitlin, I am little busy with my exams atm. The latest attachment is almost the fix for the bug. If anyone else wants to work on it, please feel free to assign to them.

Comment 46

[Caitlin Neiman [:caitmuenster]]

No worries! I'll unassign for now but please feel free to come back once your exams are over. Good luck!

Comment 47

[Arshad Kazmi [:arshadkazmi42]]

Is it still open? Looks like it just needs some code formatting? I can work on that.

Comment 48

[Caitlin Neiman [:caitmuenster]]

Hey Arshad! You've been crushing these WebExtensions bugs. :) We'd like to hold on to the remaining good-first-bugs to help onboard other contributors who would like to get involved. 

It looks like you're ready to tackle some more challenging stuff. Can you give me a day or so to find some good-second-bugs for you?

Comment 50

[Mark Striemer [:mstriemer]]

This was fixed in bug 1303384 (part 4).