620853 - Holding Ctrl+Enter a little too long causes unintentional confirmation of "Send Message?" prompt, and sends multiple copies of the message - only plain Enter (without modifier key) should confirm the prompt

Status: RESOLVED FIXED

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

Description

[Adam Nielsen]

User-Agent:       Mozilla/5.0 (X11; Linux i686 on x86_64; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-GB; rv:1.8.1.23) Gecko/20091130 Thunderbird/2.0.0.23 Mnenhy/0.7.5.0

There is a warning popup to prevent Ctrl+Enter from accidentally sending a message, however that popup's default button is "OK", meaning that the popup's purpose is defeated - if you accidentally hold down Ctrl+Enter the message will be sent without warning.

Reproducible: Always

Steps to Reproduce:
1. Compose a message
2. Hold Ctrl+Enter for a second or two
Actual Results:  
Message will be sent with no warning

Expected Results:  
Popup warning would appear and stay put

When you install software in Firefox there is a countdown on the 'Install' button, which is reset every time there is a keypress or mouse click.  Applying this to the send confirmation box would be ideal, as holding down Ctrl+Enter would simply reset the countdown until the keys are released.  Then pressing Enter again would send the message as it currently does.

Comment 1

[Wayne Mery (:wsmwk)]

iirc, there is a report in the bugzilla database that has resolution of WONTFIX for this issue. But perhaps the user presents a novel solution?

Comment 2

[Thomas D. (:thomas8)]

The behaviour described in comment 0 leads the confirmation prompt ad absurdum. Confirmation will cease to be effective when user just holds Ctrl+Enter a little too long, and msg might be sent against user's intention.

Think of users who might try to type a line break (or paragraph break, since we only do line breaks atm), and perhaps they (wrongly) try Ctrl+Enter for that purpose.

I agree with Wayne's comment 1 that introducing a waiting time like on FF install button would be wontfix; but I have a simple solution for this:

When confirmation prompt is shown, and user presses Enter WHILE modifier "Ctrl" is still held down by user, ignore that action. Only plain Enter (without Ctrl or other modifiers) should confirm the confirmation. I suppose this proposed behaviour would make sense even if we need to implement into toolkit or such.

Wayne?

Comment 3

[Wayne Mery (:wsmwk)]

(In reply to Thomas D. from comment #2)
> When confirmation prompt is shown, and user presses Enter WHILE modifier
> "Ctrl" is still held down by user, ignore that action. Only plain Enter
> (without Ctrl or other modifiers) should confirm the confirmation. I suppose
> this proposed behaviour would make sense even if we need to implement into
> toolkit or such.

I've been burnt by this. I agree

Comment 4

[Thomas D. (:thomas8)]

I just tried this on Windows XP and (perhaps due to server problems) got very confusing behaviour because even subsequent dialogues (a la "sending failed, try again?") will all be blindly confirmed as long as you keep holing Ctrl+Enter...

Comment 5

[Thomas D. (:thomas8)]

Or perhaps it even tries to send the same msg multiple times...? [EDIT: indeed, it will send multiple copies of the message as long as you keep Ctrl+Enter pressed].

Comment 6

[Rohan Jaswal]

Hi, I would like to work on this bug. I am a newbie to mozilla and open source too. could you please tell me the files associated with this bug and how to proceed?

Comment 7

[Thomas D. (:thomas8)]

Hi Rohan,

thanks for your offer to work on this bug - most welcome! :)

I won't do full mentoring on this, but here's a starting point:

Use send dialogue text to find string ID
http://mxr.mozilla.org/comm-central/search?string=ready%20to%20send%20this%20message

Use string ID to find the dialogue
http://mxr.mozilla.org/comm-central/search?string=sendMessageCheckLabel

String in dialogue is here:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2849

Dialogue call starts here:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2847
>       let buttonPressed = Services.prompt.confirmEx(window,

I suspect we can't fix this outside the confirmEx dialogue call, so we probably need to find the spot where the Services.prompt.confirmEx is defined as a function:

http://mxr.mozilla.org/comm-central/ident?i=confirmEx

After finding that prompt function, we'll have to check for modifier keys somehow before accepting Enter as confirmation, so that modifier+Enter does nothing and only plain Enter confirms (I think that makes sense for all such prompts).

Developer guide (mostly same for Firefox, but choose TB whereever possible)
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide

On your way to writing a patch, you ideally want:
https://developer.mozilla.org/en-US/docs/Simple_Thunderbird_build

Simple patches which don't need compiling might get away without building, so e.g. you rename omni.ja to omni.zip and then work on files in there before re-zipping and testing with your TB install.

A simple way to create a patch locally on Windows is using Tortoise Hg
http://tortoisehg.bitbucket.org/download/

Finally submit your patch (and more info on patches in links there)
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 8

[Thomas D. (:thomas8)]

Rohan, apart from comments here on this bug, you can also ask questions or ask for more mentoring on IRC
irc://irc.mozilla.org#maildev

Comment 9

[Rohan Jaswal]

Thanks for your help Thomas!
I am going through all the files you mentioned and studying the functions associated with this problem.
will surely ask you for more help. :)

Comment 10

[Rohan Jaswal]

Hi Thomas!
I was studying the code of thunderbird. This is an instruction written in:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2860

 >if (checkValue.value) {
 >          Services.prefs.setBoolPref("mail.warn_on_send_accel_key", false);

Could you please tell me what is the purpose of this instruction? What does string "mail.warn_on_send_accel_key" represent?

Comment 11

[Thomas D. (:thomas8)]

It's a user preference (pref) which can be set by user, Tools > Options > Composition > General > "Confirm when using keyboard shortcut to send message"

When set to true, and user presses Ctrl+Enter, we show the prompt asking to send or not.
If false, prompt is not shown.

Here's where the pref is found/read in code:
http://mxr.mozilla.org/comm-central/search?string=mail.warn_on_send_accel_key

And here is where it's stored:
http://mxr.mozilla.org/comm-central/source/mailnews/mailnews.js#576

For your bug here, the pref is not relevant because you only want to fix the dialogue (or the dialogue template), which is shown only when pref is true.

Comment 12

[Rohan Jaswal]

(In reply to Thomas D. from comment #7)
 
> After finding that prompt function, we'll have to check for modifier keys
> somehow before accepting Enter as confirmation, so that modifier+Enter does
> nothing and only plain Enter confirms (I think that makes sense for all such
> prompts).

How to accomplish this? What and where should I look in codebase for dealing with modifier keys(associated with this prompt).

Comment 13

[Magnus Melin [:mkmelin]]

I'd just like to point out this is likely not at all a good first bug. Unless passing in enableDelay happen to help. The code is probably here: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/prompts/src/nsPrompter.js#664

Comment 14

[Rohan Jaswal]

(In reply to Magnus Melin from comment #13)
> I'd just like to point out this is likely not at all a good first bug.
> Unless passing in enableDelay happen to help. The code is probably here:
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/
> prompts/src/nsPrompter.js#664

The code you are referring to, can we solve this problem by making changes there? As I don't see any instruction written there which could serve this purpose.

and please tell me what does "passing in EnableDelay" means?

Comment 15

[Thomas D. (:thomas8)]

Magnus, would a stacktrace help to find out the entire chain of functions which is executed here upon Ctrl+Enter keypress?
If yes, how can I get a stacktrace for that keypress event? (I know nothing about stacktraces so far...)

Magnus, would you agree that the desired behaviour for all such prompts is this?:

In the prompt, IF modifier key is held down while button is clicked/enter is pressed, just ignore that click/keypress and do nothing


(In reply to Rohan Jaswal from comment #12)
> (In reply to Thomas D. from comment #7)
>  
> > After finding that prompt function, we'll have to check for modifier keys
> > somehow before accepting Enter as confirmation, so that modifier+Enter does
> > nothing and only plain Enter confirms (I think that makes sense for all such
> > prompts).
> 
> How to accomplish this? What and where should I look in codebase for dealing
> with modifier keys(associated with this prompt).

Unfortunately it looks as if Magnus comment 13 is right, for this bug we'll have to dig so deep into the nested internals that it's probably not very suitable as a first/beginner's bug.

I don't know how to accomplish this, and the code is really nested deeply with so many calls, so it's hard to get down to where the the actual button clicks are received and where we could listen for modifier keys. But that's what's needed for this bug, I think.

Rohan, the plan is to continue what I started in comment 7, to track down all the nested code which runs when the prompt is shown and the user clicks the button.

So in comment 7 I stopped here:
let buttonPressed = Services.prompt.confirmEx(window,

So there's confirmEx function which calls the dialogue, so we have to find that function:
http://mxr.mozilla.org/comm-central/ident?i=confirmEx

Now that's where I gave up because there are several functions with the name confirmEx and it's hard to know which one is used in this case. But Magnus believes it might be this one (comment 13):

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/prompts/src/nsPrompter.js#664
664     confirmEx : function (title, text, flags, button0, button1, button2,
665                           checkLabel, checkValue) {
666 [...]
696         this.openPrompt(args);

So from there it goes on with openPrompt, so we need to continue searching for openPrompt function e.g. using mxr code search as I did in the examples shown...
openPrompt in turn will probably call some other function again... And somewhere down the line there must be code which closes the dialogue and produces return value when the button is clicked. That's where we might be able to change things, e.g. listen for the modifier key with something like onkeydown event and event.keycode (if it's javascript, but perhaps it's not), and annihilate that keypress if modifier key is pressed simultaneously (e.g. for javascript, something like event.stoppropagation()). So after event propagation is stopped, the dialogue should just stay open. I really only have a vague idea on this, we need other coders with more detailed knowledge.

The function calls are so deeply nested that I guess it probably needs getting a stacktrace (programmatic tracking list of all the functions as they are called here), but then we'd have to find out how to do that, and so far I don't know...

Plus I don't have much time atm...

Comment 16

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk) from comment #1)
> iirc, there is a report in the bugzilla database that has resolution of
> WONTFIX for this issue. But perhaps the user presents a novel solution?

Wayne, could you dig out that bug you were thinking of?
Might be worthwhile to see their arguments...
Although I think this definitely needs fixing for Thunderbird, at least on our side...
If we can't do it in the default prompt, perhaps we have to create our own xul dialogue...

Comment 17

[Wayne Mery (:wsmwk)]

I was unable to locate the bug I was thinking about.
Which would mean either the resolution changed, or I don't remember the search terms correctly

Comment 18

[Magnus Melin [:mkmelin]]

(In reply to Rohan Jaswal from comment #14)
> The code you are referring to, can we solve this problem by making changes
> there? As I don't see any instruction written there which could serve this
> purpose.

It can be fixed there if the solution has general value. Otherwise it should be fixed elsewhere. But frankly I don't know what the solution to this bug would be. 

> and please tell me what does "passing in EnableDelay" means?

The code does something special if you pass in an argument "enableDelay".

Anyway, should be easy enough to try out changing some things there (like always set enableDelay true) and see if you get further.

Comment 21

[Thomas D. (:thomas8)]

To fix this, I've filed toolkit Bug 1742707 - Default button on dialog widget triggers even when modifier keys are used, but should react to plain Enter keypress only. I offered a patch there. Let's keep this open to track the Thunderbird fallout.

Comment 23

[Alessandro Castellani [:aleca]]

Attachment: Bug 620853 - Handle compose send actions on keyup with the new shortcuts controller to avoid accidental long press sends. r=#thunderbird-reviewers

Comment 24

[Alessandro Castellani [:aleca]]

All right, I have a WIP that is mostly a proof of concept, but it works.
To fix this, the changes are fairly involved.

What I'm doing is:

• Convert the XUL key shortcut to a JavaScript based shortcut handled by our ShortcutsOverlay module.
• Use an HTML5 modal dialog for sending confirmation instead of a popup dialog.

Doing so will allow us to offer users the ability to disable the shortcut in the future if they want, but most importantly it allows us to detect the if Enter press is still a continuation of the previous one (repeat) and if it has modifiers like Ctrl or Shift when the confirmation modal is visible.
Detecting those situations, will allow us to prevent accidental "confirmation submission" on that modal.
We will be able to only accept a clean and single Enter press, without any modifier, on that confirmation dialog.

This should drastically mitigate accidental submissions on prolonged key presses.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6f01900108c0
Handle compose send actions on keyup with the new shortcuts controller to avoid accidental long press sends. r=arschmitz,mkmelin,tobyp

Comment 26

[Calum Mackay]

Possible regression? See bug 2073315

Comment 27

[Eugene Savitsky]

Regression https://bugzilla.mozilla.org/show_bug.cgi?id=2083823 on Windows and Linux.