1578450 - with focus on attachment pane, Ctrl+Enter both tried to open the attachment and sends

Status: VERIFIED FIXED

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

Description

[Jarek]

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

Steps to reproduce:

I did following:

1. typed an email
2. inserted an attachment using drag and drop
3. pressed [Ctrl+Enter] to send it

Actual results:

• thunderbird tried to open the attachment
• at the same time it started sending the email

Expected results:

It should ONLY send the email, as always before.

Comment 1

[Jorg K (CEST = GMT+2)]

OK, if you're in the attachment pane, Ctrl+Enter or Ctrl+Shift+Enter sends and opens the attachment. If the attachment pane doesn't have focus, it works as before.

Alex, you wanted a regression, here it is. I don't think it's worth letting Alice find out where this changed, it's related to the de-XBL of that attachment list.

Comment 2

[Alessandro Castellani [:aleca]]

Attachment: 1578450-attachment-email.patch

This tiny change fixes the issue, but I'm not sure is the proper solution.

I've implemented an extra condition to check if the list has the ID "attachmentBucket" which is the ID assigned to the attachment list used in the compose dialog.

With this conditions, attachments can't be downloaded with CTRL+ENTER or CTRL+SHIFT+ENTER at all, which is the same behaviour we had in TB60.
Is this correct UX wise?
Are we safe to assume that no user will need to download its own attachments while composing a new message?

Comment 3

[Jarek]

When you want to download/open attachment while composing a new message, you can naturally use DOUBLECLICK or ENTER when it is focused.
So YES, you are save to assume that no user will need to download its own attachments while composing a new message with CTRL+ENTER or CTRL+SHIFT+ENTER.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9097745 [details] [diff] [review]
1578450-attachment-email.patch

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

::: mail/base/content/mailWidgets.js
@@ +1275,5 @@
>          if (event.keyCode == KeyEvent.DOM_VK_SPACE) {
>            this.moveByOffset(0, !event.ctrlKey, event.shiftKey);
>            event.preventDefault();
>          } else if (event.keyCode == KeyEvent.DOM_VK_RETURN) {
> +          if (this.currentItem && this.id != "attachmentBucket") {

we shouldn't be checking for ids. 

I think what is missing is an event.stopPropagation() or event.preventDefault()

I also don't understand why a new event is needed. Can't we just dispatch the original event?

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: 1578450-attachment-email.patch

I think what is missing is an event.stopPropagation() or event.preventDefault()

We can't as those methods will stop the event that sends the email.

I also don't understand why a new event is needed. Can't we just dispatch the original event?

We're not triggering any new event, the problem is that 2 events are getting triggered at once.

• Hitting ENTER on an attachment item in the compose window triggers the download.
• Hitting CTRL+ENTER or CTRL+SHIFT+ENTER in the compose window, also triggers the download of the attachment if one item has focus, instead it should only submit the message.

I think I found the solution by checking if the CTRL or SHIFT modifier keys are pressed.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9098352 [details] [diff] [review]
1578450-attachment-email.patch

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

Thx! r=mkmelin

Comment 7

[Alessandro Castellani [:aleca]]

I launched a try-run just to be sure I'm not messing up anything for the attachment panel: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d222d91c69e180d8db61ffb4b6456cff0b86641c

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/73754cd20bd8
Prevent opening attachments when sending an email with Ctrl+Enter. r=mkmelin

Comment 9

[Jorg K (CEST = GMT+2)]

TB 70 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/c0f2654f80ea358243b33e87618544034aa7610a

Comment 10

[Jorg K (CEST = GMT+2)]

Works well in TB 70 beta 3 (I hadn't tried it before).

Comment 11

[Alessandro Castellani [:aleca]]

Yay! :D

Comment 12

[Jorg K (CEST = GMT+2)]

TB 68.1.2 or TB 68.2 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/2832a28c6e26790a6b5f1250d47e817bbca870a6