Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Command description is not localized

NEW
Assigned to

Status

()

Toolkit
WebExtensions: General
P5
normal
a year ago
2 days ago

People

(Reporter: wbamberg, Assigned: anejaalisha, Mentored)

Tracking

({good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [commands][triaged])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

a year 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.
Whiteboard: [commands]

Comment 2

a year 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

a year 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.)
Mentor: kmaglione+bmo@mozilla.com
Keywords: good-first-bug
Whiteboard: [commands] good first bug triaged → [commands][good first bug][triaged]

Updated

a year ago
Whiteboard: [commands][good first bug][triaged] → [commands][triaged]

Updated

9 months ago
Component: WebExtensions: Untriaged → WebExtensions: General
Priority: -- → P5
(Assignee)

Comment 4

5 months ago
Hi,

If nobody is working on this, can I take up this bug?
Flags: needinfo?(kmaglione+bmo)

Comment 5

5 months ago
Sure! Let me know if you have any questions
Assignee: nobody → anejaalisha
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 6

5 months 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/extensions/ext-commands.js#70
2) http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_getAll.js
Flags: needinfo?(kmaglione+bmo)

Updated

3 months ago
Mentor: kmaglione+bmo@mozilla.com → tomica@gmail.com
Hey @anejaalish! Just checking in to see how you are doing with this bug. :)
(Assignee)

Comment 9

2 months 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.
(Assignee)

Comment 10

2 months 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/schemas/browser_action.json#19
2) http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_getAll.js
Flags: needinfo?(tomica)
(Assignee)

Comment 12

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

Comment 14

a month 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+
(Assignee)

Comment 16

28 days 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)
(Assignee)

Comment 19

18 days 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)
(Assignee)

Comment 21

16 days 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/browser/browser_ext_commands_getAll.js#41
Flags: needinfo?(tomica)
(Assignee)

Comment 23

12 days 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)
(Assignee)

Comment 25

5 days 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)
You need to log in before you can comment on or make changes to this bug.