736584 - Move inline-forward DIV wrapping code out of cloudAttachmentLinkManager.js and into nsMsgCompose.cpp

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

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

During the hubbub of landing Filelink a.k.a BigFiles, some code landed in mail/components/compose/cloudAttachmentLinkManager.js that registers a state listener.  The state listener listens for us doing a forward-inline of mail, and then wraps the contents of the inline-forward in a DIV so that attachment URLs can be put in the right place.

We should probably move that DIV-wrapping code into nsMsgCompose.cpp.

Comment 1

[Jorg K (CEST = GMT+2)]

Maybe ;-)

Comment 2

[Jorg K (CEST = GMT+2)]

Here a more qualified comment:
nsMsgCompose.cpp is very complicated code that deals with all the options of composing in nsMsgCompose::ConvertAndLoadComposeWindow:
1) Reply above/below quote with signature above/below quote and 2) forward with or without signature also above/below the forwarded inline message. I tried to make it more complicated by adding paragraphs instead of newlines in bug 330891, here is a WIP patch (attachment 8696532 [details] [diff] [review]). In the end I left nsMsgCompose.cpp unchanged and added the paragraphs into cloudAttachmentLinkManager.js (NotifyComposeBodyReady).

In principle it is a good idea to manipulate the message in JS code rather then making nsMsgCompose.cpp more complicated. However, there are two problems with the current implementation:

First:
Obviously the listener "NotifyComposeBodyReady" lives in the wrong source file. General post-processing of a message has nothing to do with cloud attachments.

Second:
The inline forward processing is just plain wrong. It grabs the entire composed message and stuffs it into a <div class="moz-forward-container">, including any added signature, which is of course not forwarded. Also, reshuffling the entire e-mail body seems like an expensive and unnecessary operation.

So a clean-up is necessary here. I think the solution should have two parts:
- Move the wrapping into nsMsgCompose.cpp
- Move the listener into a more suitable source file.

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #2)
> Second:
> The inline forward processing is just plain wrong. It grabs the entire
> composed message and stuffs it into a <div class="moz-forward-container">,
> including any added signature, which is of course not forwarded.
As a consequence an identify/signature switch doesn't find the signature in the <div>. Therefore the signature is not switched, but instead a new one is added.

Comment 4

[Magnus Melin [:mkmelin]]

Is it possible the div wrapping is not really needed? 
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/cloudAttachmentLinkManager.js#144

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #4)
> Is it possible the div wrapping is not really needed? 
You're asking the wrong person. I came to the party when the goose was already cooking ;-)
BTW, I added and you reviewed and approved the comment "This is kind of mad ...".
I think it makes sense to prepare a message where you can see the forwarded part. There are many add-ons around that will pull things apart, and if you remove this wrapper, they will get upset.

It's HTML and you can structure it, so why remove the structure?

Comment 6

[Jorg K (CEST = GMT+2)]

The identity/signature switch now relies on the fact that the inlined content can be recognised. So the wrapping in <div class="moz-forward-container"> is needed. Refer to attachment 8716629 [details] [diff] [review] for details, look for nsMsgCompose::MoveToAboveQuote(void). As mentioned in comment #3, the wrapping needs to be fixed, so that the identity/signature switch can recognise the signature. (This bug is next on to-do my list.)

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: WIP (v1): This fixes the problem for HTML composition

Here is a simple fix for the HTML composition. Instead of munging the body after it's been completed, we insert the tag we want into the HTML while it's in text form. This works nicely, however, this approach don't fly for plaintext composition.

Aceman, if you have time, you can cast an eye on it.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: Proposed final solution (v1)

OK. This works for HTML and plaintext forward.

For HTML, we insert the text "<div class="moz-forward-container">" into the HTML that will be inserted.

For plaintext, we insert a DOM element <div class="moz-forward-container"> and make sure the process places the forwarded inline message into this element.

That's 1000 times nicer than munging the result in JS.

The big advantage is that the signature is now no longer placed into the "forward container", so the identity/signature switch actually works.

Here a tip for testing:
I use two add-ons: "DOM Inspector" and "Element Inspector".
So in the composition window, I just <Shift><Right-Click> into the composition, and the DOM Inspector pops up and shows me the DOM of the composition.
#document
  HTML
    HEAD
    BODY.
You open the body and can see the DOM. Forgive me if you already know all that.

Ah yes, the patch could do with some error checking, but other then that, it's what I want to do here. Maybe we can land it this week, so this could still go into TB 45 so complete the identity/signature switch cleanup. Tests should be added in bug 1248045.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a).

Make the logic look a little nicer. Only missing now are the error checks.

Comment 10

[Jorg K (CEST = GMT+2)]

See whether this breaks anything. The cloud storage code introduced the div wrapping, so perhaps it takes offence that the wrapping changed a little. Let's see:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3b9a1b69577f

Comment 11

[Jorg K (CEST = GMT+2)]

As expected, the cloud storage tests get confused since the two <br> elements at the start are now no longer in the "forward container". So the tests will need adjustment. Test failures:

INFO -  SUMMARY-UNEXPECTED-FAIL | test-cloudfile-attachment-urls.js | test-cloudfile-attachment-urls.js::test_adding_filelinks_to_empty_forward
INFO -    EXCEPTION: a != b: '[object HTMLBRElement]' != '[object HTMLBRElement]'.
INFO -      at: test-folder-display-helpers.js line 2849
INFO -         assert_true test-folder-display-helpers.js:2849 11
INFO -         assert_equals test-folder-display-helpers.js:2836 3
[stuff deleted]
INFO -  SUMMARY-PASS | test-cloudfile-attachment-urls.js::setupTest
INFO -  SUMMARY-UNEXPECTED-FAIL | test-cloudfile-attachment-urls.js | test-cloudfile-attachment-urls.js::test_adding_filelinks_to_forward
INFO -    EXCEPTION: a != b: 'br' != 'div'.
INFO -      at: test-folder-display-helpers.js line 2849
INFO -         assert_true test-folder-display-helpers.js:2849 11
INFO -         assert_equals test-folder-display-helpers.js:2836 3

Comment 12

[Jorg K (CEST = GMT+2)]

Oops, didn't paste enough from the log:

test_adding_filelinks_to_empty_forward fails like this:
EXCEPTION: a != b: '[object HTMLBRElement]' != '[object HTMLBRElement]'.
INFO -      at: test-folder-display-helpers.js line 2849
INFO -         assert_true test-folder-display-helpers.js:2849 11
INFO -         assert_equals test-folder-display-helpers.js:2836 3
INFO -         subtest_adding_filelinks_to_forward test-cloudfile-attachment-urls.js:663 5
INFO -         try_with_and_without_signature_in_reply_or_fwd test-cloudfile-attachment-urls.js:199 3
INFO -         test_adding_filelinks_to_empty_forward test-cloudfile-attachment-urls.js:618 3

test_adding_filelinks_to_forward fails like this:
INFO -    EXCEPTION: a != b: 'br' != 'div'.
INFO -      at: test-folder-display-helpers.js line 2849
INFO -         assert_true test-folder-display-helpers.js:2849 11
INFO -         assert_equals test-folder-display-helpers.js:2836 3
INFO -         subtest_adding_filelinks_to_forward test-cloudfile-attachment-urls.js:650 3
INFO -         try_with_and_without_signature_in_reply_or_fwd test-cloudfile-attachment-urls.js:199 3
INFO -         test_adding_filelinks_to_forward test-cloudfile-attachment-urls.js:631 3

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a) with texts fixed.

I fixed the horrible test. It's horrible since it assumes so much about the content of the editor window, so if you make changes in the composition pipeline, this test will break first. It already broke when introducing paragraph format in bug 330891.

New try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=acf43d2136d1

Comment 14

[Jorg K (CEST = GMT+2)]

This try is green.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1b) with texts fixed.

OK, I added error checking, so this should be good for review. Please note the tips in comment #8. Of course the prints will be removed in the final version.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: Comparing DOM of HTML-forward (signature above)

I visually compared a HTML-forwarded message using TB 45/46 and my version. They look the same. Here is a comparison of the DOM, left is old, right is new.

We see that the signature has moved out of the <div class="moz-forward-container"> and so have the two <br> elements. So this looks very good.

Sadly, the plain text forward has an additional <br> in the new version, so I need to revise that again.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: Comparing DOM of plain text forward (signature above)

OK, with the patch I am about to submit, this looks much better. Note that also some extra unneeded <br> elements at the end have also disappeared.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2) with tests fixed.

Comment 19

[Jorg K (CEST = GMT+2)]

Grrr, when forwarding with no signature or signature at the bottom, in the new version I still get four blank lines before the -------- Forwarded Message -------- instead of three in the old version. So another adjustment to the patch is necessary.

Comment 20

[:aceman]

Comment on attachment 8720217 [details] [diff] [review]
Proposed solution (v2) with tests fixed.

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

::: mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
@@ +663,5 @@
>      let br = assert_previous_nodes("br", root, 2);
>      let textNode = assert_previous_text(br.previousSibling, aText);
>    } else {
> +    // Otherwise, there are only <br> elements and the number depends on
> +    // whether it's HTLM or plain text. The top most <br> should be the

HTML.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +724,5 @@
> +                         .EqualsLiteral("<HTML><BODY><BR><BR>")) {
> +        // We assign the opening tag after "<HTML><BODY><BR><BR>".
> +        // This is a bit hacky but we know that the MIME code prepares the
> +        // forwarded content like this:
> +        // <HTML><BODY><BR><BR> + forwarded header + header table.

Can we somehow check this at compile time? Is that string hardcoded in MIME module, or can it only be determined from the resulting HTML source? At least we could have a test (maybe xpcshell) that tests whether this is true. The test would immediatelly flag if this changes. Without the test, you may never know this code gets never executed after a change in MIME.

@@ +748,5 @@
>        {
>          if (isForwarded && sigOnTop)
>            m_editor->BeginningOfDocument();
>          else
> +          MoveToEndOfDocument();

Why does only this one occurrence need changing to your new function?

@@ +800,5 @@
> +
> +        // Position into the div, so out content goes there.
> +        nsCOMPtr<nsISelection> selection;
> +        m_editor->GetSelection(getter_AddRefs(selection));
> +        rv = selection->Collapse(divElem, 0);

Can selection be null or GetSelection() fail?

Comment 21

[:aceman]

Comment on attachment 8720217 [details] [diff] [review]
Proposed solution (v2) with tests fixed.

I also feel I do not understand enough of the code here so I would suggest a different reviewer. Maybe squib understands something of this. Or as this depends on precise elements produced in the message, maybe jcranmer could work for the MIME dependencies (also it is in /mailnews).

Comment 22

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #20)
> Can we somehow check this at compile time? Is that string hardcoded in MIME
> module, or can it only be determined from the resulting HTML source? At
> least we could have a test (maybe xpcshell) that tests whether this is true.
> The test would immediatelly flag if this changes. Without the test, you may
> never know this code gets never executed after a change in MIME.
Yes, this is a bit hacky since it depends on MIME. However, the said test test-cloudfile-attachment-urls.js will fail immediately if the <div class="moz-forward-container"> is not inserted.

> Why does only this one occurrence need changing to your new function?
Since the old function will position into the <div class="moz-forward-container"> so the signature ends up in there again. Remember, the M-C core version will position into containers.

> Can selection be null or GetSelection() fail?
Not if you're doing something simple as positioning at the start of an element with offset 0.

Looks like I need to look for another reviewer and with the delay this will entail, it will be too late for TB 45.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: Comparing DOM of plain text forward (signature below)

With the patch which I will attach in a moment, there are the same amount of <br> inserted (see comment #19), but the signature is no longer wrapped in the forward container. So it's all good.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: Comparing DOM of HTML-forward (signature below)

Just for completeness, here the comparison of the DOM for HTML-forward with signature below. All four cases work perfectly now. Patch coming.

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v3) with tests fixed.

Hi Magnus, my reviewer has left me ;-( so I hope you can review this so it can still make TB 45.

Let me give you some background: When cloud attachments were implemented, the forwarded message that came out of nsMsgCompose.cpp was stuffed into a <div class="moz-forward-container"> in JS. Sadly, the signature ended up in this div as well, so the identity/signature switch didn't find it.

In bug 1231902 I much improved the signature switch so it works well in (almost) all cases, including when paragraph format/mode is used, which is now the default.

The missing link is this bug here, which moves the div wrapping out of JS in cloudAttachmentLinkManager.js and into nsMsgCompose.cpp where it really belongs.

Getting the div put into the editor is a little messy, but I have worked around all the quirks, so the forwarded messages look exactly the same as they looked before, only the internal structure is different, that is, the signature is no longer in the forwarded container. I've attached screenshots of the DOM in all four cases (HTML/plain text x signature above/below).

You asked in comment #4 whether the div wrapping is needed: It is needed, for once since the cloud attachment stuff relies on in, secondly the signature switch relies on it, since when you want to replace a signature with "on top" placement, this is no longer placed right at the top of the message, but instead just above the "forward container" (or above a block quote or cite prefix for replies).

Comment 26

[Jorg K (CEST = GMT+2)]

Another comment: You may ask why I construct
<HTML><BODY><BR><BR><div class="moz-forward-container"> and not
<HTML><BODY><div class="moz-forward-container"><BR><BR>
before calling RebuildDocumentFromSource() so the two <br> elements would go into the <div>.
Simple answer: It ain't working. If I do this, the <div> is not inserted. Thanks M-C editor!!!

(I've tried it all, this is the solution that works best.)

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 8720346 [details] [diff] [review]
Proposed solution (v3) with tests fixed.

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

Yep, seems to work. The signature area is very error prone so lets hope there's nothing we've missed. r=mkmelin (with the printfs removed of course

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: Comparing DOM of HTML-forward (signature above)

With a better trick I can reproduce the DOM which is currently produced, that is, the two <br> elements inside the forward container, of course the signature outside. The advantage is that the behaviour doesn't change, the test doesn't have to change and it's better for signature switching, too.

Updated patch coming.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: Comparing DOM of HTML-forward (signature below)

OK, with the revised patch (coming up), we get this.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v4). Tests not affected and not changed.

OK. I've looked at it again. The HTML case could be improved. Therefore no tests need to change. Also refer to the new screenshots of the DOM.

This is much better for signature switching.

Comment 31

[Jorg K (CEST = GMT+2)]

Magnus, I'm so sorry about the double review. I found a better way to do it, that is insert the <div> tag before the <br><br>.

You can see it in the interdiff:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8720346&action=interdiff&newid=8721435&headers=1

Comment 32

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/51fb7eddac3e

Comment 33

[Jorg K (CEST = GMT+2)]

Comment on attachment 8721435 [details] [diff] [review]
Proposed solution (v4). Tests not affected and not changed.

[Approval Request Comment]
Regression caused by (bug #): bug 698925, bug 736055.
These bugs implemented wrapping the forwarded content into a <div> in JS.
User impact if declined: Identity/signature switch doesn't work on forwards.
Testing completed (on c-c, etc.): Manual and in test suite.
Risk to taking this patch (and alternatives if risky):
There is risk, since this is digging around with the way messages are forwarded.

However, this bug fits nicely with the improvement made to signature switching in bug 1231902 which were necessary after we detected that signature switch "traditionally" didn't work in paragraph format after making this the default in bug 330891.

So this bug, bug 330891 and bug 1231902 belong together and conclude the rework of this area.

Comment 34

[Jorg K (CEST = GMT+2)]

Oops, an unused variable slipped the review, I took the liberty to fix that:
https://hg.mozilla.org/comm-central/rev/51fb7eddac3e (landed in comment #32)
https://hg.mozilla.org/comm-central/rev/819d62863eb1 - cosmetic fix.
Uplift should uplift both changesets.

Comment 35

[Jorg K (CEST = GMT+2)]

Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/1375c30be23f
Note: I merged the two changesets, so the beta uplifter can use the Aurora changeset.

Comment 36

[Kent James (:rkent)]

Comment on attachment 8721435 [details] [diff] [review]
Proposed solution (v4). Tests not affected and not changed.

http://hg.mozilla.org/releases/comm-esr45/rev/9f7dc44599b6

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8721435 [details] [diff] [review]
Proposed solution (v4). Tests not affected and not changed.

Changed approval from beta to esr45 since it was never landed on beta.