1249693 - HTML compose toolbar appearing for plain text Compose

Status: VERIFIED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Calum Mackay]

Attachment: Screenshot of HTML compose buttons

With Daily 20160218, the Compose window for plain text emails is showing a toolbar of buttons for font effects, presumably intended for HTML composing.

Note that this does not happen on the first Compose window to come up (oddly), but does occur for all subsequent Compose windows.

I have plain text as my compose pref, in both global and per-account prefs.

The buttons don't seem to do anything, and the message is still sent as plain text.

This toolbar appeared in Daily 0218, and is still there in 0219. It does not appear in 0217.

Comment 1

[Onno Ekker [:nONoNonO UTC+1]]

Probably caused by bug 1202165

Comment 2

[Jorg K (CEST = GMT+2)]

I can reproduce this in the version of 18-02-2016 and on latest trunk. I've switched the HTML composition off in the account preferences.

The first message composition window comes up without the toolbar. However, the second (recycled) window comes up with the toolbar. Don't we love recycling ;-)

I'll blame it on bug 1202165 and attachment 8714103 [details] [diff] [review] (without having done an in-depth analysis). Somehow the hiding mechanism got changed and now doesn't work any more.

Comment 3

[:aceman]

Correct. I see where the bug is.

Comment 4

[:aceman]

Attachment: patch

The default of gMsgCompose.compFields.deliveryFormat is AskUser and in that case we show the HTML toolbar. But even if delivery format is AskUser, we override it and force everything (even UI) to plain text if !gMsgCompose.composeHTML. So do that here too.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ea3ae563376a

Comment 5

[Jorg K (CEST = GMT+2)]

How long do we want to wait for feedback here? Can we please have this reviewed and landed before the branch date, 8th of March 2016. This regressed in TB 47 so it would be good to fix it there so users of Earlybird will never see it.

Comment 6

[:aceman]

Comment on attachment 8721824 [details] [diff] [review]
patch

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

If we can get it reviewed without the original author's look at it, then surely let's try it :)

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8721824 [details] [diff] [review]
patch

I really hate to give an r-, but I dislike this kludge.

The bug is: Set the composition to plain text in the account preferences. Then write e-mail:
1st e-mail: Window comes up without the format toolbar.
2nd and subsequent e-mail: Window comes up *with* the formatting toolbar.

Your patch fixes that, no doubt. It should be commented and read something like this:

if (gMsgCompose.composeHTML) {
  // Restore send format from the draft or use nsIMsgCompSendFormat.AskUser
  // for a new message;
  gSendFormat = gMsgCompose.compFields.deliveryFormat;
} else {
  // We are composing plain text.
  gSendFormat = nsIMsgCompSendFormat.PlainText;
}
SetCompositionAsPerDeliveryFormat(gSendFormat);
// Note that for plain text the menu is hidden, but let's set it anyway.
SelectDeliveryFormatMenuOption(gSendFormat);

But that is not the main problem.

I've asked myself why it works for the first e-mail even without the patch. Well, here is the answer:

https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2330
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2344

if (!recycled)
  if (gMsgCompose.composeHTML)
  else
    document.getElementById("outputFormatMenu").setAttribute("hidden", true);
    document.getElementById("FormatToolbar").setAttribute("hidden", true);
    document.getElementById("formatMenu").setAttribute("hidden", true);
    document.getElementById("insertMenu").setAttribute("hidden", true);
    document.getElementById("menu_showFormatToolbar").setAttribute("hidden", true);

Something is very clumsy here, I have a function SetCompositionAsPerDeliveryFormat which sets four of these five (why??) and then I do it again for (non-recycled) first messages. This really needs straightening out.

While we're cleaning up, please explain to me what this does:
  toolbar.hidden = gHideMenus ||
    (show_menuitem.getAttribute("checked") == "false");
I've looked at it and now my head is hurting and I still don't understand it.

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #7)
>     document.getElementById("outputFormatMenu").setAttribute("hidden", true);
> Something is very clumsy here, I have a function
> SetCompositionAsPerDeliveryFormat which sets four of these five (why??) and
> then I do it again for (non-recycled) first messages.
OK, I can see the difference. For a true plain text mail there is no
Options > Delivery Format (outputFormatMenu).

For a message that started as an HTML e-mail but got delivery format "Plain Text Only" this menu item is maintained.

Comment 9

[:aceman]

(In reply to Jorg K (GMT+1) from comment #7)
> Your patch fixes that, no doubt. It should be commented and read something
> like this:
> 
> if (gMsgCompose.composeHTML) {
>   // Restore send format from the draft or use nsIMsgCompSendFormat.AskUser
>   // for a new message;
>   gSendFormat = gMsgCompose.compFields.deliveryFormat;
> } else {
>   // We are composing plain text.
>   gSendFormat = nsIMsgCompSendFormat.PlainText;
> }
> SetCompositionAsPerDeliveryFormat(gSendFormat);
> // Note that for plain text the menu is hidden, but let's set it anyway.
> SelectDeliveryFormatMenuOption(gSendFormat);
> 
> But that is not the main problem.

But in the rest of the code we keep the gSendFormat have the proper value (usually AskUser), but ignore it when !gMsgCompose.composeHTML. Your proposal would change that convention.

Comment 10

[Jorg K (CEST = GMT+2)]

Fair enough, leave it, I'm flexible ;-)

Just merge the processing with the stuff for "if (!recycled)" below and please explain the
  toolbar.hidden = gHideMenus ||
    (show_menuitem.getAttribute("checked") == "false");
I've still got a headache looking at that ;-)

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: How about this?

Sorry about my unfriendly words the other day. Here is your patch with a few tweaks. I'm still wondering about the
  || (show_menuitem.getAttribute("checked") == "false"); 
bit. Can that go?

Comment 12

[Jorg K (CEST = GMT+2)]

There is still something fishy here. On my slow Windows debug build the HTML toolbar now shows for a split second the second time I open a composition and is then removed. That shouldn't happen. As the second time, we're using a recycled windows, the toolbar was already removed, so I don't understand how it gets put back and then removed again. I need to look into it.

BTW, I've switched HTML composition off in the account settings.

Comment 13

[:aceman]

Attachment: patch v2

Yes, I came to a similar solution, like this.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: How about this? (v2)

I've taken your v2, changed a few variable names, added some words to the comment and removed the un-hiding of the UI elements in the recycler to address comment #12. Please user interdiff.

Since we changed the logic around and set the menus every time we start, we don't need to un-hide stuff when we recycle. Do you agree?

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8724560 [details] [diff] [review]
patch v2

Good, but see my improvement ;-)

Comment 16

[:aceman]

Comment on attachment 8724563 [details] [diff] [review]
How about this? (v2)

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

Seems to be nice cleanup :)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3066,2 @@
>    let format_menubar = document.getElementById("formatMenu");
>    let insert_menubar = document.getElementById("insertMenu");

When playing with variable names, I think the "bar" could be dropped from these two.

Comment 17

[:aceman]

Attachment: patch v3

Thanks, merged the patches and updated the comment why output format is hidden.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8724575 [details] [diff] [review]
patch v3

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

Perfect! Oh, wait. Can't we remove the gHideMenus global now?

Comment 19

[:aceman]

Attachment: patch v3.1

Yes, we removed its single use.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8724578 [details] [diff] [review]
patch v3.1

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

Very nice. I think we're (almost) done. Thanks a lot for taking on this job.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3074,5 @@
> +  let insert_menu = document.getElementById("insertMenu");
> +  let view_menuitem = document.getElementById("menu_showFormatToolbar");
> +
> +  let gHideMenus = !gMsgCompose.composeHTML ||
> +                   (aDeliveryFormat == nsIMsgCompSendFormat.PlainText);

Please change the name. gXXX is for globals, is it not?

Comment 21

[:aceman]

Attachment: patch v3.2

Yes, pedantic :)
I'm not concentrated enough today.

Comment 22

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #21)
> Yes, pedantic :)
I think I got that from you(?). I'd call it perfectionist.

Comment 23

[:aceman]

https://hg.mozilla.org/comm-central/rev/e625d681fc96

Comment 24

[Calum Mackay]

yup, working fine now, for me, in 0229; thanks very much indeed.


Should I now mark as "Verified" - as Reporter - or do I leave that for some formal QA activity?

Comment 25

[Jorg K (CEST = GMT+2)]

We aim to please ;-) Yes, I think you can mark it verified. Thanks.

Comment 26

[Calum Mackay]

thanks very much again, Jörg, and aceman.