Reply above quote (no signature, HTML composition) no longer leaves a blank line

RESOLVED FIXED in Thunderbird 36.0

Status

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jbg, Assigned: computersforpeace)

Tracking

({regression})

Thunderbird 36.0
regression

Thunderbird Tracking Flags

(thunderbird36 fixed, thunderbird_esr3136+ fixed, seamonkey2.33 fixed)

Details

(Whiteboard: [gs], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.142 Safari/535.19

Steps to reproduce:

Reply to a message with the "reply above" option set


Actual results:

TB used to include at least one blank line before the reply section, now it jams the reply right up against the first line of the reply area.


Expected results:

Leave at least one blank line, or make the amount of space configurable.

Updated

6 years ago
Duplicate of this bug: 765826

Comment 2

6 years ago
Looks like a regression in the 13.0 HTML editor, possibly introduced with the new filelink feature. Steps to reproduce:

1. Compose in HTML mode
2. Don't have a signature defined
3. Set "reply above quote"

Clicking reply inserts two blank lines above the reply header in 10.0.5 ESR and places the cursor to the first one. On 14.0a2, there is only a single blank line above the <div class="moz-cite-prefix"> node.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All

Comment 3

6 years ago
Plain-text composition doesn't appear to be affected.
Summary: Reply above does not leave any blank space → Reply above quote (no signature, HTML composition) does no longer leave a blank line

Updated

6 years ago
Whiteboard: [gs]

Updated

6 years ago
Keywords: regressionwindow-wanted
Assignee: nobody → mconley

Updated

6 years ago
Duplicate of this bug: 768292

Updated

6 years ago
Duplicate of this bug: 767752

Updated

6 years ago
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
QA Contact: message-compose → composition

Comment 6

6 years ago
This occurs even if a signature is defined, so long as signature is placed below the quote.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0) Gecko/20120614 Thunderbird/13.0.1 ID:20120614161456

Comment 7

6 years ago
I noticed this recently too.  I am running 13.0.1.  I hope this gets fixed soon.  It's really annoying.

Comment 8

6 years ago
Regression window
Good:
http://hg.mozilla.org/comm-central/rev/3ad9c88900d6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120313 Thunderbird/13.0a1 ID:20120313030025
Bad:
http://hg.mozilla.org/comm-central/rev/1593bfcee4fb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120314 Thunderbird/14.0a1 ID:20120314030025
Pushlog:
http://hg.mozilla.org/comm-central/pushloghtml?fromchange=3ad9c88900d6&tochange=1593bfcee4fb

Suspected : Bug 735380
Blocks: 735380

Updated

6 years ago
Keywords: regressionwindow-wanted
(Assignee)

Comment 9

6 years ago
I can confirm that this bug still exists in 15.0. I'm using Ubuntu 12.04.

This issue has been reported on the Ubuntu launchpad tracker:
https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1021813
I am using 15.0.1 and I've been waiting for many versions for this to be fixed. I've been using Thunderbird since the very beginning and this is very annoying as it only got introduced into TB a few versions ago.  Please fix this.

Comment 11

6 years ago
I'm running Thunderbird 17.0, portable on XP, and this is STILL an issue.  Does not look like they are working on this bug too hard.  Very annoying!

Comment 12

6 years ago
duplicate
I'm running Thunderbird 17.0, portable on XP, and this is STILL an issue.  Does not look like they are working on this bug too hard.  Very annoying!

Comment 13

6 years ago
duplicate
I'm running Thunderbird 17.0, portable on XP, and this is STILL an issue.  Does not look like they are working on this bug too hard.  Very annoying!

Comment 14

6 years ago
Sorry about the multiple posts.  Not sure how to delete the two duplicates.
(In reply to Toby Banner from comment #14)
> Sorry about the multiple posts.  Not sure how to delete the two duplicates.

You can't. Just be more careful next time. :-)
(Assignee)

Comment 16

6 years ago
Created attachment 687497 [details] [diff] [review]
Path: adds back a newline for HTML replies

Since there's been no real help from Mozilla, I'm posting my own fix. This fixes the worst annoyance -- the missing newline at the start of HTML, reply-on-top mail -- but it still leaves a seemingly-extraneous newline below the quote. Please at least give this a look.
(Assignee)

Updated

6 years ago
Attachment #687497 - Flags: review?(mconley)
(Assignee)

Comment 17

6 years ago
Bump! Mike, can you take a look at this? Or shall I find a different reviewer?
Created attachment 728622 [details] [diff] [review]
Test fix

This test needs to be modified in order to accommodate the new patch.
Assignee: mconley → computersforpeace
Comment on attachment 687497 [details] [diff] [review]
Path: adds back a newline for HTML replies

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

This appears to fix the described problem, although it breaks one of the Filelink tests (which I've added a patch to address). So I like this!

I think we'll need to request review from the SM folks, since this is a mailnews change.
Attachment #687497 - Flags: review?(mconley) → review+
Attachment #687497 - Flags: review+ → review?(neil)
Comment on attachment 728622 [details] [diff] [review]
Test fix

We've added an extra line break, so we have to modify one of the tests to accommodate it. Blake, you originally reviewed these tests...does this look OK to you?
Attachment #728622 - Flags: review?(bwinton)

Comment 21

6 years ago
Is there any fix available as an ADD-IN or a SETTING CHANGE? I'm lost in code and am look for something even a STOOPID person can master. Thanks.
(Assignee)

Comment 22

6 years ago
(In reply to johnvorhaus from comment #21)
> Is there any fix available as an ADD-IN or a SETTING CHANGE? I'm lost in
> code and am look for something even a STOOPID person can master. Thanks.

Hi John (forgive me if that is not your name),

This fix is seemingly making progress toward being included in an upcoming Thunderbird release, and so it will not need an add-on or other workaround. Please be patient, and hopefully this regression will be squashed!

If, however, you want an immediate solution, I did find a workaround that sorta works (it fixes HTML mail but adds an *extra* newline for plain text mail) here:

https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1021813/comments/5
(Assignee)

Updated

6 years ago
Summary: Reply above quote (no signature, HTML composition) does no longer leave a blank line → Reply above quote (no signature, HTML composition) no longer leaves a blank line
(Assignee)

Comment 23

6 years ago
Just a ping: any responses from Neil or Blake on the bugfix + test case?
Status: NEW → ASSIGNED
Comment on attachment 728622 [details] [diff] [review]
Test fix

Yeah, this change seems okay to me, assuming it makes the test pass…

Thanks,
Blake.
Attachment #728622 - Flags: review?(bwinton) → review+
(Assignee)

Comment 25

5 years ago
Ping: Neil, can you review the bugfix patch? If not, what's the next step?
Neil seems pretty busy. 

IanN, do you have a few to review this? (Looks like this is your area)
Flags: needinfo?(iann_bugzilla)

Comment 27

5 years ago
Any news on this?

Tried user.js with
// Modify quote header //
 user_pref("mailnews.reply_header_ondate", "\nOn %s");

but it does not help
TB version 24.1.1

Comment 28

5 years ago
Small patch to fix major annoyance is just waiting review for the past year. Come on, ladies and gentlemen. How can we find someone to review this?
Neil, could you please review this quickly? It's a two liner.
Flags: needinfo?(iann_bugzilla) → needinfo?(neil)

Comment 30

4 years ago
Obviously, it is way too difficult to fix this :(
(Assignee)

Comment 31

4 years ago
Time for an annual checkup here. Anyone listening? Can the provided patch be applied?
If I can't get Neil to look at this later today then I think Mike or myself could probably just review it.
Flags: needinfo?(neil)
(Assignee)

Comment 33

4 years ago
(In reply to Josiah Bruner [:JosiahOne] from comment #32)
> If I can't get Neil to look at this later today then I think Mike or myself
> could probably just review it.

It's past "later today" on all parts of the globe ;) Can you or Mike take a look please?

FWIW, I retested against the latest, and everything patches and runs as expected.

(Now that I'm revisiting the history, it looks like Mike already added his "review+", which I think means my patch was officially "reviewed." But then he pushed it back to Neil, who hasn't responded. Can we just finish this off already?)
Flags: needinfo?(mconley)
Flags: needinfo?(josiah)
Comment on attachment 687497 [details] [diff] [review]
Path: adds back a newline for HTML replies

Sorry for dropping the ball on this.

Editor is a bit strange when it comes to breaks, in particular you need a break in an empty block so that you can put the caret there, so it creates a bogus break to achieve this.

However when you insert a break (e.g. by pressing Enter in an empty Compose window) this bogus break gets deleted, and the net result is that nothing happens, which is unhelpful.

Inserting a break by pressing Shift+Enter does force the insert of a break without deleting the bogus break, but there's no direct function to achieve this, and the one API point we have behaves differently in plain text compose.

So I think the best solution here is to move this extra break to the beginning of the if (reply_on_top == 1) block, with a suitable comment to the effect that the HTML editor eats the initial break.

(On an unrelated note, the code to detect a signature is over-engineered - any signature is already in aSignature at this point.)
Attachment #687497 - Flags: review?(neil) → feedback+
Flags: needinfo?(mconley)
Flags: needinfo?(josiah)
(Assignee)

Comment 35

4 years ago
(In reply to neil@parkwaycc.co.uk from comment #34)
> So I think the best solution here is to move this extra break to the
> beginning of the if (reply_on_top == 1) block, with a suitable comment to
> the effect that the HTML editor eats the initial break.

So you'd like essentially the same patch, just moved around and with an extra comments? I'll respin, retest, and flag you for review shortly.
(Assignee)

Comment 36

4 years ago
Created attachment 8451310 [details] [diff] [review]
Patch: adds back a newline for HTML replies (version 2)

Adding version 2 patch. Essentially the same, refactorerd to address comments. Please review, Neil.
Attachment #687497 - Attachment is obsolete: true
Attachment #8451310 - Flags: review?(neil)

Updated

4 years ago
Attachment #8451310 - Flags: review?(neil) → review+

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: [gs] → [gs][for check-in comment 13 and comment 36]
https://hg.mozilla.org/comm-central/rev/e1e1955cce9c
https://hg.mozilla.org/comm-central/rev/64e49d15dff7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0

Updated

4 years ago
Whiteboard: [gs][for check-in comment 13 and comment 36] → [gs]
(Assignee)

Updated

4 years ago
tracking-thunderbird_esr31: --- → ?

Updated

4 years ago
May I ask, will be this bug fixed in next 31.x, or waiting for the next major release?

Comment 39

4 years ago
Comment on attachment 8451310 [details] [diff] [review]
Patch: adds back a newline for HTML replies (version 2)

Let's find out, this should be simple enough for a branch landing.
Patch seems to apply to comm-esr31 by visual inspection.

[Approval Request Comment]
Regression caused by (bug #): bug 735380
User impact if declined: blank-space regression in replies for certain cases
Testing completed (on c-c, etc.): sufficient baking on all channels
Risk to taking this patch (and alternatives if risky): appears low
Attachment #8451310 - Flags: approval-comm-esr31?

Comment 40

4 years ago
Comment on attachment 728622 [details] [diff] [review]
Test fix

[Approval Request Comment]
Test adjustments implied by attachment 728622 [details] [diff] [review].
Attachment #728622 - Flags: approval-comm-esr31?

Comment 41

4 years ago
(on the other hand, we have 31.5 and 31.6 left, then comes 38.0 already...)
status-seamonkey2.33: --- → fixed
status-thunderbird36: --- → fixed
status-thunderbird_esr31: --- → affected

Updated

4 years ago
tracking-thunderbird_esr31: ? → +

Updated

4 years ago
Attachment #8451310 - Flags: approval-comm-esr31? → approval-comm-esr31+

Comment 42

4 years ago
Comment on attachment 8451310 [details] [diff] [review]
Patch: adds back a newline for HTML replies (version 2)

Pushed https://hg.mozilla.org/releases/comm-esr31/rev/8dda16e711da

Updated

4 years ago
Attachment #728622 - Flags: approval-comm-esr31? → approval-comm-esr31+

Updated

4 years ago
status-thunderbird_esr31: affected → fixed
tracking-thunderbird_esr31: + → 36+

Comment 44

4 years ago
Just wanted to thank those that kept at this. Seemed so simple, but obviously not. Much appreciated.
You need to log in before you can comment on or make changes to this bug.