933101 - Send button disabled when write a message with to, subject and content, if mail address is Dropped at outside of address text input box

Status: RESOLVED FIXED

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

Description

[Arthur Tam]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

steps:
- write a message
- drag an email from address book
- type in subject and content
- Send button still disabled


Actual results:

Send button disabled when write a message with to, subject and content


Expected results:

Send button should enable as the new message already with to, subject and content

Comment 1

[Arthur Tam]

work-around: type a character in address field and clear it will have Send button turn to enable

Comment 2

[Thomas D. (:thomas8)]

Aceman, I'm quite sure that the algorithm that disables send buttons etc isn't complete yet.
E.g. changing From from a sender without automatic bcc to a sender *with* automatic bcc will not enable send, but should. Any other cases that we might have missed here?

Unfortunately, I could not reproduce reporter's drag case on tb24/winxp (but perhaps we need more detail).

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Possible fix without test

Comment 4

[:aceman]

(In reply to Thomas D. from comment #2)
So it looks like <textbox id="addressCol2#1"> and co can be changed without firing oninput and onchange. We need to find out what other events can be fired on drag'n'drop or when the fields are automatically filled in as Thomas found in comment 2.

I can look at this.
The test file for this is at http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/composition/test-send-button.js .

Comment 5

[WADA:World Anti-bad-Duping Agency]

I can't reproduce problem with Tb 24.1.0 on Win-XP.
Send button was enabled by any of following.
  Type mail address,
  Copy&Paste mail addr text, Drag&Drop mail addr text
  Drag&Drop contact from Address Book, Drag&Drop contact from Side Bar
  Add to To:/CC:/BCC: of Side Bar
Change back Hardware:/OS: to original.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to WADA from comment #5)
> I can't reproduce problem with Tb 24.1.0 on Win-XP.
> Send button was enabled by any of following.
>   Type mail address,
>   Copy&Paste mail addr text, Drag&Drop mail addr text
>   Drag&Drop contact from Address Book, Drag&Drop contact from Side Bar

Try to drop a contact from address book to the second (or third) of recipient fields.

Comment 7

[Thomas D. (:thomas8)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to WADA from comment #5)
> > I can't reproduce problem with Tb 24.1.0 on Win-XP.
> > Send button was enabled by any of following.
> >   Type mail address,
> >   Copy&Paste mail addr text, Drag&Drop mail addr text
> >   Drag&Drop contact from Address Book, Drag&Drop contact from Side Bar
> 
> Try to drop a contact from address book to the second (or third) of
> recipient fields.

Thanks Hiroyuki, that was the missing hint for reliable reproduction of this bug.

STR to reproduce drag&drop failure on WinXP/TB24.1.0:

1 compose with AB sidebar, start with empty address widget (Send disabled)
2 drag address from sidebar and drop anywhere in the header, but *outside* those recipient rows which already show a contact item. E.g., drop on subject label, or on blank recipient row *without* contact icon.
-> Send still disabled (but should be enabled)

STR for scenario of comment 2 (other failure on WinXP/TB24.1.0):

1 compose with from-sender *without* automatic CC/BCC, start with empty address widget (Send disabled)
2 change from-sender to account/identity *with* automatic CC/BCC which gets auto-filled into first row(s) of address widget
-> Send still disabled (but should be enabled)

Unless shown otherwise, let's assume this will fail on all OS, and it's probably what comment 0 saw.

Comment 8

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #7)
> STR to reproduce drag&drop failure on WinXP/TB24.1.0:
> 1 compose with AB sidebar, start with empty address widget (Send disabled)
> 2 drag address from sidebar and drop anywhere in the header, but *outside*
> those recipient rows which already show a contact item.

/contact icon/

> E.g., drop on
> subject label, or on blank recipient row *without* contact icon.
> -> Send still disabled (but should be enabled)

Comment 9

[:aceman]

Thanks, I can see both the problems (Win XP). This should be OS independent.

(Dropping on subject line does fill in the subject, not any recipient so it does not seem relevant here.)

Comment 10

[:aceman]

Hiro, would you like to continue on this further, or can I take it as it was caused by my change?

Comment 11

[WADA:World Anti-bad-Duping Agency]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Try to drop a contact from address book to the second (or third) of recipient fields.
Gotcha!
  Drag&Drop to row where type selection box(To:/CC:/BCC:/Reply-To:) is not shown yet.
  (If more than 3 addresss are Drag&Droped, Drag&Srop to bottom row of no selection box)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to :aceman from comment #10)
> Hiro, would you like to continue on this further, or can I take it as it was
> caused by my change?

Yes, please take it. I am always hoping that someone fixes bug while I am sleeping. ;-p

Comment 13

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #9)
> Thanks, I can see both the problems (Win XP). This should be OS independent.
> 
> (Dropping on subject line does fill in the subject, not any recipient so it
> does not seem relevant here.)

Fwiw, yes, but dropping address from AB on subject *label* (the word "Subject" in UI) *will* fill recipient field, where the subject label is just part of the whole header area outside type-defined recipient fields.

Comment 14

[WADA:World Anti-bad-Duping Agency]

FYI.

(In reply to :aceman from comment #4)
> So it looks like <textbox id="addressCol2#1"> and co can be changed
> without firing oninput and onchange
Even when Drop is done at inside of <textbox id="addressCol2#N">(N=1, 2, 3, ...), if Drop is done on image(person icon) which is placed at left side of the textbox, Send button is not enabled.

  DOM structure.
    listbox id="addressingWidget"
      xul:listcols
      listcols
      xul:listrows
      xul:listboxbody
       (ordinal row)
        listitem   class="addressingWidgetItem" 
          listcell class="addressingWidgetCell"
            menulist id="addressCol1#N" (N=1, 2, 3, ...)
          listcell
            textbox  id="addressCol2#N" (N=1, 2, 3, ...)
              xul:hbox
              image
              xul:hbox <= rectangular box contains html:input. Call addressCol2#N-InputBox
                html:input
       (dummy row)
        listitem   class="dummy-row"
          listcell class="addressingWidgetCell dummy-row-cell"
          listcell class="addressingWidgetCell dummy-row-cell"

  Phenomenon in Drag&Drop cases :
    If Dropped at outside of addressCol2#N-InputBox, Send button is not enabled.
  Note-1:
    image under textbox id=addressCol2#N is "element outside of addressCol2#N-InputBox".
  Note-2:
    Bottom row is always "dummy row", and the bottom row doesn't have textbox named
    id="addressCol2#N" in it.
    So, blank bottom row(dummy row) is also "element outside of addressCol2#N-InputBox".

Quick observation.
When mail address is Dropped at "outside of addressCol2#N-InputBox" :
(1) Dropped mail address is shown in first free(empty) "ordinal row".
(2) First "dummy row" is changed to empty "ordinal row" : 
      "ordinal row" : Second listitem(right column) has <textbox id="addressCol2#N">
(3) A new "dummy row" is added at bottom.
So, patch by Ikezoe-san looks to do correct thing.
Patch simply changes awAddRecipient() call to AddRecipient() call.
It merely adds onRecipientsInput() call then it fires input event at step (1), because AddRecipient() is as follows.
> http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4415
> 4415 // public method called by the address picker sidebar
> 4416 function AddRecipient(recipientType, address)
> 4417 {
> 4418   awAddRecipient(recipientType, address);
> 4419   onRecipientsInput();
> 4420 }

Questions to aceman:.
It's impossible to type at "dummy row".
Why "dummy row" should be shown? What kind of problem will occur if display:none?

As Ikezoe-san says, I also think that heading part(first listcell) of "Subject row" is better placed at "outside of listbox id=addressingWidget". What do you think?

(In reply to :aceman from comment #10)
> it was caused by my change?

I can't think fault in your code.
Sidebar/Addres Book code perhaps merely had avoided non-needed input event upon Drop by directly calling awAddRecipient() instead of calling standard AddRecipient() in Mailnews, because such event had been merely useless for long time.
So, I think "fault" is "To skip firing standard input or changed event even though content of an XUL element is put or changed" in Sidebar or Address Book feature.
However, you are who has changed "input event upon Drop" from useless to useful/mandatory. And, this bug is obviously "problem when new feature(what was implemented by you) is used".
Please go forward.

Comment 15

[:aceman]

Attachment: patch with test

This adds the onRecipientsInput check deeper in the manipulation of the contents of recipients widget. It should catch more cases again. It seems to fix the drag and drop case and also the switching identities case for which I add a test. I don't know how to make a drag and drop test so if anybody wants feel free to add one.

Please try this guys if you can find any new scenario that is not covered.

Comment 16

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #15)
> patch with test

>  function AddRecipient(recipientType, address)
>  {
>    awAddRecipient(recipientType, address);
> -  onRecipientsInput();
>  }

> http://mxr.mozilla.org/comm-central/search?string=AddRecipient
AddRecipient() is called at following line only.
> http://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abContactsPanel.js#47
> 36 function addSelectedAddresses(recipientType)
> 47       parent.AddRecipient(recipientType, address); 

> http://mxr.mozilla.org/comm-central/search?string=addSelectedAddresses&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
addSelectedAddresses is called by several addresbook modules only.

So, I think "removing onRecipientsInput() from AddRecipient()" won't produce any problem, per your test result.
(Q1)
However, addSelectedAddresses() and AddRecipient() may be used by addon, and addon developers may rely on "existence of onRecipientsInput() in AddRecipient()".
No problem in such case? 
(Q2)
What is reason to keep "function AddRecipient" definition even after "removing onRecipientsInput() from AddRecipient()"?
Even if someone calls AddRecipient(), correct thing is "call awAddRecipient() instead of AddRecipient()" as done in any addresbook/sidebar code after "removing onRecipientsInput() from AddRecipient()", isn't it?

Comment 17

[:aceman]

(In reply to WADA from comment #16)
> So, I think "removing onRecipientsInput() from AddRecipient()" won't produce
> any problem, per your test result.
> (Q1)
> However, addSelectedAddresses() and AddRecipient() may be used by addon, and
> addon developers may rely on "existence of onRecipientsInput() in
> AddRecipient()".
> No problem in such case? 
AddRecipient calls awAddRecipient, which after some more calls bubbles down to awSetInputAndPopupValue where I moved the onRecipientsInput to. So whatever top level function the addon calls, onRecipientsInput should get executed.

> What is reason to keep "function AddRecipient" definition even after
> "removing onRecipientsInput() from AddRecipient()"?
The function without onRecipientsInput was there before I added the onRecipientsInput in bug 431217.

> Even if someone calls AddRecipient(), correct thing is "call
> awAddRecipient() instead of AddRecipient()" as done in any
> addresbook/sidebar code after "removing onRecipientsInput() from
> AddRecipient()", isn't it?
AddRecipient is marked "public" so maybe designed to be used by addons and other code. awAddRecipient is probably more internal. Yes, currently they do the same, but that may change in the future (or was temporarily changed by me in bug 431217).

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 828904 [details] [diff] [review]
patch with test

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

LGTM! r=mkmelin

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 828904 [details] [diff] [review]
patch with test

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

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +261,1 @@
>  }

awSetInputAndPopupValue is invoked in awRemoveRecipients too.
It is weird for me that send button is enabled when recipient is removed. If this patch is correct, the function should be renamed.

::: mail/test/mozmill/composition/test-send-button.js
@@ +113,5 @@
> +  // The first identity will have an automatic CC enabled.
> +  let identity1 = account.defaultIdentity;
> +  identity1.doCc = true;
> +  identity1.doCcList = "[e-mail]";
> +

We should use descriptive name instead of numbering.

@@ +123,5 @@
> +  assert_equals(identityPicker.selectedIndex, 0);
> +
> +  // Switch to the second identity that has no CC. Send should be disabled.
> +  assert_true(account.identities.length >= 2);
> +  let identity2 = account.identities.queryElementAt(1, Ci.nsIMsgIdentity);

Ditto.

Comment 20

[Thomas D. (:thomas8)]

Comment on attachment 828904 [details] [diff] [review]
patch with test

I can't test this unless landed or trybuild, but it looks going into the right direction.

Comment 21

[:aceman]

Attachment: patch with test v2

Improved the identifier names as Hiro suggested. Carrying over review from mkmelin as the function name was confirmed over IRC.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/3b2e5cbda317

Comment 23

[Philip Chee]

Backed out changeset 3b2e5cbda317 (bug 933101) for suspicion of causing Mozmill hangs in test-forwarded-eml-actions.js
https://hg.mozilla.org/comm-central/rev/f1ff9928a4be

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Test runs post-backout confirm that this patch was responsible.

Comment 25

[:aceman]

OK, I see now. I just do not understand how you could see it. The test results do not point to this patch in any way. Only that it touches composition :)

Comment 26

[:aceman]

Attachment: patch with test v3

So the problem was that with this patch any change to recipients was considered a change of the message draft so when attempting to close the compose window the dialog offering to safe the draft popped up. The test didn't expect that (as it was just replying to a message with recipient prefilled without changing anything) so it hung. I changed that to only mark the msg content as changed when the field was changed by the user (not automatic). Please review this again.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 831101 [details] [diff] [review]
patch with test v3

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

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +261,1 @@
>  }

Why don't you call updateSendCommands directly?

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 831101 [details] [diff] [review]
patch with test v3

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

I am sorry my previous comment failed.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +256,5 @@
>      awSetInputAndPopupId(inputElem, popupElem, rowNumber);
>  
>    _awSetAutoComplete(popupElem, inputElem);
> +
> +  onRecipientsChanged(true);

Why don't you call updateSendCommands directly.

Comment 29

[:aceman]

That is also a way to do it.
It depends on what we want to logically group. Currently I made it so there is a common function when recipients change that decides what to do based on whether the change was automatic (by code). If we want to decide that at the callers (which may become multiple later on) then that is possible too. I'll let mkmelin decide.

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 831101 [details] [diff] [review]
patch with test v3

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2998,3 @@
>  {
> +  if (!aAutomatic) {
> +    gContentChanged = true;

you do want to set gContentChanged for both cases
which makes a case for not calling updateSendCommands directly

Comment 31

[:aceman]

(In reply to Magnus Melin from comment #30)
> you do want to set gContentChanged for both cases
> which makes a case for not calling updateSendCommands directly
Which 2 cases do you mean?
The change is specifically to not set gContentChanged from awSetInputAndPopupValue where the recipient value is changed programatically. It looks like it wasn't set before my change and the test was expecting that situation.

So what is the correct behaviour?

Comment 32

[Magnus Melin [:mkmelin]]

I'd expect gContentChanged to be true after you drag a contact to the addressing field. 
Why would it work differently if you type it in vs. if you drag the name there. (= the two cases, "auto" and "manual")

Comment 33

[:aceman]

So what if the addresses are filled in from the identity? Currently it is not considered a change so gContentChanged is false. It could also be considered a change of the message. So do you intend to change the current behaviour? Do I need to adapt the test instead?

Comment 34

[Magnus Melin [:mkmelin]]

I wouldn't consider that a change, no. Just manual filling of addresses, by typing/clicking/drag'n'drop.

Comment 35

[:aceman]

Determining why the recipients were autofilled by awSetInputAndPopupValue will be harder. It looks like it would need an argument being passed from awAddRecipient down to awSetInputAndPopupValue to indicate whether we want gContentChanged set.
Also, currently drag and drop does not cause the msg to be considered changed, so this change in behaviour needs to be better specified which actions should set gContentChanged and which not.

Comment 36

[Thomas D. (:thomas8)]

Regardless of implementation details, can we please ensure the following:

After any change of any detail of the draft (including recipients), whether automatical, semi-automatical or not, we need to ensure that the changes are preserved correctly. Iirc, gContentChanged=true will trigger the "Do you want to save changes" question when closing draft. So we can't afford to get that wrong in either way: Asking when there are no changes (everything already saved in draft) is annoying; not asking when there are changes might cause dataloss. I haven't tested the details of current behaviour yet, but just wanted to point out that whatever changes here must be deliberate and considerate of that to avoid problems of ux-implemenation-level (UI/behaviours should not be organized around the underlying implementation).

More specifically, if I change sender and that action fills in new auto-recipients, than that's two content-relevant changes so gContentChanged should be true. However, it's probably already true after changing the sender, so we might not have to set it programmatically to true again when the auto-recipients get filled at the same time (but it wouldn't hurt either to set it again).

Comment 37

[:aceman]

Currently changing the sender (identity) does not make the msg changed so we are talking about changing the behaviour in many cases.

Mkmelin, can we get this patch landed as is (with no change to current behaviour)? Because we need this for TB24 to fix the regression.

If you guys want to change the behaviour to be more precise, can we make it in a new bug?

Comment 38

[:aceman]

Because which actions make the message appear changed is not related to enabling the Send button...

Comment 39

[Thomas D. (:thomas8)]

Does the current patch fix the regression described in comment 2?
Looks like a regular scenario with no less probability than dragging addresses around...

As long as we fix all "Send wrongly disabled" regressions and don't introduce new regressions, I'm fine with postponing of finetuning the gContentChanged behaviour...

(In reply to Thomas D. from comment #2)
> Aceman, I'm quite sure that the algorithm that disables send buttons etc
> isn't complete yet.
> E.g. changing From from a sender without automatic bcc to a sender *with*
> automatic bcc will not enable send, but should. Any other cases that we
> might have missed here?

Comment 40

[:aceman]

Yes, the patch fixes the drag and drop AND filling in CC via changing sending indetntity (there is even a new test for this). It should cover all automatic changes of recipients if they go through awSetInputAndPopupValue(). But it does not change the behaviour of gContentChanged.

Comment 41

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #35)
> Determining why the recipients were autofilled by awSetInputAndPopupValue
> will be harder. It looks like it would need an argument being passed from
> awAddRecipient down to awSetInputAndPopupValue to indicate whether we want
> gContentChanged set.

You could also just set gContentChanged in addition to calling the function, for that case.

But sure, we can leave stuff for follow-ups, it's not a critical think.

Comment 42

[:aceman]

Ok, thanks.

Thomas, can you file the new bug with a good summary what we need to do?

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/0b04606f23dc

Comment 44

[:aceman]

Comment on attachment 831101 [details] [diff] [review]
patch with test v3

[Approval Request Comment]
Regression caused by (bug #): bug 431217
User impact if declined: inability to send mail if recipients are drag and dropped to the address widget
Testing completed (on c-c, etc.): trunk
Risk to taking this patch (and alternatives if risky): none known, tests pass, new test added.

Comment 45

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #42)
> Ok, thanks.
> 
> Thomas, can you file the new bug with a good summary what we need to do?

Sure. Filed bug 941139 for that. Nice brain jogging. :)

Comment 46

[Mark Banner (:standard8)]

Comment on attachment 831101 [details] [diff] [review]
patch with test v3

[Triage Comment]
a=Standard8. This also should land on aurora ready for next cycle.

Comment 47

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/54d37e9ec392

Comment 48

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr24/rev/6ad01e219f06