Identity/signature switch doesn't cope with paragraph format and is also broken on reply and forward (see comment #2).

RESOLVED FIXED in Thunderbird 47.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
2 years ago
a year ago

People

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

Tracking

Trunk
Thunderbird 47.0

Thunderbird Tracking Flags

(thunderbird45 fixed, thunderbird46 fixed, thunderbird47 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

2 years ago
See bug 330891 comment #116.
(Assignee)

Updated

2 years ago
Keywords: regression
(Assignee)

Comment 1

2 years ago
This is in fact *not* a regression, it was only exposed by bug 330891 and particularly by the failing of mail/test/mozmill/composition/test-signature-updating.js.

The condition under which the signature switch works suboptimally can be created by inserting a paragraph manually.

I will look at it during the Aurora period.
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Identity/signature switch doesn't cope with paragraph format, regression from bug 330891 → Identity/signature switch doesn't cope with paragraph format, exposed by bug 330891

Updated

2 years ago
Blocks: 330891
status-thunderbird45: --- → affected
tracking-thunderbird45: --- → ?

Updated

2 years ago
Keywords: regression
Summary: Identity/signature switch doesn't cope with paragraph format, exposed by bug 330891 → Identity/signature switch doesn't cope with paragraph format

Updated

2 years ago
Keywords: regression
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
The signature switch it a *HUGE* mess. I'm running TB 38 to see what works and what doesn't:

1) New message:
   Switching identities/signatures works, but stops working when a paragraph is inserted (paragraph format).
   That we noticed when fixing bug 330891.

2) Reply above the quote, signature also above the quote:
   Switch works once, then signatures pile up.

3) Reply above the quote, signature below the quote:
   After the first switch, the signature moves *into* the quote, after that, signatures pile up inside the quote.

4) Reply below the quote, signature below the quote:
   Same behaviour as for new message.

5) Forward, signature above the quote:
   Accumulates signatures.

6) Forward, signature below the inlined forwarded message:
   Switch doesn't work at all.

Of the six cases, two work, "new message" and reply below quote. These both stop working if the user (or the system) switches to paragraph format. The test mail/test/mozmill/composition/test-signature-updating.js covers the one case that works: "New message" without paragraph format.

I promised Aleth to look into it, but this will have to wait until 2016.

Looking at the code at
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#5335
the failures can be understood. The code uses nsHTMLEditor::BeginningOfDocument() and nsHTMLEditor::EndOfDocument() and these position right into a paragraph or block quote or <div class="moz-cite-prefix">. Once the signature is placed inside a block element, the primitive search for the signature which reverse traverses the first level children of the root document doesn't find the signature since has become a second level descendant of the root.

So the approach will be to position the insertion point more carefully, so that the signature is always a first level child.
(Assignee)

Updated

2 years ago
Summary: Identity/signature switch doesn't cope with paragraph format → Identity/signature switch doesn't cope with paragraph format and is also broken on reply and forward (see comment #2).
(Assignee)

Comment 3

a year ago
Created attachment 8714129 [details] [diff] [review]
Proposed solution (v1).

Aceman, would you like to go on a little trip to nsMsgCompose.cpp with me. It's ugly down there!

The first thing I did was trying to understand the parameters to
nsMsgCompose::ProcessSignature(nsIMsgIdentity *identity, bool aQuoted, nsString *aMsgBody)

The second parameter seems to have to do with whether the message we're processing is quoted.

Whether dashes/separator are added before the signature is determined here:
      if ((mType == nsIMsgCompType::NewsPost || !suppressSigSep) &&
          (reply_on_top != 1 || sig_bottom || !aQuoted)) {
        sigOutput.AppendLiteral(dashes);
      }
I couldn't find a setting in the identity to suppress the signature separator/dashes, so let's assume the first line always comes out true.

Then you get NO dashes if !(reply_on_top != 1 || sig_bottom || !aQuoted) or in other words:
NO dashes if reply_on_top == 1 && !sig_bottom && aQuoted.
That means NO dashes if you're replying/forwarding on top and the signature is on top as well and the thing is quoted. Quoted applies to replies and inline forward.

Sadly the second parameter misleadingly had two different names in other functions:
"noDelimiter" and "addDashes".
So I changed that to "isQuoted" to make it clearer.

The next thing I did to improve the situation was to replace calls to nsEditor::BeginningOfDocument() and nsEditor::EndOfDocument() with my own functions.
The nsEditor functions will position where we don't want it, with the result that the signature can be placed inside the <div class="moz-forward-container"> (on forward) or the <blockquote> (on reply).

That's the main part of the patch.

Then I removed two things that didn't make sense at all:
-    if (sigOnTop && noDelimiter)
+    if (sigOnTop)
and
-    if (sigOnTop && noDelimiter)
-      m_editor->EndOfDocument();

So that's it.

I've tested the following cases, both in "body text" mode and "paragraph" mode.

1) New message (signature gets dashes).
2) Reply above, signature above (signature gets NO dashes):
   This really makes no sense, since signature will be inserted at the very top,
   even before a reply that was already typed.
3) Reply above the quote, signature below the quote (with dashes).
4) Reply below the quote, signature below the quote (with dashes).
5) Forward, signature above the quote and (NO dashes)
6) Forward, signature below the inlined forwarded message (with dashes).
   Adds a signature. This is because the signature is wrapped in the
   <div class="moz-forward-container">, see bug 736584 comment #2,
   and the code that's tries to remove a pre-existing signature only looks at
   the top DOM level.

Sadly cases 5) and 6) only work after the first swap. This is due to the crazy inline forwarding processing here:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/cloudAttachmentLinkManager.js#141
This is on my list of things to fix.

The proposed solution doesn't make things perfect, but it moves them significantly towards being useful. At the moment I can create situations where every identity switch adds a new signature.

With this patch that doesn't happen any more and when bug 736584 is fixed, it will be even better.

Once you agree with the code change, I will make mail/test/mozmill/composition/test-signature-updating.js a little better, but I most certainly won't add all the cases above.
Attachment #8714129 - Flags: review?(acelists)
(Assignee)

Updated

a year ago
No longer blocks: 330891
status-thunderbird46: --- → affected
status-thunderbird47: --- → affected

Comment 4

a year ago
Comment on attachment 8714129 [details] [diff] [review]
Proposed solution (v1).

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

I haven't yet run with this but the patch seems sound. I'm going to see how the sigs are placed.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +5379,5 @@
>    return NS_OK;
>  }
>  
> +/**
> + * nsEditor::BeginningOfDocument() will position to the beginning of the document

It seems we do not put the function name into the comments in this file.

@@ +5388,5 @@
> +nsMsgCompose::BeginningOfDocument(void)
> +{
> +  int32_t offset;
> +  nsCOMPtr<nsIDOMElement> rootElement;
> +  nsCOMPtr<nsIDOMNode> lastNode;

Is this variable used?

@@ +5399,5 @@
> +
> +  nsCOMPtr<nsISelection> selection;
> +  m_editor->GetSelection(getter_AddRefs(selection));
> +  if (selection)
> +    selection->Collapse(rootElement, 0);

May this fail?

@@ +5411,5 @@
> + * after the last container so we don't accidentally position into a
> + * <blockquote>.
> + */
> +nsresult
> +nsMsgCompose::EndOfDocument(void)

If the functions do not return the "end" or "beginning" of the document, they should probably have a better name. Like

@@ +5553,2 @@
>      else
> +      EndOfDocument();

Can you check the return values for failure?

@@ +5555,3 @@
>  
>      nsCOMPtr<nsIHTMLEditor> htmlEditor (do_QueryInterface(m_editor));
>      nsCOMPtr<nsIPlaintextEditor> textEditor (do_QueryInterface(m_editor));

Not your fault, but it seems these variables could be moved into their respective branch of the 'if' below so that they are not instantiated when not needed.
Of course InsertDivWrappedTextAtSelection() creates its own version of them again (and the textEditor at http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#531 seems to be unused again).

@@ -5500,5 @@
>        InsertDivWrappedTextAtSelection(aSignature, NS_LITERAL_STRING("moz-signature"));
>      }
> -
> -    if (sigOnTop && noDelimiter)
> -      m_editor->EndOfDocument();

Any idea what was this actually doing? Position at the end and then do nothing there? Is that "positioning" visible to the user? Like moved cursor or something?
Attachment #8714129 - Flags: feedback+
(Assignee)

Comment 5

a year ago
Created attachment 8715940 [details] [diff] [review]
Proposed solution (v1b).

I have addressed your comments. Please not that nsEditor::BeginningOfDocument() was used before and it didn't do what we need, so now I wrote a replacement which does what we need. I hope the comment makes that clear now.

> > m_editor->EndOfDocument();
> Any idea what was this actually doing?
Yes:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsEditor.cpp#1082
It will collapse the selection at the end of the document (but possibly inside a container element, like <blockquote>). Putting the selection there should place the caret there, but during the identity/signature switch the editable area seems to lose focus, so it doesn't matter where the selection is placed. It certainly makes no sense to place it at the end of the document.
Attachment #8714129 - Attachment is obsolete: true
Attachment #8714129 - Flags: review?(acelists)
Attachment #8715940 - Flags: review?(acelists)

Comment 6

a year ago
Comment on attachment 8715940 [details] [diff] [review]
Proposed solution (v1b).

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

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +5386,5 @@
> + * so we don't accidentally position into <div class="moz-forward-container">.
> + * That's why we use our own function.
> + */
> +nsresult
> +nsMsgCompose::BeginningOfDocument(void)

Yeah, I didn't finish my comment here. What I wanted to write:
If the functions do not return the "end" or 
"beginning" of the document, they should probably have a better name. Like MoveToBeginningOfDocument().
Attachment #8715940 - Flags: feedback+
(Assignee)

Comment 7

a year ago
Sure. But I won't submit another patch just to rename two functions. Please review the patch and I'll fix it with any other issues that might arise.
(Assignee)

Comment 8

a year ago
Created attachment 8716477 [details] [diff] [review]
Proposed solution (v2).

As discussed on IRC, there was something wrong when switching in new messages. That's due to this line:
-    if (sigOnTop && noDelimiter)
+    if (sigOnTop && isQuoted) {

Previously I had removed the "&& isDelimiter" completely, see interdiff. Now I added a comment so we understand what's going on.
Attachment #8715940 - Attachment is obsolete: true
Attachment #8715940 - Flags: review?(acelists)
Attachment #8716477 - Flags: review?(acelists)
(Assignee)

Comment 9

a year ago
Quoting from IRC:
Now all the patch does is replace the M-C version of "move to the front/end" with my own. That will avoid placing signatures inside <div> or <blockquote> blocks.

Further improvements to be had from bug 736584.
(Assignee)

Comment 10

a year ago
I've just been thinking:
    if (sigOnTop && isQuoted) {
      rv = MoveToBeginningOfDocument();
Maybe we should not move to the very top but just before a <div class="moz-forward-container"> or <blockquote> so that the replacement signature won't be placed above some existing body text.

Right now we get this:
Newly entered text
Old signature
Quoted or forwarded part.

After replacement:
New signature
Newly entered text
Quoted or forwarded part.

Would be nice to get:
Newly entered text
New signature
Quoted or forwarded part.

So I'd create a function MoveToAboveQuote() which would default to moving to the very top if no quote/forwarded part is found.

Remember, "signature above" is NOT the recommended option in the account settings where it says:
below the quote (recommended)
below my reply (above the quote).

Worth doing? It's a lot of sophistication/complexity/complication for a case that is rarely used. How many times does one change identity and how many times does the signature need to change? Given the current horrible state of the signature switch and zero complaints, it's not the most pressing issue.

Comment 11

a year ago
(In reply to Jorg K - Let the purists rule (GMT+1) from comment #10)
> Right now we get this:
> Newly entered text
> Old signature
> Quoted or forwarded part.
> 
> After replacement:
> New signature
> Newly entered text
> Quoted or forwarded part.
> 
> Would be nice to get:
> Newly entered text
> New signature
> Quoted or forwarded part.
> 

Yes, this is a scenario that still fails with the previous patch.
If this can't be easily fixed here, you can make a new bug.

> Worth doing? It's a lot of sophistication/complexity/complication for a case
> that is rarely used. How many times does one change identity and how many
> times does the signature need to change? Given the current horrible state of
> the signature switch and zero complaints, it's not the most pressing issue.

There are people who use identities to store different signatures (e.g. work and private). Surely they switch the identities often.

Comment 12

a year ago
Comment on attachment 8716477 [details] [diff] [review]
Proposed solution (v2).

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

So it seems switching identities with signatures on top/bottom now works in new message.

A case where signature gets on top before the reply and quote was described by Jorg is not covered here.

There is still the case that you can get the state:
signature1
text
signature2
forwarded text

As I understand it that is bug 736584.

So it looks to me now.

Can you make some tests now, at least for the common/recommended cases?

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +5391,5 @@
> +{
> +  nsCOMPtr<nsIDOMElement> rootElement;
> +  nsresult rv = NS_OK;
> +
> +  rv = m_editor->GetRootElement(getter_AddRefs(rootElement));

You can merge these lines.

@@ +5418,5 @@
> +  nsCOMPtr<nsIDOMElement> rootElement;
> +  nsCOMPtr<nsIDOMNode> lastNode;
> +  nsresult rv = NS_OK;
> +
> +  rv = m_editor->GetRootElement(getter_AddRefs(rootElement));

Merge.

@@ +5550,5 @@
>      bool sigOnTop = (reply_on_top == 1 && !sig_bottom);
> +    if (sigOnTop && isQuoted) {
> +      rv = MoveToBeginningOfDocument();
> +    } else {
> +      // Note: New messages aren't quoted to we always move to the end.

'to' ?
Attachment #8716477 - Flags: review?(acelists) → review+
(Assignee)

Comment 13

a year ago
Thanks for the review.

(In reply to :aceman from comment #11)
> Yes, this is a scenario that still fails with the previous patch.
> If this can't be easily fixed here, you can make a new bug.
OK, let's fix it now while we're here. Coming up in the next few days.
(Assignee)

Comment 14

a year ago
Created attachment 8716614 [details] [diff] [review]
Proposed solution (v3).

Improved processing to position before quoted content but not right at the beginning of the document.
Attachment #8716477 - Attachment is obsolete: true
Attachment #8716614 - Flags: review?(acelists)

Comment 15

a year ago
Comment on attachment 8716614 [details] [diff] [review]
Proposed solution (v3).

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

This now works nicely for me, thanks.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +5389,5 @@
> +nsMsgCompose::MoveToAboveQuote(void)
> +{
> +  int32_t offset = 0;
> +  nsCOMPtr<nsIDOMElement> rootElement;
> +  nsCOMPtr<nsIDOMNode> node;

The function is getting longer, please move the variables to where they are used.

@@ +5391,5 @@
> +  int32_t offset = 0;
> +  nsCOMPtr<nsIDOMElement> rootElement;
> +  nsCOMPtr<nsIDOMNode> node;
> +  nsresult rv = m_editor->GetRootElement(getter_AddRefs(rootElement));
> +  if (NS_FAILED(rv) || nullptr == rootElement) {

Later on you use !node to check for null. Can you see what is the preferred style in the file and use that?

@@ +5405,5 @@
> +  while (NS_SUCCEEDED(rv) && node) {
> +    nsCOMPtr<nsIDOMElement> element = do_QueryInterface(node);
> +    if (element) {
> +      // First check for <blockquote>. This will most likely not trigger
> +      // since well-behaved quotes are preceded by a cite prefix. 

Trailing space.

@@ +5428,5 @@
> +        break;
> +      }
> +
> +      rv = node->GetNextSibling(getter_AddRefs(node));
> +      if (!node || NS_FAILED(rv)) {

The style is to check NS_FAILED() first.

@@ +5600,5 @@
> +    } else {
> +      // Note: New messages aren't quoted so we always move to the end.
> +      rv = MoveToEndOfDocument();
> +    }
> +    NS_ENSURE_SUCCESS(rv, rv);

Is it OK if we exit here without calling m_editor->EndTransaction() ? Let's be safe and let in case of NS_FAILED(rv) just skip the inserting of sig below.
Attachment #8716614 - Flags: review?(acelists) → review+
(Assignee)

Comment 16

a year ago
Created attachment 8716629 [details] [diff] [review]
Proposed solution (v3a).

Would you mind checking whether I covered everything to your liking?

Personally I find "nullptr == rootElement" quite silly. Since my new code has more of these checks than the rest of the file, I decided to set the style ;-)
Attachment #8716614 - Attachment is obsolete: true
Attachment #8716629 - Flags: review?(acelists)

Comment 17

a year ago
Comment on attachment 8716629 [details] [diff] [review]
Proposed solution (v3a).

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

Yes I think it looks now. Can you make the tests now? :)
Attachment #8716629 - Flags: review?(acelists) → review+
(Assignee)

Comment 18

a year ago
(In reply to :aceman from comment #17)
> Yes I think it looks now. Can you make the tests now? :)
Yep, coming up in the next few days.
(Assignee)

Comment 19

a year ago
Created attachment 8716668 [details] [diff] [review]
Adapted test (v1).

So here is the updated test that Aleth always wanted. The tests are now run additionally in paragraph format and work there as well.
Attachment #8716668 - Flags: review?(acelists)

Comment 20

a year ago
Comment on attachment 8716668 [details] [diff] [review]
Adapted test (v1).

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

So what about adding some test for signature on reply and on forward and switching from sig on top to sig on bottom? :) Yeah, I think one of the cases does not work and has a separate bug. So that case can be tested there.

::: mail/test/mozmill/composition/test-signature-updating.js
@@ +43,4 @@
>  };
>  
>  function teardownModule(module) {
>    Services.prefs.clearUserPref("editor.CR_creates_new_p");

Please clear also the mail.identity.id* prefs changed in the tests.

@@ +150,3 @@
>  // See bug TBD
>  //function testPlaintextComposeWindowSwitchSignaturesWithSuppressedSeparator() {
>  //  plaintextComposeWindowSwitchSignatures(true);

Will this one not work now after your patch?
(Assignee)

Comment 21

a year ago
(In reply to :aceman from comment #20)
> So what about adding some test for signature on reply and on forward and
> switching from sig on top to sig on bottom? :)
I don't intend to do that as I said at the bottom of comment #3 :-(

> Please clear also the mail.identity.id* prefs changed in the tests.
Will do.

> @@ +150,3 @@
> >  // See bug TBD
> >  //function testPlaintextComposeWindowSwitchSignaturesWithSuppressedSeparator() {
> >  //  plaintextComposeWindowSwitchSignatures(true);
> 
> Will this one not work now after your patch?
This is absolutely puzzling. In TB 45 and in the patched version, I can successfully switch signatures on a new message when suppressing the "--". It just works and I couldn't see why it should fail since the code used to and still does just delete the <div class="moz-signature">. However the test fails since the signature is placed with "--" in the first place. I don't understand why.
(Assignee)

Comment 22

a year ago
Grrr. This test composes the e-mail *before* setting the preference:
https://dxr.mozilla.org/comm-central/source/mail/test/mozmill/composition/test-signature-updating.js#66
Boy, that took me long to spot.
(Assignee)

Comment 23

a year ago
Created attachment 8718128 [details] [diff] [review]
Adapted test (v2).

OK, enabled the other plaintext test and reset the preferences as requested. Please use interdiff to see the difference to the patch (v1).

That's all I'd like to do here.
Attachment #8716668 - Attachment is obsolete: true
Attachment #8716668 - Flags: review?(acelists)
Attachment #8718128 - Flags: review?(acelists)
(Assignee)

Comment 24

a year ago
Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ef084dfdd093

Comment 25

a year ago
(In reply to Jorg K (GMT+1) from comment #23)
> That's all I'd like to do here.

Can you add some new tests in a new bug? It looks like placing the signature is a fragile feature that couldn't be done properly till now. And the placing still looks fragile, iterating over HTML elements and searching for the right spot. If we inject some unexpected element in the future, this will break again.
(Assignee)

Comment 26

a year ago
(In reply to :aceman from comment #25)
> Can you add some new tests in a new bug?
I agree. There should be some tests at least for the reply case, so that the new nsMsgCompose::MoveToAboveQuote(void) gets exercised. Maybe someone else has more experience with Mozmill tests while I rip out recycling ;-)

Comment 27

a year ago
OK, please file the bug and describe some ideas for what to test and what the proper outcome should be. Maybe someone picks it up :) I could try but I am not familiar with how to test what is in the composer editor.
(Assignee)

Comment 28

a year ago
Bug 1248045. We can do it together. You're an expert in Mozmill testing. All we need is a basic test that does a reply. You could even load a message from a file and then reply to it. In the reply, we switch signature like in test-signature-updating.js. I can do the editor fiddle to check the result.

Comment 29

a year ago
Comment on attachment 8718128 [details] [diff] [review]
Adapted test (v2).

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

OK, but I get a failure in testPlaintextComposeWindowSwitchSignatures(). I often get random failures locally so if you can make a successful try run with this patch, we can move forward.
Attachment #8718128 - Flags: review?(acelists) → review+
(Assignee)

Comment 30

a year ago
On which hardware would you like the try run? The Linux try run is in comment #24:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ef084dfdd093
Is that good enough? Since we're currently busted, I believe, I can't do another one.

Is your failure random or permanent?

Comment 31

a year ago
(In reply to Jorg K (GMT+1) from comment #30)
> On which hardware would you like the try run? The Linux try run is in
> comment #24:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=ef084dfdd093
> Is that good enough? Since we're currently busted, I believe, I can't do
> another one.

I wouldn't trust that try run - the build errored out.

Comment 32

a year ago
(In reply to aleth [:aleth] from comment #31)
> Since we're currently busted, I believe, I can't do
> > another one.

You can push to try if you push your patches together with a m-c backout of the offending bug.
(Assignee)

Comment 33

a year ago
testPlaintextComposeWindowSwitchSignatures() has not changed (and the code changes shouldn't have affected it, I think). So it's weird that it should fail.

These ones are new:
testPlaintextComposeWindowSwitchSignaturesWithSuppressedSeparator()
testHTMLComposeWindowSwitchSignaturesParagraphFormat()
testHTMLComposeWindowSwitchSignaturesWithSuppressedSeparatorParagraphFormat().

As for the try run, I could include Ratty's bustage fix. I'll do it tomorrow.
(Assignee)

Comment 34

a year ago
OK, here:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=438b51457a00
(Assignee)

Comment 35

a year ago
The try came out green, at least for this test, so I checked it in:
https://hg.mozilla.org/comm-central/rev/191ed0ff0f9d
https://hg.mozilla.org/comm-central/rev/e5153ceb5d4f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-thunderbird47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
(Assignee)

Comment 36

a year ago
Comment on attachment 8716629 [details] [diff] [review]
Proposed solution (v3a).

[Approval Request Comment]
Regression caused by (bug #): Not a regression, however bug 330891 exposed the erroneous behaviour.
User impact if declined: Puzzling behaviour when identity/signature is switched.
Testing completed (on c-c, etc.): Manually and with Mozmill test-signature-updating.js
Risk to taking this patch (and alternatives if risky):
Not really risky since only a seldom used function is affected.
Attachment #8716629 - Flags: approval-comm-beta?
Attachment #8716629 - Flags: approval-comm-aurora+
(Assignee)

Comment 37

a year ago
Comment on attachment 8718128 [details] [diff] [review]
Adapted test (v2).

[Approval Request Comment]
Regression caused by (bug #): Not a regression, however bug 330891 exposed the erroneous behaviour.
User impact if declined: Puzzling behaviour when identity/signature is switched.
Testing completed (on c-c, etc.): Manually and with Mozmill test-signature-updating.js
Risk to taking this patch (and alternatives if risky):
Not really risky since only a seldom used function is affected.
Attachment #8718128 - Flags: approval-comm-beta?
Attachment #8718128 - Flags: approval-comm-aurora+
(Assignee)

Comment 38

a year ago
Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/f44ae06d8613
https://hg.mozilla.org/releases/comm-aurora/rev/fa693e739abc
status-thunderbird46: affected → fixed
(Assignee)

Comment 39

a year ago
I discovered a fatal error which will cause an endless loop and make the program hang.

Backout:
https://hg.mozilla.org/comm-central/rev/f006326842f7
https://hg.mozilla.org/comm-central/rev/4ca276dff98a

https://hg.mozilla.org/releases/comm-aurora/rev/94bbcdd6ecd1
https://hg.mozilla.org/releases/comm-aurora/rev/2b722528d5f6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 40

a year ago
Created attachment 8719320 [details] [diff] [review]
Proposed solution (v4).

Sadly the node->GetNextSibling() was inside the if (element) {}. So if the nsIDOMNode was not a nsIDOMElement, we'd have an endless loop.
Attachment #8716629 - Attachment is obsolete: true
Attachment #8716629 - Flags: approval-comm-beta?
Attachment #8719320 - Flags: review?(acelists)

Comment 41

a year ago
Comment on attachment 8719320 [details] [diff] [review]
Proposed solution (v4).

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

I can't test it right now, but it seems conceptually correct. Thanks for catching it.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +5394,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIDOMNode> node;
> +  nsAutoString tagLocalName;

Can tagLocalName be inside the loop?
Attachment #8719320 - Flags: review?(acelists) → review+
(Assignee)

Comment 42

a year ago
Created attachment 8719328 [details] [diff] [review]
Proposed solution (v4a).

Combined patch: Code change + test change - easier to back out next time ;-)

I decided to leave the original test for "element" in place. As discussed on IRC, text nodes are not elements and don't return a local name. So they can be weeded out straight away with the element test.

So the change only becomes moving the GetNextSibling() to where it should go.
Attachment #8718128 - Attachment is obsolete: true
Attachment #8719320 - Attachment is obsolete: true
Attachment #8718128 - Flags: approval-comm-beta?
Attachment #8719328 - Flags: review+
(Assignee)

Comment 43

a year ago
Comment on attachment 8719328 [details] [diff] [review]
Proposed solution (v4a).

[Approval Request Comment]
Regression caused by (bug #): Not a regression, however bug 330891 exposed the erroneous behaviour.
User impact if declined: Puzzling behaviour when identity/signature is switched.
Testing completed (on c-c, etc.): Manually and with Mozmill test-signature-updating.js
Risk to taking this patch (and alternatives if risky):
Not really risky since only a seldom used function is affected.
(Right, and the first version that got landed caused an endless loop. So there's risk everywhere.)

Landed this again:
https://hg.mozilla.org/comm-central/rev/bd1684a179ca

Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/c486776f8a8b
Attachment #8719328 - Flags: approval-comm-beta?
Attachment #8719328 - Flags: approval-comm-aurora+
(Assignee)

Updated

a year ago
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Comment on attachment 8719328 [details] [diff] [review]
Proposed solution (v4a).

http://hg.mozilla.org/releases/comm-beta/rev/a3b38093e531
Attachment #8719328 - Flags: approval-comm-beta? → approval-comm-beta+

Updated

a year ago
status-thunderbird45: affected → fixed
tracking-thunderbird45: ? → ---
You need to log in before you can comment on or make changes to this bug.