1653545 - Fixed Width not working - take 2

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect, P1)

Description

[Jorg K (CEST = GMT+2)]

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

Bug 1622231 comment #20:

Yes, regressed again.

The 2nd regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=b3eb5b294a0826a1843121fd21bd84a501863389&tochange=813ca5b455a81aa0d94b0ccc9e4d4f7261e45a8e
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b2df79a80c0303df9d710800ae37dce56847eef5&tochange=590d76562067863dd840c9ff7cf85d5e8e2d6b4d

Comment 1

[Jorg K (CEST = GMT+2)]

Actual result: <font face="tt">jkjkjk</font>
Expected: <tt>jkjkjk</tt>

Comment 2

[Massimiliano Caniparoli]

Actually, my TB 79 does NOT inject the <font face="tt">, but instead it does nothing

Comment 3

[Magnus Melin [:mkmelin]]

I don't think we really want the <tt> though, that's obsolete old junk. I guess the proper replacement would be something like <span style="font-family:monospace">foo</span>. Maybe there's a suitable attribute as well.

Comment 4

[Jorg K (CEST = GMT+2)]

How about you fix the regression and then leave "modernisation" for later. Masayuki made <tt> work again in the original bug. In general, the editor doesn't use CSS, so why start now in a regression fix?

Comment 5

[Magnus Melin [:mkmelin]]

My suspicion is <tt> can't be done using document.execCommand, and that is the reason it broke. I didn't really look around tough. I agree a css solution could lead to other problems.

Comment 6

[Jorg K (CEST = GMT+2)]

Then use an if statement?

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1653545_fixed-width-not-working-0.patch

Comment 8

[Jorg K (CEST = GMT+2)]

Nice! :-)

Comment 9

[Magnus Melin [:mkmelin]]

Would just changing "tt" to "monospace" instead fix it? https://searchfox.org/comm-central/rev/b6b6fbded631b66339ea2e04e863cd516a47f210/mail/components/preferences/compose.inc.xhtml#102 If so that could be preferable.

Comment 10

[Jorg K (CEST = GMT+2)]

monospace is a font-family name, <tt> is "teletype text". You're likely to break things relying on <tt>. Is that worth risking? Without the regression, you would have never noticed, so why try to push a "modernisation" here? To do it properly, you should announce that you're discontinuing <tt> and replace it by something else.

Comment 11

[Magnus Melin [:mkmelin]]

Besides <tt> being an obsolete element, it's semantically incorrect to use it for what we use it for. The menu just says "Fixed width". As a user I would expect Thunderbird to apply a fixed with font, nothing else.

Comment 12

[Jorg K (CEST = GMT+2)]

Magnus, <font> is equally obsolete - https://www.w3schools.com/tags/tag_font.asp. Would you therefore like to replace the font/tt-based Mozilla editor with a modern CSS-based one in this bug (because that really IS what a user would expect in a modern product (Postbox has it))? That said, as a user I would expect Thunderbird NOT to break left, right and centre during all this poorly quality-controlled modernisation efforts. The user generally doesn't care what happens behind the scenes as long as it works.

If you change <tt> to <style="font-family:monospace"> you will cause the next issue in converting to plain text:
https://searchfox.org/comm-central/rev/b6b6fbded631b66339ea2e04e863cd516a47f210/mailnews/compose/src/nsMsgCompose.cpp#4983
https://searchfox.org/comm-central/rev/b6b6fbded631b66339ea2e04e863cd516a47f210/mailnews/compose/src/nsMsgCompose.cpp#4992

Would you like to cause another regression in this chain or re-write that code as well while were here?

Excuse the sarcasm in this post, but I'm really disappointed how quality is (not) managed in this product: We started with bug 1625218 and bug 1622231. Then you promoted a lot of modernisation in bug 1582410 (some of which fortunately was moved to follow-up bug, see bug 1582410 comment #40 - or wasn't it?? - after a lot of my involvelment), and here we are again with the third very visible regression, and instead of just returning to a simple working version in the shipped product, you're promoting more change with at least one immediate side-effect as I pointed out above :-( :-( :-(

Comment 13

[Massimiliano Caniparoli]

I am another fan of modernization, and there would be nice new tags to replace the <tt>: code, samp, etc, but I agree with Jorg for at least two reasons:

1. we are talking about a mail agent, and there are several obsolete mail agents around (one in particular is very popular and not free). I'd love to be able to send wonderful animated CSS3/HTML5 mails, but we are not on the web: here, the sender needs the recipient to receive and read the message whatever agent he's using, while on the web the user is forced use another browser to avoid being cut out of the world.
2. the purpose here is solving the regression. The long and hard modernisation process may be faced in another issue

Comment 14

[Khushil Mistry [:khushil324]]

Attachment: Bug-1653545_fixed-width-not-working-monospace-0.patch

Monospace option.

Comment 15

[Jorg K (CEST = GMT+2)]

Have you tried sending a HTML mail with fixed width? Did it get downgraded to plain text? See comment #12. If I read the code correctly, font="whatever" will stop the downgrade.

Comment 16

[Jim Mason]

Thank you, Khushil! Your first patch works for me. The original <tt> semantics are restored.

Haven't tried the second patch, as my reason for being here was to find a fix, which I now have :-) Thanks again.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 9164604 [details] [diff] [review]
Bug-1653545_fixed-width-not-working-monospace-0.patch

Breaks downgrade to plaintext, and I said so, but your ignored that comment :-(

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: When I try to fix the regression, comm-central didn't use document.execCommand for <tt>. Therefore, I just test the principal not strictly, but much simpler.
https://searchfox.org/mozilla-central/rev/b2395478c6adf6e5b241be1610fb1d920ed995ed/editor/libeditor/HTMLStyleEditor.cpp#107-112
So, checking the principal strictly, document.execCommand called by chrome script can keep working as traditional behavior.

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9164542 [details] [diff] [review]
Bug-1653545_fixed-width-not-working-0.patch

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

Oh well, let's take this to fix the regression. Then file another bug to fix it better going forwards

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7ba00670f8c3
make Fixed Width (<tt>) work again. r=mkmelin DONTBUILD

Comment 21

[Jorg K (CEST = GMT+2)]

Uplift requests, please.

Comment 22

[Khushil Mistry [:khushil324]]

Comment on attachment 9164542 [details] [diff] [review]
Bug-1653545_fixed-width-not-working-0.patch

[Approval Request Comment]
Regression caused by (bug #): 1582410
User impact if declined: Fixed width Font in the Editor is not working and it will cause problems for users who are depending upon it and they will get confuse if it's not working. 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky): Low

Comment 23

[Rob Lemley [:rjl]]

Comment on attachment 9164542 [details] [diff] [review]
Bug-1653545_fixed-width-not-working-0.patch

Uplift should have been requested for comm-esr**78**.

[Approval Request Comment]
Regression caused by (bug #): 1582410
User impact if declined: Fixed width Font in the Editor is not working and it will cause problems for users who are depending upon it and they will get confuse if it's not working. 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky): Low

Comment 24

[Wayne Mery (:wsmwk)]

Comment on attachment 9164542 [details] [diff] [review]
Bug-1653545_fixed-width-not-working-0.patch

Approved for beta.
Approved for esr78

Comment 25

[Massimiliano Caniparoli]

Don't know whether this change was released in 79.0b2 (which I just installed), but it's now back to <font face="tt">, while 79.0b1 didn't inject anything on my end.

Comment 26

[Jorg K (CEST = GMT+2)]

Not yet.

Comment 27

[Rob Lemley [:rjl]]

Thunderbird 79.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/6523c360f41f

Comment 28

[Jorg K (CEST = GMT+2)]

Would it be possible for developer and reviewer to do a minimum amount of testing? This is STILL not fixed, see bug 1655279.

Comment 29

[Rob Lemley [:rjl]]

Thunderbird 78.1.0:
https://hg.mozilla.org/releases/comm-esr78/rev/203cf274ccff