1658999 - Fixed Width not working: After bug 1655279, fixed width can't be changed to variable width any more - take 6

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P1)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1658156 +++
+++ This bug was initially created as a clone of Bug #1655279 +++
+++ This bug was initially created as a clone of Bug #1653545 +++
+++ This bug was initially created as a clone of Bug #1622231 +++

In the current TB 78.1.x pre-release where 1655279 was included, compose a message with fixed width setting. Then I "select all" and chose variable width from the menu.

Expected: Text is turned into variable width.
Actual: Nothing happens.

This is similar to bug 1658156 - take 4.

Magnus and Khushil: As far as I can see, composing involving variable and fixed width "font" is rather broken in TB 78. Is there any plan to restore this area to working order?

Have you considered backing out bug 1582410? All you need to do then is fix bug 1625218 with the "strange" but working patch.

Comment 1

[Jorg K (CEST = GMT+2)]

I've done a try build
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e03a557a6bfe67461dba44a851bf0c5a8278f975
which you can try with
summary: Backed out changeset 4276129dc38e - bug 1655279
summary: Backed out changeset 203cf274ccff - bug 1653545
summary: Backed out changeset ffc62563f0d7 - bug 1582410
summary: Bug 1625218 - make sure formatting toolbar state is update for paragraph mode after setting cmd_paragraphState. r=

Comment 3

[Khushil Mistry [:khushil324]]

Magnus has suggested to try out the Monospace solution for fixed with. Checkout bug 1653545 comment 9.
Created a patch and sent it for a try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8a4b81ba2424c509b3e9ef8df2f0a5ad968fccc5

Comment 4

[Khushil Mistry [:khushil324]]

Attachment: Bug-1658999_variable-width-problem-compose-window-0.patch

Jorg, you previously said that the "monospace" solution is breaking something in plaint text mode. Can you tell the steps to reproduce that?

Comment 5

[Jorg K (CEST = GMT+2)]

Check out bug 1653545 comment #12, especially the bit about downgrading to plain text. That needs to be adapted if you drop <tt>.

Comment 6

[Jorg K (CEST = GMT+2)]

Write a message in "fixed width", was <tt> now monospace. In the compose options, send options, select: Send message as plain text if possible.

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1658999_variable-width-problem-compose-window-1.patch

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 9169991 [details] [diff] [review]
Bug-1658999_variable-width-problem-compose-window-1.patch

Don't you hate when you upload a patch and the system doesn't get it :-(

Comment 9

[Jorg K (CEST = GMT+2)]

OK, let's see:
Bug 1582410/Bug 1625218: Font indicator: working
Bug 1653545, take 2: OK, we now get <font face="monospace">
Bug 1655279, take 3: Compose pref "Fixed width" working
Bug 1658156, take 4: Change to variable and keep writing: Working, still not ideal(**), if you position at the end of the line, you continue typing in fixed width since the M-C editor gives us this: <font face="monospace"><br></font>, so the line end is wrapped in monospace font.
Bug 1658908, take 5: Solved
Bug 1658999, take 6, this bug here: can't set to variable. Seems to be fixed.

Tested in both "Body Text" and "Paragraph" mode. (**) is working better in paragraph mode since there is no <br> involved.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9169991 [details] [diff] [review]
Bug-1658999_variable-width-problem-compose-window-1.patch

Working as per the previous comment, but as we know, missing the backend change for the plaintext downgrade. That's very important.

Comment 11

[Magnus Melin [:mkmelin]]

Didn't try the patch yet, but the plaintext downgrade would only need a change for using <style>, no?
It shouldn't be a problem with this patch since it's just <font face="monospace">

Comment 12

[Jorg K (CEST = GMT+2)]

https://searchfox.org/comm-central/rev/b6b6fbded631b66339ea2e04e863cd516a47f210/mailnews/compose/src/nsMsgCompose.cpp#4992
stops the downgrade test on a style attribute, correct, and returns nsIMsgCompConvertible::No straight away.
Since <font> isn't processed as convertible, since it generally isn't, the function will fall through and also return nsIMsgCompConvertible::No.

So basically you need to add a test for <font> and then also look at the value and allow downgrade, if the value is monospace. Of course you're changing the behaviour since before <tt> was downgraded whereas <font face="monospace"> was not. That's why <tt> was worth keeping IMHO.

Comment 13

[Magnus Melin [:mkmelin]]

Ah yes. We should make sure to also allow downgrade if explicate marked variable width.

Comment 14

[Jorg K (CEST = GMT+2)]

Huh? Variable width is the default when no other font setting is present, and in the past, no <tt>.

Comment 15

[Magnus Melin [:mkmelin]]

Yes, per default. But if you change your mind and swap it back to variable, I think it will mark it as such? Switching back from fixed to variable seems broken on trunk actually, so will have to check it later.

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin [:mkmelin] from comment #15)

Yes, per default. But if you change your mind and swap it back to variable, I think it will mark it as such?

No. There is no such thing as a "variable width" font, "variable width" means the absence of a <font> or <tt> tag. Setting a section to "variable width" will remove those tags. Try it in a version that still works :-(

Switching back from fixed to variable seems broken on trunk actually, so will have to check it later.

Yes, please read the bug summary: "fixed width can't be changed to variable width any more".

Can someone provide a patch for the backend downgrade so that we stand the chance to get this fixed in TB 78.2?

Comment 17

[Magnus Melin [:mkmelin]]

Will take a look soon.

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9169991 [details] [diff] [review]
Bug-1658999_variable-width-problem-compose-window-1.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +8165,5 @@
>  function loadHTMLMsgPrefs() {
>    let fontFace = Services.prefs.getStringPref("msgcompose.font_face", "");
> +  if (fontFace == "tt") {
> +    fontFace = "monospace";
> +    Services.prefs.setStringPref("msgcompose.font_face", "monospace");

Would be nicer to do this in a migration.

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: variable-width-problem-compose-window.patch

Including the fix to downconvert, and changed to migrate the bad pref instead if needed.

Comment 20

[Magnus Melin [:mkmelin]]

The cpp changes, at least, will require some adaptions to for ESR, due to the NS_LITERAL changes.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 9170096 [details] [diff] [review]
variable-width-problem-compose-window.patch

Seems to work. You may add yourself as author and myself (jorgk) as additional reviewer.

I tried using some <tt> tags as well (using ThunderHTMLedit) to make sure a mix doesn't break too badly. Off to beta we go.

Comment 22

[Wayne Mery (:wsmwk)]

this will miss 80.0b4, but we can pick it up next week in another beta.

Comment 23

[Jorg K (CEST = GMT+2)]

Any plans to land this?

Comment 24

[Magnus Melin [:mkmelin]]

Yes actually, in a few minutes.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/69b7da83da00
Fix variable width and fixed width in message compose window. r=mkmelin, jorgk DONTBUILD

Comment 26

[Jorg K (CEST = GMT+2)]

Certainly not the best commit message. Should have been something like:
Use monospace font instead of <tt> to fix various issues ...

Comment 27

[Wayne Mery (:wsmwk)]

Comment on attachment 9170096 [details] [diff] [review]
variable-width-problem-compose-window.patch

[Triage Comment]
Proactively...
Approved for beta
Approved for esr78

Comment 28

[Rob Lemley [:rjl]]

Thunderbird 80.0b5:
https://hg.mozilla.org/releases/comm-beta/rev/4e7cb379a58f

Comment 29

[WaltS48 [:walts48]]

It looks like we have winner in my testing of the 80.0b5 release candidate on Ubuntu 18.04.5 LTS

I just use whatever the default is, which is Paragraph and Variable Width in all my versions of Thunderbird.

Comment 30

[Rob Lemley [:rjl]]

Pushing the last followup to bveta(In reply to Magnus Melin [:mkmelin] from comment #20)

The cpp changes, at least, will require some adaptions to for ESR, due to the NS_LITERAL changes.

Can you elaborate?

Comment 31

[Rob Lemley [:rjl]]

Attachment: esr78.patch

Modifications for esr78. Built locally and verified functionality.

+  // Treat <font face="monospace"> as converible to plaintext.
+  if (element.LowerCaseEqualsLiteral("font")) {
+    node->GetAttribute(NS_LITERAL_STRING("face"), attribValue);
+    if (attribValue.LowerCaseEqualsLiteral("monospace")) {
+      *_retval = nsIMsgCompConvertible::Plain;
+    }
+  }
+

Comment 32

[Rob Lemley [:rjl]]

Thunderbird 78.2.0:
https://hg.mozilla.org/releases/comm-esr78/rev/29db89860a99

Comment 33

[Magnus Melin [:mkmelin]]

Looks good.