1682147 - Command+Enter with focus in autocomplete input of addressing area does not send message

Status: VERIFIED FIXED

(Thunderbird :: Message Compose Window, defect, P1)

Description

[Wayne Mery (:wsmwk)]

84 beta. With address and subject field populated and cursor in address field, command+enter has no effect. Pretty sure this worked in the past. Sends fine on Windows with 84 beta

Comment 1

[Alessandro Castellani [:aleca]]

I suspect this is my fault, not sure what caused the regression but I should be able to fix it.

Comment 2

[Thomas D. (:thomas8)]

We touched this in bug 1629144 and bug 1663583, but this side of the code looks fine to me. Must be something else.

https://searchfox.org/comm-central/rev/5640a40e0d1d800e461050adadc46e29bdbe3982/mail/components/compose/content/addressingWidgetOverlay.js#703-711

    case "Enter":
      // If no address entered, move focus to the next available element,
      // but not for Ctrl/Cmd+[Shift]+Enter keyboard shortcuts for sending.
      if (!input.value.trim() && !event.ctrlKey && !event.metaKey) {
        // Prevent Enter from firing again on the element we move the focus to.
        event.preventDefault();
        SetFocusOnNextAvailableElement(input);
      }
      break;

Comment 3

[Thomas D. (:thomas8)]

Alice, could you find the regression window? That would be great!

Comment 4

[Alice0775 White]

(In reply to Thomas D. (:thomas8) from comment #3)

Alice, could you find the regression window? That would be great!

Sorry, I do not have a Mac.

Comment 5

[Wayne Mery (:wsmwk)]

I'll get it

Comment 6

[Wayne Mery (:wsmwk)]

https://hg.mozilla.org/comm-central/pushloghtml?fromchange=2719fb20cee2145deb05a6bfff8b30e6d326fde9&tochange=1805540a69e4c271b7bb6f2eae47797f5ecba7d0

Comment 7

[Alessandro Castellani [:aleca]]

I don't think Thomas has a mac, I'll take care of this.

Comment 8

[Alessandro Castellani [:aleca]]

I'm not sure that method is what causing the issue.
I tried removing all those conditions and interacting with plain input fields, but the result is the same.

I'm looking at our commits that touched composer files but nothing comes up.

Comment 9

[Alessandro Castellani [:aleca]]

The command+enter works if the cursor is anywhere else, even if it's on one of the dynamically generated input fields coming from extra headers.
It doesn't fire exclusively when the cursor is on one of the static fields.
Mhhhh

Comment 10

[Thomas D. (:thomas8)]

Thanks for taking care of this, Alex!
I'm not sure how my patch of bug 1663583 could have caused this, because I was explicitly enabling command-enter to reach its destination for sending shortcut.

The only thing which I noticed is that the two <keys> for this use observes attribute, whereas almost everything else uses command. Maybe observes reacts different on MAC?

https://searchfox.org/comm-central/rev/6f9eb5629f4dd83979f22a9851b17ff10650eb53/mail/components/compose/content/messengercompose.xhtml#403-404

Comment 11

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani (:aleca) from comment #9)

The command+enter works if the cursor is anywhere else, even if it's on one of the dynamically generated input fields coming from extra headers.
It doesn't fire exclusively when the cursor is on one of the static fields.
Mhhhh

Well, there are two main differences between static and extra header fields: pills vs. plaintext, autocomplete vs. no autocomplete.
Could the autocomplete checks for Enter hijack this on Mac?

Comment 12

[Alessandro Castellani [:aleca]]

Indeed, I think there's something going on with the keycode="VK_RETURN" as changing that key to another letter to do a CMD+* combination test works.

Comment 13

[Thomas D. (:thomas8)]

keycode is deprecated anyway:

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode

Comment 14

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani (:aleca) from comment #12)

Indeed, I think there's something going on with the keycode="VK_RETURN" as changing that key to another letter to do a CMD+* combination test works.

Could you file a link to the source of this?

Comment 15

[Alessandro Castellani [:aleca]]

It's definitely an issue with the <html:input is="autocomplete-input"/>.
I created that simple element without any of our event listeners or special conditions attached, and the CMD+Enter doesn't trigger anything.

Could you file a link to the source of this?

I simply replaced keycode="VK_RETURN" with key="Y" as a test, and that works.
So I think something in the autocomplete is hijacking the RETURN keystroke, but only on macOS, and I'm 99% sure it's not from our c-c code.

I wonder if this is happening only on Big Sur.
Pinging Emilio for some info as he worked on the autocomplete listeners in the past.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Seems long-standing: https://searchfox.org/mozilla-central/rev/c7cf087b6e1384608ca3989f042f12f7cabd0a5f/toolkit/content/widgets/autocomplete-input.js#556-561

Comment 17

[Alessandro Castellani [:aleca]]

AH!
So this probably never worked for us?

Pretty sure this worked in the past.

Wayne, are you sure about this?
Doesn't work on 78.

Comment 18

[Wayne Mery (:wsmwk)]

(In reply to Alessandro Castellani (:aleca) from comment #17)

AH!
So this probably never worked for us?

Pretty sure this worked in the past.

Wayne, are you sure about this?
Doesn't work on 78.

I was referring to beta. I cannot speak to version 78 - I'm using only beta on the Mac. But I am certain of the regression range I provided.
Also, I am not using Big Sur.

Comment 19

[Alessandro Castellani [:aleca]]

Are you able to download beta 83 and test it?
I doubt this ever worked as the autocomplete fields block that.

The shortcut works if your cursor is on any other field or the body, so probably that's what gave you the sense of it working before.
I backed out the changes implemented in the bug you highlighted as regression, but the result is the same.

Comment 20

[Wayne Mery (:wsmwk)]

I don't recall that I retested 83. And to be clear - I did not bisect betas. I bisected nightly builds using the mozregression tool, hitting multiple nightly builds, so I am quite certain the regression range cited in comment 6 is accurate. But not certain about the cause of the regression.

Comment 21

[Alessandro Castellani [:aleca]]

Attachment: 1682147-macos-send.diff

This fixes the issue and it's a macOS only alteration.

I know Magnus and Thomas don't have a mac and can't test it, but I'd like to have a review on the logic for the fix.
Apparently, it seems that the autocomplete input have always blocked any action if the macOS meta key is pressed, as stated in Comment 16.
So, the XUL <key> to send a message with CMD+Enter works except if the cursor is currently focus on an autocomplete addressing field.

I'm pinging Richard for a ui-r since the creation of a pill makes the container grow by a 0.5 px and it's very weird.

Comment 22

[Alessandro Castellani [:aleca]]

I tested this on a couple of nightlies and also 68 and this has been the expected behavior, even before the de-xbl.
So, this is not a regression.
I'm surprised we didn't have many more bug reports about it.

Comment 23

[Richard Marti (:Paenglab)]

Comment on attachment 9193652 [details] [diff] [review]
1682147-macos-send.diff

Works on Mac now.

Comment 24

[Alessandro Castellani [:aleca]]

Comment on attachment 9193652 [details] [diff] [review]
1682147-macos-send.diff

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

Removing Thomas' review request since he's on PTO.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8574e5a6a2cd
Fix CMD+Enter shortcut not sending message in compose window when focused on an input field. r=Paenglab

Comment 26

[Thomas D. (:thomas8)]

Comment on attachment 9193652 [details] [diff] [review]
1682147-macos-send.diff

[Approval Request Comment]
Regression caused by (bug #): Bug 1674054 - Non-pillified recipients silently discarded when clicking [Send] button directly
User impact if declined: Bug 1688336 - Users cannot send messages with custom header
Testing completed (on c-c, etc.): Think so, see Paenglab's comment 23.
Risk to taking this patch (and alternatives if risky): low.

Per my analysis in Bug 1688336 Comment 9:

Bug 1674054 introduced a significant regression:

• Bug 1688336 - Cannot send messages with custom header

This bug 1682147 has de facto (intentionally?) bandaid-fixed that regression for Daily/Trunk, but not for TB 78.

Can we uplift this into TB 78 to fix bug 1688336 also on release channel asap?
I'm not sure why this hasn't been uplifted already, but anyway, if there was need for a better reason, Bug 1688336 has it.

Comment 27

[Wayne Mery (:wsmwk)]

Comment on attachment 9193652 [details] [diff] [review]
1682147-macos-send.diff

[Triage Comment]
Approved for esr78

Comment 28

[Rob Lemley [:rjl]]

Thunderbird 78.7.1:
https://hg.mozilla.org/releases/comm-esr78/rev/6a385e266a8f

Comment 29

[Wayne Mery (:wsmwk)]

old news ... verified works in 86 beta