From SM Bug 104973 – Send button should be disabled until we have a recipient

I can verify this on Thunderbird version 3.0a2pre (2008051403) also in 32-bit Ubuntu 8.04, although the bug probably exists on all platforms. Possible duplicate (alternative solution): bug 433726 Steps to Reproduce: 1. Make a new message leaving the recipient blank (the content--if any--of the subject and message are irrelevant) 2. Click Send 3. An error message appears after the Sending Message window appears: "Sending of message failed. No recipients were specified. Please enter a recipient or newsgroup in the addressing area" Possible solutions: Bug 433726 - Verify the presence of recipients earlier (before the Sending Messages window appears with "Assembling Mail Information") This bug - Disable send button until a valid recipient is present

(In reply to comment #0) > From SM Bug 104973 – Send button should be disabled until we have a recipient > Wayne, given bug 104973 is now in core, can this be duped?

I'm not a great fan of duping thunderbird bugs, esp. UI-type, to core bugs - for reasons of searchability and not knowing if the fixing that bug will get this one. (hence my reason for filing this bug) But I don't pretend to understand the code either. So if someone has a reason that makes sense for changing from dependency to a dupe that's fine with me.

This seems like a good change and would fix bugs like bug 499137

I am playing in the compose window in bug 271730. It seems I could do something here.

So this works for me. There is one open problem. Previously if the address was invalid, the user clicked Send and got the message which address is invalid. Now he can't click it so may wonder why it is not enabled. How can we solve this? What about adding the message to the Send button tooltip? Or something better?

Comment on attachment 690109 [details] [diff] [review] patch Review of attachment 690109 [details] [diff] [review]: ----------------------------------------------------------------- Yeah this can't require a valid address, it needs to check if anything at all is entered. If there's something is entered in addresses it should be enabled.

Comment on attachment 690109 [details] [diff] [review] patch Review of attachment 690109 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/MsgComposeCommands.js @@ +2928,5 @@ > +function updateSendLock() > +{ > + gSendLocked = true; > + if (!gMsgCompose) > + return; I think it's not worth checking for this, better let it blow up if need be. @@ +2971,5 @@ > return false; > } > + > + if (aTo.length == 0 && aCc.length == 0 && aBcc.length == 0) > + return false; This won't work out, there's news, and also other address types like Reply-To that are valid.

(In reply to Magnus Melin from comment #9) > > +function updateSendLock() > > +{ > > + gSendLocked = true; > > + if (!gMsgCompose) > > + return; > > I think it's not worth checking for this, better let it blow up if need be. This is a must, because this is called very soon when gMsgCompose is not yet initialized. It would always blow up for the first invocation. > @@ +2971,5 @@ > > return false; > > } > > + > > + if (aTo.length == 0 && aCc.length == 0 && aBcc.length == 0) > > + return false; > > This won't work out, there's news, and also other address types like > Reply-To that are valid. We could include them in the expression too. (In reply to Magnus Melin from comment #8) > Yeah this can't require a valid address, it needs to check if anything at > all is entered. If there's something is entered in addresses it should be > enabled. Enabling Send button when typing the first character would make this patch unneded I think.

Comment on attachment 690109 [details] [diff] [review] patch (In reply to :aceman from comment #10) > (In reply to Magnus Melin from comment #8) > > Yeah this can't require a valid address, it needs to check if anything at > > all is entered. If there's something is entered in addresses it should be > > enabled. > Enabling Send button when typing the first character would make this patch > unneded I think. Can we turn this patch into that patch reasonably easily? (I'm clearing the ui-review request until you and Magnus decide what we want to do here, but the general idea of disabling the Send button seems like a good one.) Thanks, Blake.

Ok, like this?

Comment on attachment 717559 [details] [diff] [review] patch v2 Works for me. Thanks!

(In reply to Blake Winton (:bwinton) from comment #13) > patch v2 > + * Keep the Send buttons disabled until any recipient is entered. Question about edge case. When auto-BCC or auto-CC is used, is "send to the auto-BCC only or the auto-CC only with no To:/CC:/BCC: at composition window" possible?

WADA, where is the auto-CC feature? If you mean that one from the account manager, then I believe it just prefills those fields when the compose window is opened, so that you can see the values there. So in that case To:/CC:/BCC: will not be empty as see by the compose window code.

(In reply to :aceman from comment #15) I meant CC/BCC setting in Copies&Folders, and I could see pre-filled CC/BCC field in composition window. Sorry for my confusion.

But you have a point that I need to check with the patch applied, if the prefilling does trigger the event to recheck if Send must still be disabled. The patch triggers it only on keypresses or changes of the To/Bcc/Reply-to/newsgroup dropdown.

So you were right, WADA! It didn't work in case the addresses were prefilled automatically. This patch fixes that.

Comment on attachment 718648 [details] [diff] [review] patch v3 Review of attachment 718648 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r=mkmelin, but this needs tests too to land. it's not something we want to ever be broken.

Ok, with tests.

I can make the test better, it now checks the commands but not if the button is really connected to that command. I'll add that.

Comment on attachment 719147 [details] [diff] [review] patch v4 Review of attachment 719147 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/test/mozmill/composition/test-send-button.js @@ +12,5 @@ > +const MODULE_REQUIRES = ["folder-display-helpers", "compose-helpers", "window-helpers"]; > +var elib = {}; > +Components.utils.import("resource://mozmill/modules/elementslib.js", elib); > + > +var composeHelper = null; no need for this. it's kosher to call the methods without "composeHelper." @@ +23,5 @@ > + fdh.installInto(module); > + composeHelper = collector.getModule("compose-helpers"); > + composeHelper.installInto(module); > + let wh = collector.getModule("window-helpers"); > + wh.installInto(module); collector.getModule("folder-display-helpers").installInto(module); collector.getModule("window-helpers").installInto(module); collector.getModule("compose-helpers").installInto(module); @@ +43,5 @@ > +/** > + * Check if the send buttons are in the wished state. > + * > + * @param aCwc The compose window controller. > + * @param aState The expected state of the buttons. True = enabled, false = disabled. why not aEnabled? @@ +54,5 @@ > +} > + > +/** > + * Bug 431217 > + * Test that the Send buttons are properly enabled only if any addressee is input. any -> an @@ +72,5 @@ > + subtest_check_send_buttons_state(cwc, false); > + > + composeHelper.close_compose_window(cwc); > + > + // Set the prefs to prefill a default CC address when Compose is opened. new test function from here on

The new patch uses the new .hasRecipients attribute of nsIMsgComposeFields from bug 68784 so that must be applied first.

Comment on attachment 723202 [details] [diff] [review] patch v5 Review of attachment 723202 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, this seems to have bitrotted.

It does apply cleanly if applied on top of bug 68784 per comment 24.

No, it doesn't. magnus@magnus-laptop:/opt/comm-central/src$ hg qimport bz:68784 Fetching... done Parsing... done adding 68784 to series file renamed 68784 -> 68784.patch magnus@magnus-laptop:/opt/comm-central/src$ hg qpush applying 68784.patch now at: 68784.patch magnus@magnus-laptop:/opt/comm-central/src$ hg qimport bz:431217 Fetching... done Parsing... done adding 431217 to series file renamed 431217 -> 431217.patch magnus@magnus-laptop:/opt/comm-central/src$ hg qpush applying 431217.patch patching file mail/components/compose/content/MsgComposeCommands.js Hunk #9 FAILED at 2905 1 out of 9 hunks FAILED -- saving rejects to file mail/components/compose/content/MsgComposeCommands.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 431217.patch

It does for me on current trunk. Does your command fetch the latest version of the patch?

Or maybe the latest version I was using was not uploaded. Let's try this one.

Comment on attachment 727887 [details] [diff] [review] patch v6 Review of attachment 727887 [details] [diff] [review]: ----------------------------------------------------------------- Yes it applies now. Looks ok to me. However, with this and the most recent patch from bug 68784 applied, actually SENDING doesn't work! The button gets enabled, but atually sending gives you the "not a valid address" error message. I assume that's a problem with the other patch though. ::: mail/components/compose/content/messengercompose.xul @@ +819,1 @@ > <menupopup> This is getting a bit crowded, maybe add an onAddressColCommand function to do it all.. @@ +835,5 @@ > completedefaultindex="true" forcecomplete="true" > minresultsforpopup="3" ignoreblurwhilesearching="true" > ontextentered="awRecipientTextCommand(eventParam, this)" > onerrorcommand="awRecipientErrorCommand(eventParam, this)" > + oninput="gContentChanged=true; setupAutocomplete(); updateSendCommands(true);" ... and something similar for this? ::: mail/test/mozmill/composition/test-send-button.js @@ +10,5 @@ > + > +const RELATIVE_ROOT = "../shared-modules"; > +const MODULE_REQUIRES = ["folder-display-helpers", "compose-helpers", "window-helpers"]; > + > +var cwc = null; // compose window controller Please make this a local let variable, everywhere you use it. No need for it to be global @@ +39,5 @@ > + * > + * @param aCwc The compose window controller. > + * @param aEnabled The expected state of the commands. > + */ > +function subtest_check_send_commands_state(aCwc, aEnabled) { test_ instead of subtest_ @@ +91,5 @@ > + > + // Press backspace to remove the recipient. No other valid one is there, > + // disable Send. > + cwc.e("addressCol2#1").select(); > + cwc.keypress(null, 'VK_BACK_SPACE', {}); nit: prefer double quotes

(In reply to :aceman from comment #28) > It does for me on current trunk. > Does your command fetch the latest version of the patch? The qimport extension gets the latest non-obsolated patch. And if there are many you get a prompt for which. (I beleive the people who help check things in use it too.)

(In reply to Magnus Melin from comment #30) > @@ +39,5 @@ > > + * > > + * @param aCwc The compose window controller. > > + * @param aEnabled The expected state of the commands. > > + */ > > +function subtest_check_send_commands_state(aCwc, aEnabled) { > > test_ instead of subtest_ Are you sure? I had the impression that all functions starting with test_ will be executed as standalone tests. And this function can't be run standalone.

Uh, yes that was me misreading. But in that case i'd propose to just remove the subtest_ part from the name, as it's just a helper function and not really a test at all.

Comment on attachment 728721 [details] [diff] [review] patch v7 Sorry but it doesn't apply. Bitrotted again, or what? pplying 431217.patch patching file mail/components/compose/content/MsgComposeCommands.js Hunk #1 FAILED at 50 Hunk #9 FAILED at 2906 2 out of 10 hunks FAILED -- saving rejects to file mail/components/compose/content/MsgComposeCommands.js.rej

Comment on attachment 731557 [details] [diff] [review] patch v8 Review of attachment 731557 [details] [diff] [review]: ----------------------------------------------------------------- patching file mail/components/compose/content/MsgComposeCommands.js Hunk #9 FAILED at 2897 1 out of 10 hunks FAILED -- saving rejects to file mail/components/compose/content/MsgComposeCommands.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 431217.patch

It does apply fine for me (on top of 68784).

Ah, your right

Comment on attachment 731557 [details] [diff] [review] patch v8 Review of attachment 731557 [details] [diff] [review]: ----------------------------------------------------------------- Finally, r=mkmelin