1658156 - Fixed Width not working: Over-zealously reverting to fixed width although variable width was chosen - take 4

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect, P1)

Description

[Jorg K (CEST = GMT+2)]

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

Walt discovered the issue in bug 1655279 comment #20, but it wasn't regressed by that bug, it was broken before.

STR: Either bug 1655279 comment #20 on TB 80 beta 2 or on TB 78.1.1:

Open a compose window. Write some text and make it "Fixed Width". Hit enter to start a new paragraph. Select "Variable Width". Type something. It jumped back to "Fixed Width".

Comment 1

[Khushil Mistry [:khushil324]]

For "Fixed Width", we are using the editor controller to set the value. Then, execCommand("fontName", "") (empty string for variable-width)is not setting up the value. "monospace"/"tt" is still being used. To overcome this, we need to use the editor controller for variable width also.

Comment 2

[Khushil Mistry [:khushil324]]

Attachment: Bug-1658156_fixed-width-take-4-0.patch

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9169282 [details] [diff] [review]
Bug-1658156_fixed-width-take-4-0.patch

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

This is all very wonky...
The patch fixes the exact bug, but then if I make the second line variable width, then once the cursor is at the end of that line, to again add a new paragraph, it switches to "fixed width" again. 

So I'm not sure this improves things. This is why hacky fixes are a bad way to go. We might want to take a stab at the proper fix instead.

Comment 4

[Jorg K (CEST = GMT+2)]

I understand the concern, but there are time constraints. 78.2 will ship soon and we're at "take 4" here, so this is the fourth regression "a stab at the proper fix" for bug 1582410 has caused.

As I found out yesterday, there are more people on the team who use <tt> than me, so extrapolating that, it will hit a fair share of user. That said, those most likely won't switch back to variable width. The fix looks rather simple, so why not take it now and aim for a better fix in the various follow-up bugs already filed in the area.

Comment 5

[Magnus Melin [:mkmelin]]

Well like I said, I'm not sure the patch fixes things except for the particular case. I.e. doesn't seem to fix the opposite case of switching back again.
Since it's all hacky it's really hard to verify that all cases work as they should and do not break anything else.

Comment 6

[Jorg K (CEST = GMT+2)]

OK, just to understand Magnus comment #3 better:

Start with Fixed width as default. Type two lines, they are fixed width, in the third line, switch to variable and type. That comes out as variable, so "exact bug" fixed. Then select the second line, make it variable, position at the end, it says "Fixed width" again and hitting enter, you get a new line in fixed width.

And worse:
Start with Fixed width as default. Type two lines, they are fixed width, in the third line, switch to variable and type. That comes out as variable, so "exact bug" fixed. Then select the second line, make it variable, position at the end, it says "Fixed width" again. Move the cursor to the left, you get "Variable Width", not a surprise. Then move the cursor right, "Variable Width" stays, OK, then hit enter ==> Crash in my debug version. Playing around with this, I can easily produce more crashes. So "very wonky" is a nice way to describe it.

So what's the way forward?

Comment 7

[Khushil Mistry [:khushil324]]

(In reply to Jorg K (CEST = GMT+2) from comment #6)

OK, just to understand Magnus comment #3 better:

Start with Fixed width as default. Type two lines, they are fixed width, in the third line, switch to variable and type. That comes out as variable, so "exact bug" fixed. Then select the second line, make it variable, position at the end, it says "Fixed width" again and hitting enter, you get a new line in fixed width.

And worse:
Start with Fixed width as default. Type two lines, they are fixed width, in the third line, switch to variable and type. That comes out as variable, so "exact bug" fixed. Then select the second line, make it variable, position at the end, it says "Fixed width" again. Move the cursor to the left, you get "Variable Width", not a surprise. Then move the cursor right, "Variable Width" stays, OK, then hit enter ==> Crash in my debug version. Playing around with this, I can easily produce more crashes. So "very wonky" is a nice way to describe it.

So what's the way forward?

I am not able to reproduce this. Can you share a GIF or video?

Comment 8

[Jorg K (CEST = GMT+2)]

Hmm, that's a considerable effort. Magnus and I described it well enough. I'll try again.

Set composition font to "Fixed Width", then prepare this message (I used Body Text, but I guess it will fail for Paragraph, too):

This is the first line in fixed.
This is the second line in fixed, hit return and switch to variable.
This is the third line, now in variable.

Now select the second line end change it to variable via the the font selector. Then position the caret after the full stop at the end of the line, hit enter and type. We would expect variable, but get fixed.

Then repeat the entire thing and place the cater before the full stop. Then right arrow. Then hit return.

Comment 9

[Jorg K (CEST = GMT+2)]

OK, let's declare this FIXED by bug 1658999.