Closed Bug 1681153 Opened 11 months ago Closed 9 months ago

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

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement

Tracking

(thunderbird_esr78 wontfix)

RESOLVED FIXED
87 Branch
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: k-l-p, Assigned: TbSync)

References

Details

Attachments

(2 files, 4 obsolete files)

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"

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

Assignee: nobody → john
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9192224 - Flags: feedback?(geoff)

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.

Attachment #9192224 - Flags: feedback?(geoff) → feedback+

Now with tests.

Attachment #9192224 - Attachment is obsolete: true
Attachment #9198204 - Flags: review?(geoff)
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: "commands_execute_compose_action@mochi.test",
> +      },
> +    },

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.)
Attachment #9198204 - Flags: review?(geoff) → feedback+

Updated the test.

Attachment #9198204 - Attachment is obsolete: true
Attachment #9198679 - Flags: review?(geoff)
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.
Attachment #9198679 - Flags: review?(geoff) → review-

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.

Attachment #9198679 - Attachment is obsolete: true
Attachment #9199545 - Flags: review?(geoff)

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.

Attachment #9199545 - Attachment is obsolete: true
Attachment #9199545 - Flags: review?(geoff)
Attachment #9199546 - Flags: review?(geoff)
Attachment #9199546 - Flags: review?(geoff) → review+

(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.

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

Target Milestone: --- → 87 Branch

Pushed by mkmelin@iki.fi:
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

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED

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

Status: RESOLVED → REOPENED
Flags: needinfo?(alessandro)
Resolution: FIXED → ---

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

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

Flags: needinfo?(alessandro)
Attachment #9200165 - Flags: review?(john)

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.

Attachment #9200165 - Flags: review?(john) → review+
Attachment #9199546 - Attachment description: bug1681153_add_support_for_execute_compose_action_and_execute_message_display_action_v5.patch → bug1681153_add_support_for_execute_compose_action_and_execute_message_display_action_v5.patch [LANDED ON C-C]
See Also: → 1689838

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/f2a474f546d0
Follow up fix test for macOS. r=TbSync

Status: REOPENED → RESOLVED
Closed: 9 months ago9 months ago
Resolution: --- → FIXED
Duplicate of this bug: 1689838
Duplicate of this bug: 1641580

Todo: Include shortcuts like _execute_compose_action in the documentation.

Flags: needinfo?(john)

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.

You need to log in before you can comment on or make changes to this bug.