Closed Bug 1287126 Opened 8 years ago Closed 8 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

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

RESOLVED FIXED
Thunderbird 50.0
Tracking Status
thunderbird47 --- wontfix
thunderbird48 --- wontfix
thunderbird49 --- fixed
thunderbird50 --- fixed
thunderbird_esr45 49+ fixed

People

(Reporter: ac, Assigned: jorgk-bmo)

References

Details

(Keywords: regression)

Attachments

(1 file, 5 obsolete files)

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.
I can reproduce this when composing a plain text message. That's what you're doing, right?
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
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
Closed: 8 years ago
Resolution: --- → DUPLICATE
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 → ---
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
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
Looking at the patch: The removal of the space stuffing was removed here: https://hg.mozilla.org/comm-central/rev/99eb42a1fd3d#l1.25
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)
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)
Attached patch Alternative approach. (obsolete) (deleted) — Splinter Review
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)
(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
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 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+
Attachment #8771802 - Attachment is obsolete: true
Attachment #8771802 - Flags: review?(mkmelin+mozilla)
(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.
Attached patch Patch with test included (v1a). (obsolete) (deleted) — Splinter Review
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)
Attached patch Patch with test included (v1b). (obsolete) (deleted) — Splinter Review
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)
Attached patch Patch with test included (v1b). (obsolete) (deleted) — Splinter Review
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 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+
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
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+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
Attached patch Patch with test included (v1c). (deleted) — Splinter Review
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+
Kent, this is actually a fix for a bad regression worth taking. It's been in TB 49 beta.
Flags: needinfo?(rkent)
Flags: needinfo?(rkent)
Attachment #8773717 - Flags: approval-comm-esr45? → approval-comm-esr45+
Fixed bustage due to missing utility function: https://hg.mozilla.org/releases/comm-esr45/rev/4f370f9564a6
Regressions: 1689804
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: