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

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Jeff Gehlhaar]

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.

Comment 2

[rsx11m]

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.

Comment 3

[rsx11m]

Plain-text composition doesn't appear to be affected.

Comment 6

[Joshua Nixon]

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

[Jason]

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

Comment 8

[Alice0775 White]

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

Comment 9

[Brian Norris]

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

Comment 10

[Thorsteinn Kolbeinsson]

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

[Toby Banner]

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

[Toby Banner]

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

[Toby Banner]

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

[Toby Banner]

Sorry about the multiple posts.  Not sure how to delete the two duplicates.

Comment 15

[Tony Mechelynck [:tonymec]]

(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. :-)

Comment 16

[Brian Norris]

Attachment: 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.

Comment 17

[Brian Norris]

Bump! Mike, can you take a look at this? Or shall I find a different reviewer?

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Test fix

This test needs to be modified in order to accommodate the new patch.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 21

[johnvorhaus]

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.

Comment 22

[Brian Norris]

(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

Comment 23

[Brian Norris]

Just a ping: any responses from Neil or Blake on the bugfix + test case?

Comment 24

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 728622 [details] [diff] [review]
Test fix

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

Thanks,
Blake.

Comment 25

[Brian Norris]

Ping: Neil, can you review the bugfix patch? If not, what's the next step?

Comment 26

[Josiah Bruner [:jsbruner]]

Neil seems pretty busy. 

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

Comment 27

[PF4Public]

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

[LouiseRBaldwin]

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?

Comment 29

[Josiah Bruner [:jsbruner]]

Neil, could you please review this quickly? It's a two liner.

Comment 30

[PF4Public]

Obviously, it is way too difficult to fix this :(

Comment 31

[Brian Norris]

Time for an annual checkup here. Anyone listening? Can the provided patch be applied?

Comment 32

[Josiah Bruner [:jsbruner]]

If I can't get Neil to look at this later today then I think Mike or myself could probably just review it.

Comment 33

[Brian Norris]

(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?)

Comment 34

[[e-mail]]

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.)

Comment 35

[Brian Norris]

(In reply to [e-mail] 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.

Comment 36

[Brian Norris]

Attachment: Patch: adds back a newline for HTML replies (version 2)

Adding version 2 patch. Essentially the same, refactorerd to address comments. Please review, Neil.

Comment 37

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/e1e1955cce9c
https://hg.mozilla.org/comm-central/rev/64e49d15dff7

Comment 38

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

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

Comment 39

[rsx11m]

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

Comment 40

[rsx11m]

Comment on attachment 728622 [details] [diff] [review]
Test fix

[Approval Request Comment]
Test adjustments implied by attachment 728622 [details] [diff] [review].

Comment 41

[rsx11m]

(on the other hand, we have 31.5 and 31.6 left, then comes 38.0 already...)

Comment 42

[Kent James (:rkent)]

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

Comment 43

[Kent James (:rkent)]

Comment on attachment 728622 [details] [diff] [review]
Test fix

Pushed https://hg.mozilla.org/releases/comm-esr31/rev/0b5a99cc7cc3

Comment 44

[George]

Just wanted to thank those that kept at this. Seemed so simple, but obviously not. Much appreciated.