Last Comment Bug 431217 - Send button should be disabled until we have a recipient
: Send button should be disabled until we have a recipient
Status: RESOLVED FIXED
: polish
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: All All
: -- minor with 2 votes (vote)
: Thunderbird 23.0
Assigned To: :aceman
:
:
Mentors:
: 433726 (view as bug list)
Depends on: 941139 1075398 68784 271730 863231 933101 1290729 1290733 1292097
Blocks: 104973 TB2SM
  Show dependency treegraph
 
Reported: 2008-04-28 13:18 PDT by Wayne Mery (:wsmwk, NI for questions)
Modified: 2016-08-04 03:18 PDT (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (11.96 KB, patch)
2012-12-08 09:40 PST, :aceman
mkmelin+mozilla: feedback-
Details | Diff | Splinter Review
patch v2 (12.28 KB, patch)
2013-02-23 15:38 PST, :aceman
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v3 (12.73 KB, patch)
2013-02-26 14:05 PST, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
patch v4 (16.42 KB, patch)
2013-02-27 13:17 PST, :aceman
no flags Details | Diff | Splinter Review
patch v5 (14.94 KB, patch)
2013-03-10 08:19 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v6 (14.97 KB, patch)
2013-03-21 14:00 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v7 (15.65 KB, patch)
2013-03-24 06:02 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v8 (15.63 KB, patch)
2013-03-30 15:01 PDT, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review

Description Wayne Mery (:wsmwk, NI for questions) 2008-04-28 13:18:09 PDT
From SM Bug 104973 – Send button should be disabled until we have a recipient
Comment 1 Josh Geenen (:pi) 2008-05-15 14:30:40 PDT
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
Comment 2 Mark Banner (:standard8) 2008-05-15 14:37:33 PDT
(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?
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2008-05-16 14:45:11 PDT
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.
Comment 4 Magnus Melin 2008-07-13 09:48:37 PDT
*** Bug 433726 has been marked as a duplicate of this bug. ***
Comment 5 Bryan Clark (DevTools PM) [@clarkbw] 2009-06-18 17:05:15 PDT
This seems like a good change and would fix bugs like bug 499137
Comment 6 :aceman 2012-10-06 10:14:28 PDT
I am playing in the compose window in bug 271730. It seems I could do something here.
Comment 7 :aceman 2012-12-08 09:40:15 PST
Created attachment 690109 [details] [diff] [review]
patch

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 8 Magnus Melin 2012-12-09 02:42:51 PST
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 9 Magnus Melin 2012-12-09 02:48:44 PST
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.
Comment 10 :aceman 2012-12-09 04:48:07 PST
(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 11 Blake Winton (:bwinton) (:☕️) 2013-02-02 19:17:53 PST
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.
Comment 12 :aceman 2013-02-23 15:38:02 PST
Created attachment 717559 [details] [diff] [review]
patch v2

Ok, like this?
Comment 13 Blake Winton (:bwinton) (:☕️) 2013-02-24 10:33:52 PST
Comment on attachment 717559 [details] [diff] [review]
patch v2

Works for me.  Thanks!
Comment 14 WADA 2013-02-25 18:23:15 PST
(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?
Comment 15 :aceman 2013-02-25 23:47:43 PST
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.
Comment 16 WADA 2013-02-26 00:36:07 PST
(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.
Comment 17 :aceman 2013-02-26 02:47:41 PST
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.
Comment 18 :aceman 2013-02-26 14:05:25 PST
Created attachment 718648 [details] [diff] [review]
patch v3

So you were right, WADA! It didn't work in case the addresses were prefilled automatically. This patch fixes that.
Comment 19 Magnus Melin 2013-02-27 11:32:02 PST
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.
Comment 20 :aceman 2013-02-27 13:17:45 PST
Created attachment 719147 [details] [diff] [review]
patch v4

Ok, with tests.
Comment 21 :aceman 2013-02-28 02:55:33 PST
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 22 Magnus Melin 2013-03-05 13:14:02 PST
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
Comment 23 :aceman 2013-03-10 08:19:17 PDT
Created attachment 723202 [details] [diff] [review]
patch v5
Comment 24 :aceman 2013-03-10 08:20:33 PDT
The new patch uses the new .hasRecipients attribute of nsIMsgComposeFields from bug 68784 so that must be applied first.
Comment 25 Magnus Melin 2013-03-21 13:05:22 PDT
Comment on attachment 723202 [details] [diff] [review]
patch v5

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

Sorry, this seems to have bitrotted.
Comment 26 :aceman 2013-03-21 13:31:20 PDT
It does apply cleanly if applied on top of bug 68784 per comment 24.
Comment 27 Magnus Melin 2013-03-21 13:37:52 PDT
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
Comment 28 :aceman 2013-03-21 13:57:38 PDT
It does for me on current trunk.
Does your command fetch the latest version of the patch?
Comment 29 :aceman 2013-03-21 14:00:42 PDT
Created attachment 727887 [details] [diff] [review]
patch v6

Or maybe the latest version I was using was not uploaded. Let's try this one.
Comment 30 Magnus Melin 2013-03-23 04:21:22 PDT
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
Comment 31 Magnus Melin 2013-03-23 04:23:10 PDT
(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.)
Comment 32 :aceman 2013-03-23 06:36:53 PDT
(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.
Comment 33 Magnus Melin 2013-03-23 12:39:39 PDT
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 34 :aceman 2013-03-24 06:02:46 PDT
Created attachment 728721 [details] [diff] [review]
patch v7
Comment 35 Magnus Melin 2013-03-30 05:58:23 PDT
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 36 :aceman 2013-03-30 15:01:58 PDT
Created attachment 731557 [details] [diff] [review]
patch v8
Comment 37 Magnus Melin 2013-03-31 11:36:57 PDT
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
Comment 38 :aceman 2013-03-31 12:39:21 PDT
It does apply fine for me (on top of 68784).
Comment 39 Magnus Melin 2013-03-31 13:27:58 PDT
Ah, your right
Comment 40 Magnus Melin 2013-04-01 02:53:36 PDT
Comment on attachment 731557 [details] [diff] [review]
patch v8

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

Finally, r=mkmelin
Comment 41 Ryan VanderMeulen [:RyanVM] 2013-04-17 04:39:09 PDT
https://hg.mozilla.org/comm-central/rev/d9a1835283be

Note You need to log in before you can comment on or make changes to this bug.