Closed Bug 1658999 Opened 2 years ago Closed 2 years ago

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

Categories

(MailNews Core :: Composition, defect, P1)

Tracking

(thunderbird_esr78+ fixed, thunderbird80 fixed)

VERIFIED FIXED
81 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird80 --- fixed

People

(Reporter: jorgk-bmo, Assigned: khushil324)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

+++ 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.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(khushil324)

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=

Depends on: 1659067
Duplicate of this bug: 1659067
Assignee: nobody → khushil324
Flags: needinfo?(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

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

Attachment #9169988 - Flags: review?(mkmelin+mozilla)
Attachment #9169988 - Flags: feedback?(jorgk-bmo)

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

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

Attachment #9169988 - Attachment is obsolete: true
Attachment #9169988 - Flags: review?(mkmelin+mozilla)
Attachment #9169988 - Flags: feedback?(jorgk-bmo)
Attachment #9169991 - Flags: review?(mkmelin+mozilla)
Attachment #9169991 - Flags: feedback?(jorgk-bmo)

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 :-(

Attachment #9169991 - Attachment is patch: true

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 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.

Attachment #9169991 - Flags: feedback?(jorgk-bmo)

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">

Flags: needinfo?(mkmelin+mozilla)

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.

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

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

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.

(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?

Will take a look soon.

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.
Attachment #9169991 - Flags: review?(mkmelin+mozilla) → review+

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

Attachment #9169991 - Attachment is obsolete: true
Attachment #9170096 - Flags: review?(jorgk-bmo)

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

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.

Attachment #9170096 - Flags: review?(jorgk-bmo) → review+

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

Any plans to land this?

Flags: needinfo?(mkmelin+mozilla)

Yes actually, in a few minutes.

Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Target Milestone: --- → 81 Branch

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

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

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

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

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

Attachment #9170096 - Flags: approval-comm-esr78+
Attachment #9170096 - Flags: approval-comm-beta+

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.

Status: RESOLVED → VERIFIED

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?

Flags: needinfo?(mkmelin+mozilla)
Attached patch esr78.patchSplinter Review

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;
+    }
+  }
+

Looks good.

Flags: needinfo?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.