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

RESOLVED FIXED in Thunderbird 50.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: ac, Assigned: Jorg K (GMT+2))

Tracking

({regression})

unspecified
Thunderbird 50.0
regression

Thunderbird Tracking Flags

(thunderbird47 wontfix, thunderbird48 wontfix, thunderbird49 fixed, thunderbird50 fixed, thunderbird_esr4549+ fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
I can reproduce this when composing a plain text message. That's what you're doing, right?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

a year ago
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.
Summary: white space added to front of line in email after "save to drafts" → one space added to front of line starting with space when saving as draft, only when mailnews.send_plaintext_flowed is true
(Assignee)

Comment 3

a year ago
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.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 609908
(Assignee)

Comment 4

a year ago
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.
Status: RESOLVED → REOPENED
Keywords: regression
Resolution: DUPLICATE → ---
(Assignee)

Comment 5

a year ago
Tb 38 works too. So most likely a regression from the "Delsp" processing we did for the "Asian crisis", I'll investigate.
Status: REOPENED → NEW
(Assignee)

Comment 6

a year ago
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 :-(
Blocks: 1230968
(Assignee)

Comment 7

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

Comment 8

a year ago
Created attachment 8771792 [details] [diff] [review]
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.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8771792 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

a year ago
Summary: one space added to front of line starting with space when saving as draft, only when mailnews.send_plaintext_flowed is true → Space stuffing not removed when draft is edited (format=flowed)
(Assignee)

Comment 9

a year ago
Created attachment 8771802 [details] [diff] [review]
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 ;-(
Attachment #8771802 - Flags: review?(mkmelin+mozilla)

Comment 10

a year ago
(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
(Assignee)

Comment 11

a year ago
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

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

Updated

a year ago
Attachment #8771802 - Attachment is obsolete: true
Attachment #8771802 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 13

a year ago
(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.
(Assignee)

Comment 14

a year ago
Created attachment 8772193 [details] [diff] [review]
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.
Attachment #8772193 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 15

a year ago
Created attachment 8772588 [details] [diff] [review]
Patch with test included (v1b).

Test now uses innerHTML instead of textContent. This way we can check the line breaks, too.
Attachment #8772193 - Attachment is obsolete: true
Attachment #8772193 - Flags: review?(mkmelin+mozilla)
Attachment #8772588 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 16

a year ago
Created attachment 8772964 [details] [diff] [review]
Patch with test included (v1b).

Refreshed after bug 1287704 landed and changed test-drafts.js.
Attachment #8772588 - Attachment is obsolete: true
Attachment #8772588 - Flags: review?(mkmelin+mozilla)
Attachment #8772964 - Flags: review?(mkmelin+mozilla)

Comment 17

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

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

LGTM, r=mkmelin
Attachment #8772964 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 18

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

Change included in other patch.
Attachment #8771792 - Attachment is obsolete: true
(Assignee)

Comment 19

a year ago
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.
Attachment #8772964 - Flags: approval-comm-esr45?
Attachment #8772964 - Flags: approval-comm-aurora+
(Assignee)

Comment 20

a year ago
https://hg.mozilla.org/comm-central/rev/f99d81cd24ed
Status: ASSIGNED → RESOLVED
Last Resolved: a year agoa year ago
status-thunderbird47: --- → wontfix
status-thunderbird48: --- → wontfix
status-thunderbird49: --- → affected
status-thunderbird50: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
(Assignee)

Comment 21

a year ago
Created attachment 8773717 [details] [diff] [review]
Patch with test included (v1c).

Fixed an unnecessary doubled-up if-clause. This is what was landed.
Attachment #8772964 - Attachment is obsolete: true
Attachment #8772964 - Flags: approval-comm-esr45?
Attachment #8773717 - Flags: review+
Attachment #8773717 - Flags: approval-comm-esr45?
Attachment #8773717 - Flags: approval-comm-aurora+
(Assignee)

Comment 22

a year ago
TB 49 (Aurora):
https://hg.mozilla.org/releases/comm-aurora/rev/9e8fcdc63978
status-thunderbird49: affected → fixed
(Assignee)

Comment 23

11 months ago
Kent, this is actually a fix for a bad regression worth taking. It's been in TB 49 beta.
Flags: needinfo?(rkent)

Comment 24

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

http://hg.mozilla.org/releases/comm-esr45/rev/e6e1051a15d1
Flags: needinfo?(rkent)
Attachment #8773717 - Flags: approval-comm-esr45? → approval-comm-esr45+

Updated

11 months ago
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 49+
(Assignee)

Comment 25

11 months ago
Fixed bustage due to missing utility function:

https://hg.mozilla.org/releases/comm-esr45/rev/4f370f9564a6
You need to log in before you can comment on or make changes to this bug.