The default bug view has changed. See this FAQ.

Send button should be disabled until we have a recipient

RESOLVED FIXED in Thunderbird 23.0

Status

Thunderbird
Message Compose Window
--
minor
RESOLVED FIXED
9 years ago
8 months ago

People

(Reporter: wsmwk, Assigned: aceman)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs, {polish})

Trunk
Thunderbird 23.0
polish
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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

Comment 1

9 years ago
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.
Status: UNCONFIRMED → NEW
Depends on: 104973
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All

Updated

9 years ago
Duplicate of this bug: 433726
This seems like a good change and would fix bugs like bug 499137
(Assignee)

Comment 6

5 years ago
I am playing in the compose window in bug 271730. It seems I could do something here.
(Assignee)

Updated

5 years ago
Assignee: nobody → acelists
Version: unspecified → Trunk
(Assignee)

Updated

5 years ago
Depends on: 271730
(Assignee)

Updated

4 years ago
Blocks: 104973
No longer depends on: 104973
(Assignee)

Comment 7

4 years ago
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?
Attachment #690109 - Flags: ui-review?(bwinton)
Attachment #690109 - Flags: feedback?(mkmelin+mozilla)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED

Comment 8

4 years ago
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.
Attachment #690109 - Flags: feedback?(mkmelin+mozilla) → feedback-

Comment 9

4 years ago
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.
(Assignee)

Comment 10

4 years ago
(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.
Attachment #690109 - Flags: ui-review?(bwinton)
(Assignee)

Comment 12

4 years ago
Created attachment 717559 [details] [diff] [review]
patch v2

Ok, like this?
Attachment #690109 - Attachment is obsolete: true
Attachment #717559 - Flags: ui-review?(bwinton)
Attachment #717559 - Flags: review?(mkmelin+mozilla)
Comment on attachment 717559 [details] [diff] [review]
patch v2

Works for me.  Thanks!
Attachment #717559 - Flags: ui-review?(bwinton) → ui-review+
(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?
(Assignee)

Comment 15

4 years ago
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.
(Assignee)

Comment 17

4 years ago
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.
(Assignee)

Comment 18

4 years ago
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.
Attachment #717559 - Attachment is obsolete: true
Attachment #717559 - Flags: review?(mkmelin+mozilla)
Attachment #718648 - Flags: review?(mkmelin+mozilla)

Comment 19

4 years ago
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.
Attachment #718648 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 20

4 years ago
Created attachment 719147 [details] [diff] [review]
patch v4

Ok, with tests.
Attachment #718648 - Attachment is obsolete: true
Attachment #719147 - Flags: review?(mkmelin+mozilla)
Attachment #719147 - Flags: feedback?(mconley)
(Assignee)

Comment 21

4 years ago
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

4 years ago
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
Attachment #719147 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 23

4 years ago
Created attachment 723202 [details] [diff] [review]
patch v5
Attachment #719147 - Attachment is obsolete: true
Attachment #719147 - Flags: feedback?(mconley)
Attachment #723202 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 24

4 years ago
The new patch uses the new .hasRecipients attribute of nsIMsgComposeFields from bug 68784 so that must be applied first.
Depends on: 68784

Comment 25

4 years ago
Comment on attachment 723202 [details] [diff] [review]
patch v5

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

Sorry, this seems to have bitrotted.
Attachment #723202 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 26

4 years ago
It does apply cleanly if applied on top of bug 68784 per comment 24.
Flags: needinfo?(mkmelin+mozilla)

Comment 27

4 years ago
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
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 28

4 years ago
It does for me on current trunk.
Does your command fetch the latest version of the patch?
(Assignee)

Comment 29

4 years ago
Created attachment 727887 [details] [diff] [review]
patch v6

Or maybe the latest version I was using was not uploaded. Let's try this one.
Attachment #723202 - Attachment is obsolete: true
Attachment #727887 - Flags: review?(mkmelin+mozilla)

Comment 30

4 years ago
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
Attachment #727887 - Flags: review?(mkmelin+mozilla)

Comment 31

4 years ago
(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.)
(Assignee)

Comment 32

4 years ago
(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

4 years ago
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.
(Assignee)

Comment 34

4 years ago
Created attachment 728721 [details] [diff] [review]
patch v7
Attachment #727887 - Attachment is obsolete: true
Attachment #728721 - Flags: review?(mkmelin+mozilla)

Comment 35

4 years ago
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
Attachment #728721 - Attachment is obsolete: true
Attachment #728721 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 36

4 years ago
Created attachment 731557 [details] [diff] [review]
patch v8
Attachment #731557 - Flags: review?(mkmelin+mozilla)

Comment 37

4 years ago
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
Attachment #731557 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 38

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

Comment 39

4 years ago
Ah, your right

Comment 40

4 years ago
Comment on attachment 731557 [details] [diff] [review]
patch v8

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

Finally, r=mkmelin
Attachment #731557 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d9a1835283be
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0

Updated

4 years ago
Blocks: 360488
(Reporter)

Updated

4 years ago
Depends on: 863231
(Assignee)

Updated

3 years ago
Depends on: 933101
Depends on: 941139
(Assignee)

Updated

8 months ago
Depends on: 1290729
(Assignee)

Updated

8 months ago
Depends on: 1290733
(Assignee)

Updated

8 months ago
Depends on: 1075398
(Assignee)

Updated

8 months ago
Depends on: 1292097
You need to log in before you can comment on or make changes to this bug.