1542667 - Using From field's "customize from address" breaks saving and sending

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Not so Veteran]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

1. Open the message compose window (ctrl+n)
2. At this stage, save the message (ctrl+s) to confirm that saving currently works
3. In the "From:" dropdown menu, choose "Customize From Address..."
4. Add at least one character to the From field's current value
5. De-select the "From" field (e.g. click into main message body textarea)
6. Attempt to save the message again (ctrl+s)
7. Click "Send" to attempt to send the message

Actual results:

Steps 6 and 7 fail. After a manual change is made to the From address, saving the message does nothing (the message isn't updated in the Drafts folder). Sending the message also does nothing (the message is never sent).

Other notes:

• After re-selecting a known account in the "From" dropdown menu, saving and sending functionality is restored
• A current workaround is to save the message (with ctrl+s) before de-selecting the "From" field. After doing this, the bug won't occur on deselect.
• Exact version: 67.0b1 (32-bit)

Expected results:

The message should have been saved, and sent, successfully.

Comment 1

[Jorg K (CEST = GMT+2)]

Thanks for reporting. I can confirm the issue in TB 67 beta.

When I save or send the draft after editing the From:, I get this error in the console:

TypeError: identityList.selectedItem is null
MsgComposeCommands.js:4032:3
getCurrentIdentityKey chrome://messenger/content/messengercompose/MsgComposeCommands.js:4032
getCurrentIdentity chrome://messenger/content/messengercompose/MsgComposeCommands.js:4040
GenericSendMessage chrome://messenger/content/messengercompose/MsgComposeCommands.js:3273
SaveAsDraft chrome://messenger/content/messengercompose/MsgComposeCommands.js:3513
Save chrome://messenger/content/messengercompose/MsgComposeCommands.js:3493
doCommand chrome://messenger/content/messengercompose/MsgComposeCommands.js:704
doCommand chrome://messenger/content/messengercompose/MsgComposeCommands.js:915
goDoCommand chrome://global/content/globalOverlay.js:81
oncommand chrome://messenger/content/messengercompose/messengercompose.xul:1
An error occurred executing the cmd_saveDefault command: [Exception... "[JavaScript Error: "identityList.selectedItem is null" {file: "chrome://messenger/content/messengercompose/MsgComposeCommands.js" line: 4032}]'[JavaScript Error: "identityList.selectedItem is null" {file: "chrome://messenger/content/messengercompose/MsgComposeCommands.js" line: 4032}]' when calling method: [nsIController::doCommand]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 81" data: yes]

Looks like the de-XBL team have broken the editable menu list.

Comment 2

[Arshad Khan [:arshad]]

The error that is being thrown, comes from https://searchfox.org/comm-central/source/mail/base/content/mailWidgets.js#629 . When I tried to look back to Bug 1524457 and Bug 1524497 to see the earlier code for the change, I couldn't find the part of patch which has the xbl equivalent code for _change event. Probably Geoff would know.

Comment 3

[Magnus Melin [:mkmelin]]

I have at least part of a patch, still investigating if it's complete.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: bug1542667_custom_from_broken.patch

This fixes it. Unfortunately, I notice other things that are wrong too now, and this patch is basically wrong too. Looks like The mutation observer is supposed to take care of this, but we override it so it doesn't.t

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1542667_custom_from_broken.patch

The real fix.

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9062731 [details] [diff] [review]
bug1542667_custom_from_broken.patch

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

::: mail/base/content/mailWidgets.js
@@ +641,5 @@
>  
> +      if (!this.menupopup) {
> +        this.appendChild(MozXULElement.parseXULToFragment(`<menupopup />`));
> +      }
> +      

Bad white-space.

@@ +652,4 @@
>      }
>  
>      disconnectedCallback() {
> +      // Override. Unlike super we never do n re-setup in connectedCallback,

Typo.

@@ +657,2 @@
>      }
> +    

More bad white-space.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5039009bbaed
Fix menulist-editable to have correct selectedItem. r=darktrojan CLOSED TREE DONTBUILD

Comment 8

[Jorg K (CEST = GMT+2)]

Landed with trailing spaces removed and typo fixed. Since the tree is closed, here's a try together with some other stuff:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fe90a2c95c01254e441d8fc0196b06f9ca7e6bc6

Comment 9

[Jorg K (CEST = GMT+2)]

Looks like this caused a test failure:
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/composition/test-draft-identity.js | test-draft-identity.js::test_draft_identity_selection
which shows on all platforms. Log says:

13:26:02     INFO -    EXCEPTION: identityList.selectedItem is null
13:26:02     INFO -      at: MsgComposeCommands.js line 3994
13:26:02     INFO -         getCurrentAccountKey MsgComposeCommands.js:3994 3
13:26:02     INFO -         checkCompIdentity test-draft-identity.js:108 28
13:26:02     INFO -         test_draft_identity_selection test-draft-identity.js:178 5

Can you address this quickly before I back it out?

Comment 10

[Magnus Melin [:mkmelin]]

I think I see the problem. Checking the solution now.

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: bug1542667_custom_from_followup.patch

This should be the fix.
(I added a desc property for the tests too so one could see which was failing.)

At least in my build locally the customize functionality is now very odd even without the previous patch: only one identity listed until you click the customize - and then they all get listed. I didn't see that when testing earlier on a few days old tree, so something must have changed.

Comment 12

[Geoff Lankow (:darktrojan)]

I've had a closer look at this and I don't think it's right. Changing the value of one of the existing items when typing in the textbox feels like the wrong thing to do. In fact, if there's no selected item when typing:

JavaScript error: chrome://messenger/content/mailWidgets.js, line 635: TypeError: this.selectedItem is null

It might seem to work okay for the identity header but in the general case it doesn't.

Comment 13

[Geoff Lankow (:darktrojan)]

Sorry, the previous comment refers to this solution as a whole, including the first patch.

Comment 14

[Jorg K (CEST = GMT+2)]

Just to clarify this even further: Geoff suggested to back out the first patch:
01:08:34 - jorgk: oh, well, if you say the 1st patch was wrong already, I might as well take it out
01:09:45 - darktrojan: no tests were fixed by landing the first patch were there?
01:09:58 - jorgk: right
01:10:04 - jorgk: no tests were fixed
01:10:06 - darktrojan: yeah, take it out

Comment 15

[Geoff Lankow (:darktrojan)]

In fact, this has busted the test mail/base/test/browser/browser_menulist.js, which I had forgotten even existed.

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d9dfeb170a29
Backed out changeset 5039009bbaed for causing test failures

Comment 18

[Ben Bucksch (:BenB)]

Thanks for reporting and fixing this.

This is a major bug. I'd rather have a real bug fixed and a test failure than working tests and real bugs. Could we land this and then fix the test?

Comment 21

[Jorg K (CEST = GMT+2)]

This is broken for over two months now damaging TB 67 beta and TB 68 beta. Geoff, can you finish this off. Magnus hasn't dedicated time to this in seven(!!) weeks.

Comment 22

[Ben Bucksch (:BenB)]

+1 I need this for my daily communication. Adding dogfood.

Comment 23

[Geoff Lankow (:darktrojan)]

Attachment: 1542667-custom-compose-1.diff

This feels very simple, so I may have missed something, but I can't think what. Anyway, it does seem to work.

Comment 24

[:aceman]

Comment on attachment 9073516 [details] [diff] [review]
1542667-custom-compose-1.diff

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

Yeah, it is a pity we have 'value', 'accountkey', 'identitykey' attributes on the listitems and from those only 'value' is automatically copied to the menulist when a selection is made.
Maybe we could investigate if all are really needed. Or we could stuff all of them into 'value' (with a separator) and then this event listening wouldn't be needed.
But in the current patch, do we still needed getting the attribute values from identityElement.selectedItem (instead of the the menulist), at and around of
https://searchfox.org/comm-central/rev/7999bf7768ee8b6bd57deef0c87080938ec77c75/mail/components/compose/content/MsgComposeCommands.js#5507 ?

Comment 25

[Ben Bucksch (:BenB)]

Yeah, it is a pity we have 'value', 'accountkey', 'identitykey' attributes on the listitems and from those only 'value' is automatically copied to the menulist when a selection is made.

Well, that's normal. But menulist.selectedItem.get... should work. Why exactly does return identityList.selectedItem.getAttribute("accountkey"); not work? That should normally work. It does not rely on properties to be copied on selection.

Comment 26

[:aceman]

(In reply to Ben Bucksch (:BenB) from comment #25)

Yeah, it is a pity we have 'value', 'accountkey', 'identitykey' attributes on the listitems and from those only 'value' is automatically copied to the menulist when a selection is made.
Well, that's normal.

Yes it is normal in the way that it is designed to only copy 'value'. So we shouldn't attach more attributes if that doesn't really work and needs special hooks on top.

But menulist.selectedItem.get... should work. Why exactly does return identityList.selectedItem.getAttribute("accountkey"); not work? That should normally work. It does not rely on properties to be copied on selection.

It may be that once we set a new value on the editable menulist, and it calls the 'value' setter internally, https://searchfox.org/comm-central/rev/7999bf7768ee8b6bd57deef0c87080938ec77c75/mozilla/toolkit/content/widgets/menulist.js#116, then .selectedItem becomes null as the value no longer represents any of the items in the list. So the patch seems to go in the right direction of not relying on .selectedItem. We shouldn't need it ever, if we get all 3 attributes/value on the menulist automatically.

Comment 27

[Ben Bucksch (:BenB)]

selectedItem. goes to the original element and should have all properties you attached to the item. It should work and seems like the right solution. To me, the right direction would be to only use identityList.selectedItem.getAttribute('accountkey') and never try to mess with the identityList.value. If setting identityList.value messes up selectedItem, then the fix would be to not set it, no? Or do you absolutely need to set it, e.g. for the edited text case? But in the edited case, you wouldn't need the account and identity key from the item, would you?

Comment 28

[Ben Bucksch (:BenB)]

Oh, I think I see the problem. The old behavior was that the widget would store the identity that I had selected before I selected "customize From...". I would send from whatever account was selected before.

But that's completely invisible state and too subtle. I think it would be fine to drop back to whatever account was selected when the composer first opened, i.e. the account and identity of the email that I reply to.
I think that would solve your issue.

Comment 29

[Geoff Lankow (:darktrojan)]

Attachment: 1542667-custom-compose-2.diff

But in the current patch, do we still needed getting the attribute values from identityElement.selectedItem (instead of the the menulist), at and around of
https://searchfox.org/comm-central/rev/7999bf7768ee8b6bd57deef0c87080938ec77c75/mail/components/compose/content/MsgComposeCommands.js#5507 ?

Probably not, but that's a better place to make the change I'm making than in another listener.

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 9062731 [details] [diff] [review]
bug1542667_custom_from_broken.patch

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

Seems to work.

There are problems with the MozMenulistEditable implementation though. 
It's reusing the super mAttributeObserve for a different purpose. But super will swap that around when changing selection, and "our" observer would no longer do anything.

Comment 31

[Magnus Melin [:mkmelin]]

Comment on attachment 9073697 [details] [diff] [review]
1542667-custom-compose-2.diff

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4076,5 @@
>  
>  function getCurrentIdentityKey() {
>    // Get the identity key.
>    let identityList = GetMsgIdentityElement();
> +  return identityList.getAttribute("identitykey");

gCurrentIdentity.key?

Comment 32

[Geoff Lankow (:darktrojan)]

Attachment: 1542667-custom-compose-3.diff

If we'd had this one line in the first place this bug probably wouldn't have happened.

Comment 33

[:aceman]

Comment on attachment 9074384 [details] [diff] [review]
1542667-custom-compose-3.diff

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5501,4 @@
>      if (identityElement.selectedItem) {
> +      // Set the identity key value on the menu list.
> +      idKey = identityElement.selectedItem.getAttribute("identitykey");
> +      identityElement.setAttribute("identitykey", idKey);

Is anything reading the identitykey on the menulist now? I haven't spotted any user left.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a32a7dd50791
Copy selected identity's attributes when customising the From address. r=mkmelin

Comment 35

[Jorg K (CEST = GMT+2)]

This reminds me of the discussion in ...
EDIT: Jolly good, "Form History Control" managed to play another trick on me and posted an undesired comment. Well, since the damage is done, I guess I was going to say that is reminded me of the discussion in bug 1524457 (or maybe elsewhere), where there was also a long discussion about how to fix this best. I find comment #30 disconcerting where Magnus claims that some problems in the editable menu list still exist.

Comment 36

[Magnus Melin [:mkmelin]]

Will fix the problems in bug 1562130.

Comment 37

[Jorg K (CEST = GMT+2)]

TB 68 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/fa31402211e8c0af8d3343e219f68722b9c138a8