1287126 - Space stuffing not removed when draft is edited (format=flowed)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[ac]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160623154057

Steps to reproduce:

Thunderbird version 45.2.0 32bit on Win10x64Pro_English.
German regional settings. IMAP.

1) open new message window (or edit an old email)
2) write a line with at least 1 " "-character at the beginning. (I mean the SPACE-character)
3) save the message to drafts. Close it.
4) open the message again from the drafts folder.



Actual results:

Now 1 new SPACE-character was added to the front of the line.


Expected results:

No additional SPACE should be added to the line.

Comment 1

[Jorg K (CEST = GMT+2)]

I can reproduce this when composing a plain text message. That's what you're doing, right?

Comment 2

[Jorg K (CEST = GMT+2)]

I looked at this a little more. When mailnews.send_plaintext_flowed is set to true (default), then every line that starts with a space gets one space prepended when saving as draft.

So
 1
  2
   3
4
becomes
  1
   2
    3
4

Looks like a serialiser problem, I'll do a little debug.

Comment 3

[Jorg K (CEST = GMT+2)]

OK, this is not a new bug, it was already reported here: Bug 609908.

The thing is: When looking at the message, the extra spaces actually don't show up in the display. However, in the message source they are present and when editing the draft again, the extra spaces are put into the composition.

The key here is bug 609908 comment #3 and
http://tools.ietf.org/html/rfc3676#section-4.4

Adding the space is actually correct and not showing it on display is also correct.

The bug is that when editing the draft, the space should equally be removed.

Comment 4

[Jorg K (CEST = GMT+2)]

Sorry, I closed bug 609908 since it complained about the space stuffing in general which works as intended.

I'm reopening this bug. I tried TB 31. When editing the draft, the extra spaces are removed. Not so in current trunk. So this is a regression.

Comment 5

[Jorg K (CEST = GMT+2)]

Tb 38 works too. So most likely a regression from the "Delsp" processing we did for the "Asian crisis", I'll investigate.

Comment 6

[Jorg K (CEST = GMT+2)]

OK, my worst fear has become true. This is a regression from bug 1230968.
There we landed a patch from Hiro (which we really never understood):
https://hg.mozilla.org/comm-central/rev/99eb42a1fd3d
This patch originated from bug 26734 (attachment 8537118 [details] [diff] [review]).

And now, more than half a year later a regression was found. Great :-(

Comment 7

[Jorg K (CEST = GMT+2)]

Looking at the patch: The removal of the space stuffing was removed here:
https://hg.mozilla.org/comm-central/rev/99eb42a1fd3d#l1.25

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: restore removal of space stuffing removed in changeset 99eb42a1fd3d

OK, the easy part is to confirm that the patch is fixing the problem reported here.

We also need to confirm that partly undoing the fix of bug 1230968 doesn't regress that bug. Note that bug 1230968 also fixed bug 1230971 (see bug 1230968 comment #11), so we need to make sure that 1230971 isn't regressed.

Done: Basically we need to "edit as new" and (shift/plaintext) forward attachment 8696494 [details].
I did both and neither bug 1230968 nor bug 1230971 regressed.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: Alternative approach.

Sorry Magnus, here comes a second option.

We observe that Hiro removed the processing for space stuffing here:
https://hg.mozilla.org/comm-central/rev/99eb42a1fd3d#l1.25

The comment below reads:
If we are processing a flowed plain text line, we need to parse the
line in mimeInlineTextPlainFlowedClass.

OK, that's a great concept. Only that this class looks for space stuffing but doesn't act on it.

So I'm enhancing Hiro's addition:
https://hg.mozilla.org/comm-central/rev/99eb42a1fd3d#l2.22
and make it take the space stuffing into account.

Perhaps that's the cleaner way.

Which one do you prefer? I know we both prefer not to look at that ugly MIME stuff ever again ;-(

Comment 10

[Alessandro Vesely]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #4)
> Sorry, I closed bug 609908 since it complained about the space stuffing in
> general which works as intended.

I'm not clear how its working is intended to be.  FWIW, I asked here:
https://mailarchive.ietf.org/arch/msg/ietf-822/JdGz69-kLgvDVp3bJSJMQ8N1PTI

Undoing any space-stuffing is needed for both proper displaying and edit as new.  That requires to know how to treat leading spaces.  BTW, I can edit-as-new messages created by different clients, which may do space-stuffing in their own way.

Ale

Comment 11

[Jorg K (CEST = GMT+2)]

Let's have this discussion in bug 609908. This bug here is a clear regression (something that worked sadly broke) so editing the draft stopped working.

The code simply removes one leading space for format=flowed messages.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 8771792 [details] [diff] [review]
restore removal of space stuffing removed in changeset 99eb42a1fd3d

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

I like this better. Could you add a test so it doesn't regress again?

::: mailnews/mime/src/mimemsg.cpp
@@ +181,5 @@
> +      // Remove any stuffed space.
> +      if (length > 0 && ' ' == *line && mime_typep(kid, (MimeObjectClass *)&mimeInlineTextPlainFlowedClass))
> +      {
> +        line ++;
> +        length --;

please no extra spaces (hah!) before ++ an --

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #12)
> please no extra spaces (hah!) before ++ an --
I restored the original code:
https://hg.mozilla.org/comm-central/rev/99eb42a1fd3d#l1.28
;-)

Test coming up.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Patch with test included (v1a).

OK, here comes the test.

Note that changes to test-drafts.js won't apply until bug 1169184 is landed. You can of course manually add the test to that file at the end.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Patch with test included (v1b).

Test now uses innerHTML instead of textContent. This way we can check the line breaks, too.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: Patch with test included (v1b).

Refreshed after bug 1287704 landed and changed test-drafts.js.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 8772964 [details] [diff] [review]
Patch with test included (v1b).

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

LGTM, r=mkmelin

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8771792 [details] [diff] [review]
restore removal of space stuffing removed in changeset 99eb42a1fd3d

Change included in other patch.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8772964 [details] [diff] [review]
Patch with test included (v1b).

Let's uplift this since it's a really ugly regression with data loss consequences since we obliterate the draft of a user.

For TB 45:

[Approval Request Comment]
Regression caused by (bug #): Bug 1230968
User impact if declined: Data loss, draft obliterated
Testing completed (on c-c, etc.): In test suite
Risk to taking this patch (and alternatives if risky):
Low, just restoring previous code.

Comment 20

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/f99d81cd24ed

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: Patch with test included (v1c).

Fixed an unnecessary doubled-up if-clause. This is what was landed.

Comment 22

[Jorg K (CEST = GMT+2)]

TB 49 (Aurora):
https://hg.mozilla.org/releases/comm-aurora/rev/9e8fcdc63978

Comment 23

[Jorg K (CEST = GMT+2)]

Kent, this is actually a fix for a bad regression worth taking. It's been in TB 49 beta.

Comment 24

[Kent James (:rkent)]

Comment on attachment 8773717 [details] [diff] [review]
Patch with test included (v1c).

http://hg.mozilla.org/releases/comm-esr45/rev/e6e1051a15d1

Comment 25

[Jorg K (CEST = GMT+2)]

Fixed bustage due to missing utility function:

https://hg.mozilla.org/releases/comm-esr45/rev/4f370f9564a6