Closed Bug 1249693 Opened 5 years ago Closed 5 years ago

HTML compose toolbar appearing for plain text Compose

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 47.0

People

(Reporter: calum.mackay, Assigned: aceman)

References

Details

(Keywords: regression)

Attachments

(2 files, 6 obsolete files)

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.
Probably caused by bug 1202165
OS: Mac OS X → All
Hardware: x86_64 → All
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.
Blocks: 1202165
Flags: needinfo?(syshagarwal)
Flags: needinfo?(acelists)
Keywords: regression
Correct. I see where the bug is.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Flags: needinfo?(acelists)
Attached patch patch (obsolete) — Splinter Review
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
Flags: needinfo?(syshagarwal)
Attachment #8721824 - Flags: feedback?(syshagarwal)
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 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 :)
Attachment #8721824 - Flags: review?(mozilla)
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.
Attachment #8721824 - Flags: review?(mozilla)
Attachment #8721824 - Flags: review-
Attachment #8721824 - Flags: feedback?(syshagarwal)
(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.
(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.
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 ;-)
Attached patch How about this? (obsolete) — Splinter Review
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?
Attachment #8724559 - Flags: feedback?(acelists)
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.
Attached patch patch v2 (obsolete) — Splinter Review
Yes, I came to a similar solution, like this.
Attachment #8721824 - Attachment is obsolete: true
Attachment #8724560 - Flags: feedback?(mozilla)
Attached patch How about this? (v2) (obsolete) — Splinter Review
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?
Attachment #8724559 - Attachment is obsolete: true
Attachment #8724559 - Flags: feedback?(acelists)
Attachment #8724563 - Flags: feedback?(acelists)
Comment on attachment 8724560 [details] [diff] [review]
patch v2

Good, but see my improvement ;-)
Attachment #8724560 - Flags: feedback?(mozilla) → feedback+
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.
Attachment #8724563 - Flags: feedback?(acelists) → feedback+
Attached patch patch v3 (obsolete) — Splinter Review
Thanks, merged the patches and updated the comment why output format is hidden.
Attachment #8724560 - Attachment is obsolete: true
Attachment #8724563 - Attachment is obsolete: true
Attachment #8724575 - Flags: review?(mozilla)
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?
Attached patch patch v3.1 (obsolete) — Splinter Review
Yes, we removed its single use.
Attachment #8724575 - Attachment is obsolete: true
Attachment #8724575 - Flags: review?(mozilla)
Attachment #8724578 - Flags: review?(mozilla)
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?
Attachment #8724578 - Flags: review?(mozilla) → review+
Attached patch patch v3.2Splinter Review
Yes, pedantic :)
I'm not concentrated enough today.
Attachment #8724578 - Attachment is obsolete: true
Attachment #8724581 - Flags: review+
(In reply to :aceman from comment #21)
> Yes, pedantic :)
I think I got that from you(?). I'd call it perfectionist.
https://hg.mozilla.org/comm-central/rev/e625d681fc96
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
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?
We aim to please ;-) Yes, I think you can mark it verified. Thanks.
thanks very much again, Jörg, and aceman.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.