1681153 - A browserAction button can be "clicked" by a keyboard shortcut, but a composeAction button cannot

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[C-E]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

in manifest.json:

"commands": {
"_execute_compose_action": {
"suggested_key": {
"default": "Ctrl+Shift+Y"
}
}
}

See also https://thunderbird.topicbox.com/groups/addons/Tf0e478d7b210e4dd

Actual results:

Nothing

Expected results:

composeAction button should be "clicked" by the keyboard shortcut "Ctrl+Shift+Y"

Comment 1

[John Bieling (:TbSync)]

Attachment: bug1681153_add_support_for_execute_compose_action_and_execute_message_display_action.patch

Before working on a test, I would like to know if this is the correct way to fix this.

Comment 2

[Geoff Lankow (:darktrojan)]

Comment on attachment 9192224 [details] [diff] [review]
bug1681153_add_support_for_execute_compose_action_and_execute_message_display_action.patch

Yeah, this is great, it's more-or-less what I was going to suggest anyway. I think there's a test for browserAction already that you can work with.

Comment 3

[John Bieling (:TbSync)]

Attachment: bug1681153_add_support_for_execute_compose_action_and_execute_message_display_action_v2.patch

Now with tests.

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9198204 [details] [diff] [review]
bug1681153_add_support_for_execute_compose_action_and_execute_message_display_action_v2.patch

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

Looking good. I'd just like the tests to cover some more potential cases.

::: mail/components/extensions/test/browser/browser_ext_commands_execute_compose_action.js
@@ +9,5 @@
> +    applications: {
> +      gecko: {
> +        id: "[e-mail]",
> +      },
> +    },

You don't need an ID unless you're going to look for it later. If you did this after looking at my recent changes, I only changed IDs where there was already one. In other cases a random string is used so no problem with conflicts there.

@@ +41,5 @@
> +        </html>
> +      `,
> +
> +      "popup.js": function() {
> +        console.log("sending from-compose-action-popup");

Use `info` for logging during tests. Console messages show up out-of-sync in test logs which makes them less useful than they could be.

@@ +128,5 @@
> +});
> +
> +add_task(async function test_execute_compose_action_without_popup() {
> +  await testExecuteComposeActionWithOptions();
> +});

I'd like this tested with `default_area: "formattoolbar"` too.

::: mail/components/extensions/test/browser/browser_ext_commands_execute_message_display_action.js
@@ +131,5 @@
> +});
> +
> +add_task(async function test_execute_message_display_action_without_popup() {
> +  await testExecuteMessageDisplayActionWithOptions();
> +});

Please can we test message displays in a new tab and a new window? It's a bit of extra work but may save regressions in the future.

::: mail/components/extensions/test/browser/head.js
@@ +293,5 @@
> +          { once: true }
> +        );
> +      },
> +    };
> +    Services.ww.registerNotification(observer);

I know you're just moving this code, but while you're at it can you change it to use a BrowserTestUtils function? `promiseAlertDialogOpen` or `domWindowOpened` with a check on the URL. (It's from the time before I knew about these functions.)

Comment 5

[John Bieling (:TbSync)]

Attachment: bug1681153_add_support_for_execute_compose_action_and_execute_message_display_action_v3.patch

Updated the test.

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9198679 [details] [diff] [review]
bug1681153_add_support_for_execute_compose_action_and_execute_message_display_action_v3.patch

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

You've put the format toolbar option in the wrong test. That's a composeAction thing.

Otherwise I think we're okay here.

::: mail/components/extensions/test/browser/browser_ext_commands_execute_message_display_action.js
@@ +38,5 @@
> +            <meta charset="utf-8">
> +            <script src="popup.js"></script>
> +          </head>
> +          <body>popup.js</body>
> +        </html>`,

Why does this file differ from the one in the other two tests?

::: mail/components/extensions/test/browser/head.js
@@ +313,5 @@
> +      );
> +      return true;
> +    }
> +  );
> +  window.MsgOpenNewWindowForMessage();

I recently learned this function takes an optional msgHdr argument and I think in future we should use it wherever possible instead of relying on the currently selected message. Likewise MailUtils.displayMessage instead of MsgOpenSelectedMessages.

I'm not going to force you to do it here because these tests don't care what the message is, but something to consider next time.

Comment 7

[John Bieling (:TbSync)]

Attachment: bug1681153_add_support_for_execute_compose_action_and_execute_message_display_action_v4.patch

The popup html files are identical now and the formattoolbar thing has been moved to the composeAction test.

I added the msg parameter to openNewWindowForMessage and used it in my test.

I did not touch MsgOpenSelectedMessages. I suggest to open a new bug for that (do you want a wrapper in head.js? How should it be called?) so we do not forget.

Edit: Patch was broken. Ignore.

Comment 8

[John Bieling (:TbSync)]

Attachment: bug1681153_add_support_for_execute_compose_action_and_execute_message_display_action_v5.patch [LANDED ON C-C]

The popup html files are identical now and the formattoolbar thing has been moved to the composeAction test.

I added the msg parameter to openNewWindowForMessage and used it in my test.

I did not touch MsgOpenSelectedMessages. I suggest to open a new bug for that (do you want a wrapper in head.js? How should it be called?) so we do not forget.

Comment 9

[Geoff Lankow (:darktrojan)]

(In reply to John Bieling (:TbSync) from comment #8)

I did not touch MsgOpenSelectedMessages. I suggest to open a new bug for that (do you want a wrapper in head.js? How should it be called?) so we do not forget.

There's nothing that needs to change unless you really want to, just something to think about when writing tests in future.

Comment 10

[Magnus Melin [:mkmelin]]

Please set tracking flags for the branches for what you intend to uplift or not.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/137d5a4c564a
add support for _execute_compose_action and _execute_message_display_action commands in commands API. r=darktrojan

Comment 12

[John Bieling (:TbSync)]

Fails the following test on mac:
mach test comm/mail/components/extensions/test/browser/browser_ext_commands_execute_compose_action.js

Comment 13

[John Bieling (:TbSync)]

It looks like the test is delayed at startup, so that it runs into the timeout. But why?
https://treeherder.mozilla.org/logviewer?job_id=328249166&repo=try-comm-central&lineNumber=3850

Comment 14

[Alessandro Castellani [:aleca]]

Attachment: 1681153-macos-followup.diff

The issue here is that the shortcut key needs a variation for macOS.
Alt on macOS doesn't really exist as it's Option, so shortcuts should be changed to use Ctrl which is actually the Command or metaKey.

The test pass locally, here's a try run to see if it works: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f3f336b153706234cef318d3c74e0b61a55ed4ab

Comment 15

[John Bieling (:TbSync)]

Comment on attachment 9200165 [details] [diff] [review]
1681153-macos-followup.diff

Thanks Alessandro for your help with this. In the long run I need a mac to be able to test locally.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f2a474f546d0
Follow up fix test for macOS. r=TbSync

Comment 19

[John Bieling (:TbSync)]

Todo: Include shortcuts like _execute_compose_action in the documentation.

Comment 20

[John Bieling (:TbSync)]

• https://webextension-api.thunderbird.net/en/78/commands.html
• https://webextension-api.thunderbird.net/en/latest/commands.html

Comment 21

[Magnus Melin [:mkmelin]]

Uplift to 78?

Comment 22

[John Bieling (:TbSync)]

Before requesting uplift, I wanted to make sure it is upliftable, as we have made some serious changes to the tests. I had this scheduled for next week.