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

RESOLVED FIXED in Thunderbird 28.0

Status

P1
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: y2k31121999-all, Assigned: aceman)

Tracking

(Blocks: 1 bug)

24 Branch
Thunderbird 28.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird27 fixed, thunderbird_esr2426+ fixed)

Details

(Whiteboard: [STR comment 7])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
work-around: type a character in address field and clear it will have Send button turn to enable
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).
Created attachment 827163 [details] [diff] [review]
Possible fix without test
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 4

5 years ago
(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 .
OS: Mac OS X → All
Hardware: x86 → All
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.
OS: All → iOS 3
Hardware: All → x86
OS: iOS 3 → Mac OS X
(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.
(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.
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [STR comment 7]
(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)
(Assignee)

Comment 9

5 years ago
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.)
Blocks: 431217
(Assignee)

Comment 10

5 years ago
Hiro, would you like to continue on this further, or can I take it as it was caused by my change?
(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)
(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
(Assignee)

Updated

5 years ago
Assignee: nobody → acelists
Severity: normal → major
status-thunderbird_esr24: --- → affected
Priority: -- → P1
(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.
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.
(Assignee)

Comment 15

5 years ago
Created attachment 828904 [details] [diff] [review]
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.
Attachment #827163 - Attachment is obsolete: true
Attachment #828904 - Flags: review?(mkmelin+mozilla)
Attachment #828904 - Flags: feedback?(hiikezoe)
Attachment #828904 - Flags: feedback?(bugzilla2007)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Summary: Send button disabled when write a message with to, subject and content → Send button disabled when write a message with to, subject and content, if mail address is Dropped at outside of address text input box
(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?
(Assignee)

Comment 17

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

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

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

LGTM! r=mkmelin
Attachment #828904 - Flags: review?(mkmelin+mozilla) → review+
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 = "Auto@recipient.invalid";
> +

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 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.
Attachment #828904 - Flags: feedback?(bugzilla2007)
(Assignee)

Comment 21

5 years ago
Created attachment 830369 [details] [diff] [review]
patch with test v2

Improved the identifier names as Hiro suggested. Carrying over review from mkmelin as the function name was confirmed over IRC.
Attachment #828904 - Attachment is obsolete: true
Attachment #828904 - Flags: feedback?(hiikezoe)
Attachment #830369 - Flags: review+
(Assignee)

Updated

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

Comment 23

5 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Test runs post-backout confirm that this patch was responsible.
(Assignee)

Comment 25

5 years ago
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 :)
(Assignee)

Comment 26

5 years ago
Created attachment 831101 [details] [diff] [review]
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.
Attachment #830369 - Attachment is obsolete: true
Attachment #831101 - Flags: review?(mkmelin+mozilla)
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 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.
(Assignee)

Comment 29

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

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

Comment 31

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

5 years ago
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")
(Assignee)

Comment 33

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

Comment 34

5 years ago
I wouldn't consider that a change, no. Just manual filling of addresses, by typing/clicking/drag'n'drop.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 35

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

Comment 37

5 years ago
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?
(Assignee)

Comment 38

5 years ago
Because which actions make the message appear changed is not related to enabling the Send button...
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?
(Assignee)

Comment 40

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

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

Comment 42

5 years ago
Ok, thanks.

Thomas, can you file the new bug with a good summary what we need to do?
Status: REOPENED → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/0b04606f23dc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 44

5 years ago
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.
Attachment #831101 - Flags: approval-comm-esr24?
(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 on attachment 831101 [details] [diff] [review]
patch with test v3

[Triage Comment]
a=Standard8. This also should land on aurora ready for next cycle.
Attachment #831101 - Flags: approval-comm-esr24?
Attachment #831101 - Flags: approval-comm-esr24+
Attachment #831101 - Flags: approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/54d37e9ec392
status-thunderbird27: --- → fixed
tracking-thunderbird_esr24: --- → 26+
https://hg.mozilla.org/releases/comm-esr24/rev/6ad01e219f06
status-thunderbird_esr24: affected → fixed
You need to log in before you can comment on or make changes to this bug.