Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Thunderbird 47.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
5 years ago
11 months ago

People

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

Tracking

Trunk
Thunderbird 47.0
x86
All
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird45 fixed, thunderbird46 fixed, thunderbird47 fixed)

Details

Attachments

(5 attachments, 9 obsolete attachments)

25.06 KB, image/png
Details
25.08 KB, image/png
Details
21.41 KB, image/png
Details
21.84 KB, image/png
Details
13.89 KB, patch
Magnus Melin
: review+
Jorg K (GMT+2)
: approval-comm-aurora+
Jorg K (GMT+2)
: approval-comm-esr45+
Details | Diff | Splinter Review
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.
Blocks: 698925
Component: Message Compose Window → FileLink
QA Contact: message-compose → filelink
(Assignee)

Comment 1

2 years ago
Maybe ;-)
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
(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

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

Comment 5

2 years ago
(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?
(Assignee)

Comment 6

2 years ago
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.)
(Assignee)

Comment 7

2 years ago
Created attachment 8719427 [details] [diff] [review]
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.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8719427 - Flags: feedback?(acelists)
(Assignee)

Comment 8

2 years ago
Created attachment 8719526 [details] [diff] [review]
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.
Attachment #8719427 - Attachment is obsolete: true
Attachment #8719427 - Flags: feedback?(acelists)
Attachment #8719526 - Flags: feedback?(acelists)
(Assignee)

Comment 9

2 years ago
Created attachment 8719566 [details] [diff] [review]
Proposed solution (v1a).

Make the logic look a little nicer. Only missing now are the error checks.
Attachment #8719526 - Attachment is obsolete: true
Attachment #8719526 - Flags: feedback?(acelists)
Attachment #8719566 - Flags: feedback?(acelists)
(Assignee)

Comment 10

2 years ago
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
(Assignee)

Comment 11

2 years ago
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
(Assignee)

Comment 12

2 years ago
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
(Assignee)

Comment 13

a year ago
Created attachment 8719745 [details] [diff] [review]
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
Attachment #8719566 - Attachment is obsolete: true
Attachment #8719566 - Flags: feedback?(acelists)
Attachment #8719745 - Flags: feedback?(acelists)
(Assignee)

Comment 14

a year ago
This try is green.
(Assignee)

Comment 15

a year ago
Created attachment 8719787 [details] [diff] [review]
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.
Attachment #8719745 - Attachment is obsolete: true
Attachment #8719745 - Flags: feedback?(acelists)
Attachment #8719787 - Flags: review?(acelists)
(Assignee)

Comment 16

a year ago
Created attachment 8720191 [details]
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.
(Assignee)

Comment 17

a year ago
Created attachment 8720209 [details]
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.
(Assignee)

Comment 18

a year ago
Created attachment 8720217 [details] [diff] [review]
Proposed solution (v2) with tests fixed.
Attachment #8719787 - Attachment is obsolete: true
Attachment #8719787 - Flags: review?(acelists)
Attachment #8720217 - Flags: review?(acelists)
(Assignee)

Comment 19

a year ago
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

a year ago
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

a year ago
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).
Attachment #8720217 - Flags: review?(acelists)
(Assignee)

Comment 22

a year ago
(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.
(Assignee)

Updated

a year ago
Attachment #8720209 - Attachment description: Comparing DOM of plain text forward → Comparing DOM of plain text forward (signature above)
(Assignee)

Comment 23

a year ago
Created attachment 8720329 [details]
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.
(Assignee)

Updated

a year ago
Attachment #8720191 - Attachment description: Comparing DOM of HTML-forward → Comparing DOM of HTML-forward (signature above)
(Assignee)

Comment 24

a year ago
Created attachment 8720331 [details]
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.
(Assignee)

Comment 25

a year ago
Created attachment 8720346 [details] [diff] [review]
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).
Attachment #8720217 - Attachment is obsolete: true
Attachment #8720346 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 26

a year ago
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

a year ago
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
Attachment #8720346 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 28

a year ago
Created attachment 8721390 [details]
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.
Attachment #8720191 - Attachment is obsolete: true
(Assignee)

Comment 29

a year ago
Created attachment 8721406 [details]
Comparing DOM of HTML-forward (signature below)

OK, with the revised patch (coming up), we get this.
Attachment #8720331 - Attachment is obsolete: true
(Assignee)

Comment 30

a year ago
Created attachment 8721435 [details] [diff] [review]
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.
Attachment #8720346 - Attachment is obsolete: true
Attachment #8721435 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 31

a year ago
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

Updated

a year ago
Attachment #8721435 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 32

a year ago
https://hg.mozilla.org/comm-central/rev/51fb7eddac3e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-thunderbird45: --- → affected
status-thunderbird46: --- → affected
status-thunderbird47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
(Assignee)

Comment 33

a year ago
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.
Attachment #8721435 - Flags: approval-comm-beta?
Attachment #8721435 - Flags: approval-comm-aurora+
(Assignee)

Updated

a year ago
Component: FileLink → Message Compose Window
(Assignee)

Comment 34

a year ago
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.
(Assignee)

Comment 35

a year ago
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.
status-thunderbird46: affected → fixed
(Assignee)

Updated

a year ago
Depends on: 1249884
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
Attachment #8721435 - Flags: approval-comm-beta? → approval-comm-beta+

Updated

a year ago
status-thunderbird45: affected → fixed
(Assignee)

Comment 37

11 months ago
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.
Attachment #8721435 - Flags: approval-comm-beta+ → approval-comm-esr45+
You need to log in before you can comment on or make changes to this bug.