Command description is not localized

NEW
Unassigned

Status

P5
normal
3 years ago
2 months ago

People

(Reporter: wbamberg, Unassigned, Mentored)

Tracking

({good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [commands][triaged])

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8751464 [details]
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.
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.

Updated

3 years ago
Whiteboard: [commands]

Comment 2

3 years ago
we don't use descriptions yet - they are shown in prefferences where you can reassign key bindings.
Whiteboard: [commands] → [commands] good first bug triaged
(Reporter)

Comment 3

3 years ago
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.)

Updated

2 years ago
Mentor: kmaglione+bmo
Keywords: good-first-bug
Whiteboard: [commands] good first bug triaged → [commands][good first bug][triaged]

Updated

2 years ago
Whiteboard: [commands][good first bug][triaged] → [commands][triaged]

Updated

2 years ago
Component: WebExtensions: Untriaged → WebExtensions: General
Priority: -- → P5

Comment 4

2 years ago
Hi,

If nobody is working on this, can I take up this bug?
Flags: needinfo?(kmaglione+bmo)
Sure! Let me know if you have any questions
Assignee: nobody → anejaalisha
Flags: needinfo?(kmaglione+bmo)

Comment 6

2 years ago
Apologies for such a basic question. But how to exactly reproduce this bug?
Flags: needinfo?(kmaglione+bmo)
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/﷒0﷓
2) http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_getAll.js
Flags: needinfo?(kmaglione+bmo)

Updated

2 years ago
Mentor: kmaglione+bmo → tomica
Hey @anejaalish! Just checking in to see how you are doing with this bug. :)

Comment 9

2 years ago
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

2 years ago
How do I check if the localization for a command exists?
Flags: needinfo?(tomica)
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/﷒0﷓
2) http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_getAll.js
Flags: needinfo?(tomica)

Comment 12

2 years ago
'preprocess' needs to be added in commands.json file line 78?
Flags: needinfo?(tomica)
Yes, that looks about right.
Flags: needinfo?(tomica)

Comment 14

a year ago
Created attachment 8877657 [details] [diff] [review]
Bug1272130.patch
Attachment #8877657 - Flags: feedback?(tomica)
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.
Attachment #8877657 - Flags: feedback?(tomica) → feedback+

Comment 16

a year ago
Created attachment 8880685 [details] [diff] [review]
Bug1272130.patch

Kindly review the patch.
Attachment #8877657 - Attachment is obsolete: true
Flags: needinfo?(mixedpuppy)
Attachment #8880685 - Flags: review?(tomica)
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()`.
Attachment #8880685 - Flags: review?(tomica)
As zombie mentioned, looks like the test wont work yet.
Flags: needinfo?(mixedpuppy) → needinfo?(anejaalisha)

Comment 19

a year ago
Created attachment 8883072 [details] [diff] [review]
Bug1272130.patch

I think I have made the correct changes, but the tests still don't pass :/
Attachment #8880685 - Attachment is obsolete: true
Flags: needinfo?(anejaalisha)
Attachment #8883072 - Flags: feedback?(tomica)
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?
Attachment #8883072 - Flags: feedback?(tomica)

Comment 21

a year ago
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.
Flags: needinfo?(tomica)
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/﷒0
Flags: needinfo?(tomica)

Comment 23

a year ago
Created attachment 8884618 [details] [diff] [review]
Bug1272130.patch
Attachment #8883072 - Attachment is obsolete: true
Attachment #8884618 - Flags: review?(tomica)
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.
Attachment #8884618 - Flags: review?(tomica)

Comment 25

a year ago
Created attachment 8886871 [details] [diff] [review]
Bug1272130.patch
Attachment #8884618 - Attachment is obsolete: true
Attachment #8886871 - Flags: review?(tomica)
Attachment #8886871 - Flags: review?(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 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
Attachment #8886871 - Flags: review?(tomica)
anejaalisha, Just checking in since this is still in my review queue.
Flags: needinfo?(anejaalisha)

Comment 29

a year ago
Yes, I am submitting the updated patch.
Flags: needinfo?(anejaalisha)

Comment 30

a year ago
Created attachment 8891289 [details] [diff] [review]
Bug1272130.patch
Attachment #8886871 - Attachment is obsolete: true
Attachment #8886871 - Flags: review?(mixedpuppy)
Attachment #8891289 - Flags: review?(tomica)
Attachment #8891289 - Flags: review?(mixedpuppy)
Comment on attachment 8891289 [details] [diff] [review]
Bug1272130.patch

># HG changeset patch
># User Alisha <anejaalisha@yahoo.com>
>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.
Attachment #8891289 - Flags: review?(mixedpuppy) → review+
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?
Attachment #8891289 - Flags: review?(tomica)

Comment 33

a year ago
Created attachment 8892053 [details] [diff] [review]
Bug1272130.patch
Attachment #8891289 - Attachment is obsolete: true
Attachment #8892053 - Flags: review?(tomica)
Attachment #8892053 - Flags: review?(mixedpuppy)
Attachment #8892053 - Flags: review?(mixedpuppy) → review+
Attachment #8892053 - Flags: review?(tomica) → review+
Hi Alisha, feel free to add "checking-needed" keyword if/once the Try push looks ok.

Comment 35

a year ago
(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?
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
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.
Flags: needinfo?(anejaalisha)

Comment 38

a year ago
I will check it again and submit the updated patch.
Flags: needinfo?(anejaalisha)

Comment 39

a year ago
Where are the 'message' and 'description' switched?
Flags: needinfo?(tomica)
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`).
Flags: needinfo?(tomica)

Comment 41

a year ago
Created attachment 8903153 [details] [diff] [review]
Bug1272130.patch
Attachment #8892053 - Attachment is obsolete: true
Attachment #8903153 - Flags: review?(tomica)
Attachment #8903153 - Flags: review?(mixedpuppy)

Comment 42

a year ago
Created attachment 8903154 [details] [diff] [review]
Bug1272130.patch
Attachment #8903153 - Attachment is obsolete: true
Attachment #8903153 - Flags: review?(tomica)
Attachment #8903153 - Flags: review?(mixedpuppy)
Attachment #8903154 - Flags: review?(tomica)
Attachment #8903154 - Flags: review?(mixedpuppy)
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?
Attachment #8903154 - Flags: review?(tomica)
Attachment #8903154 - Flags: review?(mixedpuppy)
Hey anejaalisha! How's it going with this bug?
Flags: needinfo?(anejaalisha)

Comment 45

a year ago
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.
Flags: needinfo?(anejaalisha) → needinfo?(cneiman)
No worries! I'll unassign for now but please feel free to come back once your exams are over. Good luck!
Assignee: anejaalisha → nobody
Flags: needinfo?(cneiman)

Updated

5 months ago
Product: Toolkit → WebExtensions
Is it still open? Looks like it just needs some code formatting? I can work on that.
Flags: needinfo?(tomica)
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?
Flags: needinfo?(tomica)
You need to log in before you can comment on or make changes to this bug.