Closed Bug 1653545 Opened 2 years ago Closed 2 years ago

Fixed Width not working - take 2

Categories

(MailNews Core :: Composition, defect, P1)

x86_64
All

Tracking

(thunderbird_esr78 fixed, thunderbird79 fixed)

RESOLVED FIXED
Thunderbird 80.0
Tracking Status
thunderbird_esr78 --- fixed
thunderbird79 --- fixed

People

(Reporter: jorgk-bmo, Assigned: khushil324)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Flags: needinfo?(khushil324)

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

Summary: Fixed Width new or broken behavior - take 2 → Fixed Width not working - take 2
Blocks: tb78found

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

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.

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?

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.

Then use an if statement?

Flags: needinfo?(khushil324)
Attachment #9164542 - Flags: review?(mkmelin+mozilla)

Nice! :-)

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.

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.

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

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

Monospace option.

Attachment #9164604 - Flags: review?(mkmelin+mozilla)

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.

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

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 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
Attachment #9164542 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → Thunderbird 80.0

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

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

Uplift requests, please.

Flags: needinfo?(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
Flags: needinfo?(khushil324)
Attachment #9164542 - Flags: approval-comm-esr68?
Attachment #9164542 - Flags: approval-comm-beta?
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
Attachment #9164542 - Flags: approval-comm-esr68? → approval-comm-esr78?
Comment on attachment 9164542 [details] [diff] [review]
Bug-1653545_fixed-width-not-working-0.patch

Approved for beta.
Approved for esr78
Attachment #9164542 - Flags: approval-comm-esr78?
Attachment #9164542 - Flags: approval-comm-esr78+
Attachment #9164542 - Flags: approval-comm-beta?
Attachment #9164542 - Flags: approval-comm-beta+

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.

Not yet.

Blocks: 1655279

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

Blocks: 1658156
Blocks: 1658908
Blocks: 1658999
See Also: → 1659067
You need to log in before you can comment on or make changes to this bug.