1336531 - <br>-Tag before signatures

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Kilian]

Attachment: sourcecode.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

1st I've chosen a default font type and size for composing HTML emails. (global settings)
2nd I added a HTML signature. (account settings) (Without signature everything works fine!)
3rd I startet composing an HTML email.


Actual results:

There are three lines in the text area of the message compose window:
line 1: empty. when I start writing, font size and type are right. (source code shows a <p></p>, when I start writing Thunderbird adds font-tags)
line 2: empty. when I start writing, Thunderbird ignores my default font size and type. (source code shows a <br>, when I start writing no font-tag is added)
line 3: my signature. everything correct.


Expected results:

There shouldn't be the line 2 (so there shoudn't be the <br>-tag)

Comment 1

[Jorg K (CEST = GMT+2)]

Right. Sadly we're placing the <br> in front of the signature traditionally, and when composing in paragraph mode (the new default since 45.x which can be switched off), it's even more noticeable.

Someone else complained before some inconsistencies, see bug 1323371.

Technical details:
We could remove the <br> after the <p> in NotifyComposeBodyReadyNew(). However, the bigger problem is the signature switch in nsMsgCompose::SetIdentity() which tries to delete a <br> before the signature since signature insertion adds this break again.

It needs to add the break, since it needs to insure that the signature starts on the next line. Sadly it's not smart enough to detect that no <br> is necessary since the preceding element is a block element <p>. (I haven't checked whether htmlEditor->InsertHTML() inserts this break).

So this isn't an easy bug to fix. I'll put it only my list.

Reporter: To understand better what I'm saying, install the ThunderHTMLedit add-on. Create a new message. Switch to HTML mode and just delete the <br>. Now change the From address to another account which has a signature. Result: The unwanted <br> comes back. And this second part is the hard part to fix.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1336531-no-br-before-sig.patch (v1)

OK, this was easier than I thought ;-)

The trouble is that we have a few tests that rely on the wrong signature behaviour and those will fail now. So let's see what fails:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=59873bbccd69d3cc7e858e147ad20511172e02af

I bet that "cloud attachment" test will fail, that already fails when someone sneezes in China ;-(

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: WIP: 1336531-no-br-before-sig.patch (v2)

There was another spot that needed treatment. However, this doesn't work at all since the <p> is not inserted into the signature. Since I removed the <br>, in NotifyComposeBodyReadyNew() and friends we insert the <p> at the selection point which the editor has placed into the <div class=moz-signature> since that's where the first relevant content is located. Grrrr.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: WIP: 1336531-no-br-before-sig.patch (v3)

Turned out harder than I thought. This fixes new message and clicking on mailto: links, with and without bodies. Reply with signature, reply above, signature also above before the quote still needs fixing. Here we still insert the <p> into the signature.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1336531-no-br-before-sig.patch (v4)

This may do it. Some test failures expected:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9a9ee9ce67b006c9825443e98614661aed3fb265
Some failures are expected.

This also needs porting to SM.

Comment 6

[Jorg K (CEST = GMT+2)]

Looking at this bug and bug 1323371 I realised that I have a problem.

In bug 1323371 someone complained that he doesn't get an empty line before the signature in a new message, and here someone complains that there is too much white-space before a signature in paragraph mode.

This area is a real pain due to the many variations:

1) New message
2) Reply
   a) with signature below the quote
   b) above the quote with signature below the quote
   c) above the quote with signature below the reply and above the quote.
3) Forward (various cases)
Multiply by two for plain text processing.
Add changing entities onto the mix so the signature needs to be switched without adding or losing white-space if possible.

Testing is limited, we have test-signature-updating.js which covers some cases and we have bug 1248045.

Let's limit ourselves to some cases and get some opinions. Here is the current behaviour:

Paragraph mode:
PA1) New message: You get *two* empty lines before the --, one in a <p> and then just a <br>
     (which is what this bug complains about).
PA2) Reply below: You get *two* empty lines before the --, one in a <p> and then just a <br>

No paragraph mode:
NP1) New message: You get *one* empty lines before the --,
     (which is what bug 1323371 about).
NP2) Reply below: You get *two* empty lines before the --.

Plain text mode:
PT1) New message: You get *one* empty lines before the --,
PT2) Reply below: You get *two* empty lines before the --.

In my patch I successfully remove the second line in case PA1) (causing a test failure in test-signature-updating.js) and then started looking at the two empty lines in the reply case. That's when I stopped and the doubts started.

So gentlemen, please answer these questions:

Should there generally be two empty lines in front of the --? In which case we WONTFIX this bug but fix bug 1323371?
Or should there only be one empty like to start typing into, so we fix this bug here and also make all the reply cases consistent.
Or should the reply cases be different?

This stuff is a mine field and has a long and sorry history, while looking at where line breaks are inserted I found bug 765803, bug 735380, bug 762413, etc.

Your answers would be much appreciated.

Comment 7

[Kilian]

Jorg, thanks for checking this, for your explanationans and for your detailed analyses!

Considering bug 1323371 there are good reasons for having this second line - but only for plain text emails and for the no-paragraph mode, because it's usual to have a blank line between two paragraphes (using <br>). But when using the paragraph mode you don't need blank lines because this is why you're using paragraph mode.

This (useless) second line in paragaph mode is accompanied by the following problem:
You get this <br> outside of a paragraph tag and in result line 1 and the signature has a "paragraph stlye" and line 2 is formatted as "normal text". When you start your email in line 1, everything is fine (apart from the fact that you have to delete the blank line after it). But when you start your email in line 2, Thunderbird ignores you default font type and size, because you're outside of a paragraph.

You have the same problem with the signature seperator, because the "--" is outside of a paragraph tag (and in result: unformatted) and ends also with a <br>-Tag, but you can suppress this sepeator seperatly in the Thunderbird config (and add it to the signature template within a <p>-tag manually - if you want). So there shouldn't be <br>-Tags outside of <p>-Tags in paragraph mode.

In my opinion it would be consistent to have this additional <br>-Tag in your NP and PT cases and not to have it in your PA cases. Can Thunderbird differentiate between paragaph mode and non-paragaph mode? Otherwise the paragraph mode wouldn't work really corect. A temporary solution would be to add a config option for suppressing this <br>-Tag like there is a config option for suppressing the signature seperator, but this wouldn't help the unskilled user which will wonder why Thunderbird ignores their default font options in line 2.

Comment 8

[Jorg K (CEST = GMT+2)]

I'm happy to remove the second line, the loose <br>, from the PA cases, in fact, that's what I spent a few hours on today. But I'd really like to know why in NP and PT, there are differences between "new" and "reply". I checked back to TB 24. These differences have always existed, so there must be a reason.

Maybe those differences should be resolved in bug 1323371.

To answer your question: Whether you get in new mail or reply in paragraph mode is determined by a preference that you can find in the compose options:
Tools > Options, Composition, General. Untick last item.
"When using paragraph format, the enter key creates a new paragraph"
Sorry about the confusing wording, we've changed that to
"Use Paragraph format instead of Body Text by default"
So yes, TB can tell these modes apart.

Let's see that the TB and SM luminaries have to say, let's add one more.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1336531-no-br-before-sig.patch (v5)

OK, this does exactly what the reporter wants. In paragraph mode there is no <br> any more between the <p> and the signature. That applies to new messages, replies and forwards. Clicking on mailto: links also still works. The test is also fixed.

The behaviour of non-paragraph mode and plaintext mode is unchanged and inconsistent described in comment #6 and repeated below. So with this patch we get:

Paragraph mode - fixed by the patch here:
PA1) New message: You get *one* empty line before the --, which is a <p>
PA2) Reply below: You get *one* empty line before the --, which is a <p>

Unchanged and inconsistent between 
No paragraph mode:
NP1) New message: You get *one* empty lines before the --,
     (which is what bug 1323371 is about).
NP2) Reply below: You get *two* empty lines before the --.

Plain text mode:
PT1) New message: You get *one* empty lines before the --,
PT2) Reply below: You get *two* empty lines before the --.

So if we accept the patch here, we can debate the inconsistencies in NP and PT in the bug 1323371. Somehow I can't see that we will change this since that has been established behaviour for years if not decades. Most likely NP and PT users will just have to hit enter if they want another line in new messages and remove a line if they don't like it in replies.

BTW, the patch has a conditional |#ifdef MOZ_THUNDERBIRD| since the mailnews/ changes would immediately smash SM.

So gentlemen, do you like this approach?

Comment 10

[Frank-Rainer Grahl (:frg)]

I am fine with it. One or two lines that is something I will think about seriously when I am running out of bugs to fix :)

FRG

Comment 11

[rsx11m]

I think the reason for two blank lines before the signature (especially in plain-text mode) was to introduce by default an empty line between the end of the text and the begin of the signature to more clearly separate the signature as such. I agree that for HTML composition in paragraph mode this line is redundant, given that the </p> leaves a space already. So, if you want to remove it in paragraph mode but leave it in for non-paragraph and plain-text composition, that's fine with me as well.

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #11)
> I think the reason for two blank lines before the signature (especially in
> plain-text mode) was to introduce by default an empty line between the end
> of the text and the begin of the signature to more clearly separate the
> signature as such.
Sure, but why don't new messages do that? You only get your empty line in replies.

Comment 13

[rsx11m]

I would think that it should be consistent - for replies, the only reason coming to my mind would be related to placement of the quote. When the quote is placed below my reply, there is still a single blank line before the signature (and two empty lines at the top of the message to start the reply); replying after the quote leaves two blank lines (one for ending the quote, the other related to the signature that follows?).

But then, attachment 8833425 [details] /does/ show a blank line in a new message? Testing this, I get a single <br> in new messages followed by the <pre> which introduces a vertical space with SM 2.46.

Confusing...

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment 8833425 [details] shows the current state in paragraph mode, see comment #6, case PA1. That's what the reporter complains about.

In non-paragraph mode and plaintext mode you get a single empty line before the signature in a new message and two in a reply. Just tested in SM 2.40 (no typo, old version, but this hasn't changed in ages). I'd still like to understand the reason, and so would the reporter of bug 1323371.

Comment 15

[rsx11m]

Then we'll have to wait until someone stops by who knows the reason.
The signature code as such has been full of inconsistencies for ages.  ;-)

Comment 16

[Joe Sabash [:JoeS1]]

I applaud your efforts to make composition more user friendly
But in my opinion, If the user composes in HTML, then he must
have some understanding of HTML. A nice feature back in the Netscape
days was the option to show <p> tags in the compose window.
(not suggesting that you implement that)
Bottom line Is, if you are not inspecting/editing the raw HTML,
then the end product is dubious at best.
File>>send later>>Preview has always been my method on complex compositions

https://bugzilla.mozilla.org/show_bug.cgi?id=1336531#c9 seems
to be a fine compromise to me.

Comment 17

[Magnus Melin [:mkmelin]]

I agree we shouldn't add the <br> for paragraph mode.

Regarding the inconsistency, it's certainly possible there were no real reason behind it... just that the whole signature feature is so over-engineered that it's complex to change the smallest thing => maybe nobody wanted to touch it.

Comment 18

[Philip Chee]

> OK, this does exactly what the reporter wants. In paragraph mode there is no
> <br> any more between the <p> and the signature. That applies to new
> messages, replies and forwards. Clicking on mailto: links also still works.
> The test is also fixed.
> 
> The behaviour of non-paragraph mode and plaintext mode is unchanged and
> inconsistent described in comment #6 and repeated below. So with this patch
> we get:
> 
> Paragraph mode - fixed by the patch here:
> PA1) New message: You get *one* empty line before the --, which is a <p>
> PA2) Reply below: You get *one* empty line before the --, which is a <p>
> 
> Unchanged and inconsistent between 
> No paragraph mode:
> NP1) New message: You get *one* empty lines before the --,
>      (which is what bug 1323371 is about).
> NP2) Reply below: You get *two* empty lines before the --.
> 
> Plain text mode:
> PT1) New message: You get *one* empty lines before the --,
> PT2) Reply below: You get *two* empty lines before the --.
> 
> So if we accept the patch here, we can debate the inconsistencies in NP and
> PT in the bug 1323371. Somehow I can't see that we will change this since
> that has been established behaviour for years if not decades. Most likely NP
> and PT users will just have to hit enter if they want another line in new
> messages and remove a line if they don't like it in replies.
> 
> BTW, the patch has a conditional |#ifdef MOZ_THUNDERBIRD| since the
> mailnews/ changes would immediately smash SM.
> 
> So gentlemen, do you like this approach?

This looks fine. It fixes one problem without regressing anything else which is better than nothing. Let's do it.

Comment 19

[Jorg K (CEST = GMT+2)]

Thanks. So far no one gave a reason for the inconsistency. Does anyone know?

Unchanged and inconsistent
No paragraph mode:
NP1) New message: You get *one* empty lines before the --,
     (which is what bug 1323371 is about).
NP2) Reply below: You get *two* empty lines before the --.

Plain text mode:
PT1) New message: You get *one* empty lines before the --,
PT2) Reply below: You get *two* empty lines before the --.

Comment 20

[Philip Chee]

Comment on attachment 8833685 [details] [diff] [review]
1336531-no-br-before-sig.patch (v5)

> +        if (firstChild.localName != 'br' &&
> +            !((firstChild.localName == "div" || firstChild.localName == "pre") &&
> +               firstChild.getAttribute("class") == "moz-signature")) {

Suggestions:
> !(/div|pre/.test(firstChild.localName) && firstChild.className.includes("moz-signature"));
> className: string with space separated classes.

> !(/div|pre/.test(firstChild.localName) && firstChild.classList.contains("moz-signature"));
> classList: DOMTokenList with built in method contains().

Comment 21

[:aceman]

(In reply to Philip Chee from comment #18)
> This looks fine. It fixes one problem without regressing anything else which
> is better than nothing. Let's do it.

So what does it mean now? Do you want it in SM too? Currently it the .js part is missing.

Comment 22

[:aceman]

Comment on attachment 8833685 [details] [diff] [review]
1336531-no-br-before-sig.patch (v5)

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

Ok, new message only has one <p> and no <br>

In reply (paragraph mode), there are 2 lines between body and quoted text and one <br> between quoted text and signature:
  <body bgcolor="#FFFFFF" text="#000000">
    <p><br>
    </p>
    <br>
    <div class="moz-cite-prefix">
</div>
<br>
<pre class="moz-signature" cols="74">-- 

Forwarded message is another mess of it own, but I assume that is not covered by the bug here.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +361,5 @@
>        let nextSibling = firstChild.nextSibling;
>        do {
> +        if (firstChild.localName != 'br' &&
> +            !((firstChild.localName == "div" || firstChild.localName == "pre") &&
> +               firstChild.getAttribute("class") == "moz-signature")) {

I've seen what Ratty said here, I would go with this avoiding a regexp:
(["div","pre"].includes(firstChild.localName) && firstChild.classList.contains("moz-signature"))

Also, there are at least 3 places in this file detecting the signature. Could you make a helper function to merge them and keep them consistent in the future?

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4525,5 @@
> +#ifdef MOZ_THUNDERBIRD
> +  bool paragraphMode = false;
> +  nsCOMPtr<nsIPrefBranch> prefBranch (do_GetService(NS_PREFSERVICE_CONTRACTID));
> +  if (prefBranch)
> +    prefBranch->GetBoolPref("mail.compose.default_to_paragraph", &paragraphMode);

You can try:
paragraphMode = mozilla::Preferences::GetBool("mail.compose.default_to_paragraph", false)

@@ +4535,5 @@
>      // should put in the appropriate HTML for inclusion, otherwise, do nothing.
>      if (m_composeHTML)
>      {
> +#ifdef MOZ_THUNDERBIRD
> +      if (!paragraphMode)

What is the value of paragraphMode we would get from SM? Would it be the right value for this to work without the ifdefs?

Comment 23

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #21)
> So what does it mean now? Do you want it in SM too? Currently it the .js
> part is missing.
They can have it when the port it and remove the MOZ_THUNDERBIRD.

(In reply to :aceman from comment #22)
> Ok, new message only has one <p> and no <br>
That's the idea.

> In reply (paragraph mode), there are 2 lines between body and quoted text
> and one <br> between quoted text and signature:
>   <body bgcolor="#FFFFFF" text="#000000">
>     <p><br>
>     </p>
>     <br>
>     <div class="moz-cite-prefix">
> </div>
> <br>
> <pre class="moz-signature" cols="74">-- 
Right. That's reply above the quote, signature below. Try reply below (and signature below). That works better now. Also try reply above with signature above. That hasn't changed, but forward is better now.

> Forwarded message is another mess of it own, but I assume that is not
> covered by the bug here.
Not sure what your saying. The idea is that the the signature, wherever it's inserted is no longer preceded by a <br> since that's no longer inserted in the C++ code. Looks like that's still not true in the case cited above, but given that, as Magnus said, the whole thing is so terribly complex, I won't touch that right now.

> I've seen what Ratty said here, I would go with this avoiding a regexp:
> (["div","pre"].includes(firstChild.localName) &&
> firstChild.classList.contains("moz-signature"))
Agreed.

> Also, there are at least 3 places in this file detecting the signature.
> Could you make a helper function to merge them and keep them consistent in
> the future?
Will do.

> You can try:
> paragraphMode =
> mozilla::Preferences::GetBool("mail.compose.default_to_paragraph", false)
That's news to me ;-) Will try.

> What is the value of paragraphMode we would get from SM? Would it be the
> right value for this to work without the ifdefs?
No. SM also supports both modes. So with the C++ change only they're hosed ;-)

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1336531-no-br-before-sig.patch (v6).

Much nicer now ;-)

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 1336531-no-br-before-sig.patch (v6a).

Killed another variable ;-)

Comment 26

[:aceman]

(In reply to Jorg K (GMT+1) from comment #23)
> > Forwarded message is another mess of it own, but I assume that is not
> > covered by the bug here.
> Not sure what your saying. The idea is that the the signature, wherever it's
> inserted is no longer preceded by a <br> since that's no longer inserted in
> the C++ code. Looks like that's still not true in the case cited above, but
> given that, as Magnus said, the whole thing is so terribly complex, I won't
> touch that right now.

There are several <br>s before the forwarded part and one <br> before the signature, but that is for another bug.
 
> > You can try:
> > paragraphMode =
> > mozilla::Preferences::GetBool("mail.compose.default_to_paragraph", false)
> That's news to me ;-) Will try.

This version is supposedly simpler and faster. There is also a mozilla::services::get*Service version, see e.g. bug 560772. So always try to use these first if they exist, when adding new do_GetService() calls.

Comment 27

[:aceman]

Comment on attachment 8836459 [details] [diff] [review]
1336531-no-br-before-sig.patch (v6a).

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

Thanks.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +377,3 @@
>  
>          insertParagraph = false;
>        } while (false);

I do not see why this is made as a do{} while{} loop instead of a if {}else{}cascade. But you do not need to bother in this bug if you do not wish to.

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 1336531-no-br-before-sig.patch (v7)

(In reply to :aceman from comment #26)
> There are several <br>s before the forwarded part ...
They are holy ;-) - #define MIME_FORWARD_HTML_PREFIX "<HTML><BODY><BR><BR>"
https://dxr.mozilla.org/comm-central/rev/424cdd3c5239dd57a3110b6468bb79ca9d005eaa/mailnews/mime/src/nsStreamConverter.h#18

(In reply to :aceman from comment #27)
> I do not see why this is made as a do{} while{} loop instead of a if
> {}else{}cascade. But you do not need to bother in this bug if you do not
> wish to.
100% right. This was somehow grown. It was using Kent's do {} while (false) to check a few things. Of course I can always bother to make things more elegant and succinct. Thanks for noticing. Care to take another look before I'll land this in the morning?

Comment 29

[:aceman]

(In reply to Jorg K (GMT+1) from comment #28)
> (In reply to :aceman from comment #27)
> > I do not see why this is made as a do{} while{} loop instead of a if
> > {}else{}cascade. But you do not need to bother in this bug if you do not
> > wish to.
> 100% right. This was somehow grown. It was using Kent's do {} while (false)
> to check a few things.

The loops are useful when there is long code which needs to be broken out of thus avoiding some if{} elses and multiple levels of indentation. But in this case if it is possible to collapse the whole loop into a 2-line if (even no else), then it looks nicer now in the patch.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: 1336531-no-br-before-sig.patch (v8).

I agonised for hours on how to make this more correct, efficient and readable.
Note: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/Empty

Comment 31

[:aceman]

Why, what needed to be changed?

Comment 32

[Jorg K (CEST = GMT+2)]

The comment wasn't optimal and the code wasn't right either:

+      if ((firstChild.localName != 'br' && !isSignature(firstChild)) ||
+          (firstChild.nextSibling && !isSignature(firstChild.nextSibling)))

If firstChild was indeed a signature, it went off to evaluate firstChild.nextSibling, where there was no need.

Also, that old code didn't match the new comment. The new code is much clearer and reflects the comment 100%.

Comment 33

[Jorg K (CEST = GMT+2)]

Or do you think this is more readable?

      if (!(firstChild.localName == "br" &&
            (!firstChild.nextSibling || isSignature(firstChild.nextSibling)) ||
             isSignature(firstChild)))
        insertParagraph = false;

compared to:

      if (firstChild.localName == "br" &&
          (!firstChild.nextSibling || isSignature(firstChild.nextSibling)))
        ; // insertParagraph is already true.
      else if (isSignature(firstChild))
        ; // insertParagraph is already true.
      else
        insertParagraph = false;

Comment 34

[:aceman]

If we wanted to be very sparse, it could end up as:
insertParagraph =
  !((firstChild.localName == "br" &&
   (!firstChild.nextSibling || isSignature(firstChild.nextSibling))) ||
  isSignature(firstChild))


But yes, your version is more readable. It appears too verbose to me, but I don't know how to make it less verbose and still readable.

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: 1336531-no-br-before-sig.patch (v9).

I agonised some more :-(

Turned out that text nodes are not DOM elements so .localName returns |undefined|. So it's better to use .nodeName.

Next I realised that there are only two "empty body" cases, not three. So I simplified the logic.

Tested with click on mailto links with and without body with and without signature. Result: mailto link with body doesn't get the paragraph, mailto link without body gets it.

Comment 36

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/a25600ed9d2e8630919133744622e33956e2e3ad
Finally.

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8836605 [details] [diff] [review]
1336531-no-br-before-sig.patch (v9).

Let's take this to the branches while it's not too late so it will ship in TB 52 in March.

Comment 38

[Jorg K (CEST = GMT+2)]

Aurora (TB 53):
https://hg.mozilla.org/releases/comm-aurora/rev/a56e5ddc34077798712708f25294a07ab42d45b2

Comment 39

[Jorg K (CEST = GMT+2)]

Beta (TB 52):
https://hg.mozilla.org/releases/comm-beta/rev/125f99f82419ea8c5692a46d2121d82bf0b321cd