Command+Enter with focus in autocomplete input of addressing area does not send message
Categories
(Thunderbird :: Message Compose Window, defect, P1)
Tracking
(thunderbird_esr78 fixed)
Tracking | Status | |
---|---|---|
thunderbird_esr78 | --- | fixed |
People
(Reporter: wsmwk, Assigned: aleca)
References
Details
Attachments
(1 file)
3.65 KB,
patch
|
mkmelin
:
review+
Paenglab
:
ui-review+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
I suspect this is my fault, not sure what caused the regression but I should be able to fix it.
Comment 2•4 years ago
|
||
We touched this in bug 1629144 and bug 1663583, but this side of the code looks fine to me. Must be something else.
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•4 years ago
|
||
Alice, could you find the regression window? That would be great!
Comment 4•4 years ago
|
||
(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.
Reporter | ||
Comment 5•4 years ago
|
||
I'll get it
Reporter | ||
Comment 6•4 years ago
|
||
Assignee | ||
Comment 7•4 years ago
•
|
||
I don't think Thomas has a mac, I'll take care of this.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 8•4 years ago
|
||
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.
Assignee | ||
Comment 9•4 years ago
|
||
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•4 years ago
|
||
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?
Comment 11•4 years ago
|
||
(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?
Assignee | ||
Comment 12•4 years ago
|
||
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•4 years ago
|
||
keycode is deprecated anyway:
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode
Comment 14•4 years ago
|
||
(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?
Assignee | ||
Comment 15•4 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Comment 16•4 years ago
|
||
Seems long-standing: https://searchfox.org/mozilla-central/rev/c7cf087b6e1384608ca3989f042f12f7cabd0a5f/toolkit/content/widgets/autocomplete-input.js#556-561
Assignee | ||
Comment 17•4 years ago
|
||
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.
Reporter | ||
Comment 18•4 years ago
|
||
(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.
Assignee | ||
Comment 19•4 years ago
|
||
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.
Reporter | ||
Comment 20•4 years ago
|
||
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.
Assignee | ||
Comment 21•4 years ago
|
||
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.
Assignee | ||
Comment 22•4 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Comment 23•4 years ago
|
||
Comment on attachment 9193652 [details] [diff] [review]
1682147-macos-send.diff
Works on Mac now.
Updated•4 years ago
|
Assignee | ||
Comment 24•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Comment 25•4 years ago
|
||
Pushed by mkmelin@iki.fi:
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
Updated•4 years ago
|
Comment 26•4 years ago
|
||
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.
Reporter | ||
Comment 27•4 years ago
|
||
Comment on attachment 9193652 [details] [diff] [review]
1682147-macos-send.diff
[Triage Comment]
Approved for esr78
Comment 28•4 years ago
|
||
bugherder uplift |
Thunderbird 78.7.1:
https://hg.mozilla.org/releases/comm-esr78/rev/6a385e266a8f
Updated•4 years ago
|
Reporter | ||
Comment 29•4 years ago
|
||
old news ... verified works in 86 beta
Updated•4 years ago
|
Description
•