Open Bug 1360155 Opened 3 years ago Updated 1 month ago

After adding an event attachment to Lightning, the message is hidden

Categories

(Calendar :: E-mail based Scheduling (iTIP/iMIP), defect)

Lightning 4.7.8
defect
Not set

Tracking

(Not tracked)

People

(Reporter: hife, Unassigned)

References

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170418123315

Steps to reproduce:

1. Receive an email with an attached event.
2. Click button to add said event to Lightning calendar.
3. Try to view email contents.


Actual results:

Only the event is shown in place of the email. The rest of the message is hidden.

If you delete the event from the calendar, the message returns to normal.


Expected results:

The rest of the message body should still be visible. Important information is usually contained in it.
I have various messages from airlines where the events are simply attached and you can see the HTML body of the message.

However, just yesterday, I saw and airline e-mail on the machine of my wife and the HTML body was hidden by the event. Now I'm viewing the very same message with TB 45 and TB 55 (Daily) and can't reproduce the problem.

Does the same happen with add-ons disabled, apart from Lightning of course. Could you attach such a message here (drag to desktop, use "Attach File" above).

Note that some invitation e-mails do indeed not contain any HTML part, they only have a text/calendar part which is interpreted by Lightning.

Philipp, can you shed some light on this.
Flags: needinfo?(philipp)
Attached file Example mail single event with UID (obsolete) —
(In reply to Jorg K (GMT+2) from comment #1)
I have disabled all addons but Lightning and the behaviour persists.

After some testing I found that for the bug to occur, the event in question needs to have a UID, so Thunderbird will be aware the event has been processed. I have attached an example email that triggers the bug for me.

There also seems to be a distinction between an event and invitation to an event as well as some differences when there are multiple events contained in a single .ics file. I am not familiar with the file format and structure, so I have not run any further tests with these variations.
Attachment #8862387 - Attachment mime type: application/x-extension-eml → text/plain
Attached image Screenshot of the event
The message you attached has two parts:

Content-Type: text/plain; charset=utf-8; format=flowed
Content-Type: text/calendar;

Lightning grabs the second part and displays it as an event invitation. The word "Test" from the plain text part is still displayed.
As described in the original report, the text only vanishes when the event is added to a calendar (and reappears when the event is deleted).
Oh, you're right, I added the event and the text disappeared. That shouldn't be so.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Are you really reporting this for TB 45.8 and Lightning 4.7.8? In those versions I don't even get prompted to add event to the calendar.
Component: Untriaged → General
Product: Thunderbird → Calendar
Version: 45 Branch → Lightning 4.7.8
Sorry, your test message has X-Account-Key: account5 and that upset my TB 45 installation where such an account does not exist. Editing this to account1 reproduces the problem in TB 45.8.
I removed the X-Account-Key altogether.
Attachment #8862387 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #9)
> Are you really reporting this for TB 45.8 and Lightning 4.7.8?

Exactly.
Attachment #8862506 - Attachment mime type: application/x-extension-eml → text/plain
Turned out that the airline e-mail contained Content-Type: application/octet-stream; for the base64-encoded ICS files. After changing this to text/calendar Lightning recognised the events. So it wasn't the encoding but the content type.

I'll obsolete comment #13 to comment #15. So now I get the behaviour originally reported:

1) HTML of the message displayed (before accepting the event)
2) ICS files *are* displayed inline.
3) Notification what the message contains events that can be accepted.
4) After accepting the event: No HTML displayed any more, only the event.
Flags: needinfo?(ssitter)
Flags: needinfo?(bv1578)
Flags: needinfo?(philipp)
See Also: → 1389014
Any plans to address this? It's a bit funny that the message disappears once you process the event.

That's not a problem in mailnews/MIME, is it? You must be doing something in Calendar to cause this.
Flags: needinfo?(philipp)
Flags: needinfo?(makemyday)
This is probably a duplicate of bug 388584. Without having looked at the code right now, as rendering of text and invitation preview already works when you receive an email with an email attached which itself has an ics part, it probably wouldn't be too hard to fix.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(philipp)
Flags: needinfo?(makemyday)
Resolution: --- → DUPLICATE
Duplicate of bug: 388584
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: DUPLICATE → ---
Status: UNCONFIRMED → NEW
Ever confirmed: true
OK, here you have two sample e-mails that show the problem and the patch that fixes it.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8901592 - Flags: review?(makemyday)
Component: General → E-mail based Scheduling (iTIP/iMIP)
Comment on attachment 8901592 [details] [diff] [review]
1360155-message-gone-after-accept.patch (v1)

Thanks, I was able to reproduce the behaviour. Unfortunately, this patch breaks the "display changes" feature, so r- for now to find a better approach. What comes to mind as a bandaid is to move the line to the end of the above if block - this wouldn't resolve the issue entirely, but would provide the user an option to disable the behaviour by flipping the pref at least, so users can decide what's more important for them.

For a complete fix we would need another way to overwrite the visual ics representation (and only this instead of overwritting everything) at this point in time of the processing, because the required information isn't yet available earlier when rendering the initial representation from the data contained in the email.

If you don't want to investigate in the latter now, I would accept the bandaid patch mentioned above with a follow-up bug on file.
Attachment #8901592 - Flags: review?(makemyday) → review-
Thanks for the feedback. Can you please explain, what is the "display changes" feature? I wasn't aware of it. How do you trigger it so I can experiment. Might it be that you have accepted an event from an e-mail, you edit the event in the calendar and expect the e-mail to show the modification?
Flags: needinfo?(makemyday)
Hmm, looks like this went broken in bug 533265.

So here ...
> For a complete fix we would need another way to overwrite the visual ics
> representation (and only this instead of overwritting everything) at this
> point in time of the processing, because the required information isn't yet
> available earlier when rendering the initial representation from the data
> contained in the email.
... you're saying that the message gets rendered with the original (wrong) data first since the (correct) new data isn't available yet, and at a later stage you want to show that data. So why does blanking out the message first help with that? Where is it finally rendered with the correct information?
Blocks: 533265
Keywords: regression
(In reply to Jorg K (GMT+2) from comment #22)
> Thanks for the feedback. Can you please explain, what is the "display
> changes" feature? I wasn't aware of it. How do you trigger it so I can
> experiment. Might it be that you have accepted an event from an e-mail, you
> edit the event in the calendar and expect the e-mail to show the
> modification?
I figured it out by reading bug 533265. You send an invitation, then change the event, send an updated invitation, the details get update in the original e-mail. Nice.

However, why was clearing the display with |msgWindow.displayHTMLInMessagePane("", msgOverlay, false);| ever a good idea? As I also asked, where would the update information be displayed? Anyway, it might be more beneficial to use displayURIInMessagePane() and give it the URI of the message.
(In reply to Jorg K (GMT+2) from comment #24)
> Anyway, it might be more beneficial to use displayURIInMessagePane() and
> give it the URI of the message.
I've looked at it, and the URL is not easy to obtain. I managed to hack it with:
            if (gMessageDisplay && gMessageDisplay.displayedMessage) {
                let url = gMessageDisplay.displayedMessage.folder.folderURL +
                    "?number=" + gMessageDisplay.displayedMessage.messageKey;
                msgWindow.displayURIInMessagePane(url, false,
                    Services.scriptSecurityManager.getSystemPrincipal());
but that only constructs the correct URL for mailbox: since imap: doesn't have "?number=" but some other scheme. Anyway, with that hack I get the raw message source of the message displayed as a long text string. Next I tried:
                let url = gMessageDisplay.displayedMessage.folder.folderURL +
                    "?number=" + gMessageDisplay.displayedMessage.messageKey + "&part=1.1.1";
to address the body of the message. That's of course a hack since the part number changes depending on the MIME structure. With that, I get the message body displayed, but no event information. So that suggestion doesn't fly, even if the hack were replaced by proper processing.

So we need to come back to the original question about the original design here, sorry for the repetition:
Why is clearing the display at that stage with |msgWindow.displayHTMLInMessagePane("", msgOverlay, false);| a good idea or necessary and who later displays the event HTML? Conceptually it would be more sense to trigger a redisplay of the message and make sure that 

BTW:
> What comes to mind as a band-aid is to move the line to the end of the above if block
> - this wouldn't resolve the issue entirely, but would provide the user an option
> to disable the behaviour by flipping the pref at least, so users can decide what's
> more important for them.
Since calendar.itip.displayInvitationChanges has default==true, no one will go looking to switch this off to unhide processed events. So that doesn't really help the average user.

So I'm in favour of a complete fix, as you said:
> For a complete fix we would need another way to overwrite the visual ics
> representation (and only this instead of overwritting everything) at this
> point in time of the processing, because the required information isn't
> yet available earlier when rendering the initial representation from the
> data contained in the email.
That's interesting reading: So there is an initial representation which uses the original old (wrong) data and then later you have a second go when you looked up the event in the calendar and found it changed. Then you replace the entire message with the event HTML instead of retriggering a display and accessing the corrected calendar data the second time around. One would also have to look at why the correct information isn't available when the message is first displayed since that would save all the hoops of displaying it with different information a second time.
Duplicate of this bug: 955663
This bug seems to be unintentionally "fixed" following your proposed approach by a recent regression related to the data-url mc changes, which effectively prevents msgWindow.displayHTMLInMessagePane() to be invoked (navigation to toplevel data: URI not allowed (Blocked loading of...) ;-)

But to explain how the feature was (and should be) working: The ics part within the email is rendered to the event summary using the simpleMimeConverter in lightningTextcalendarConverter.js at load time of the message. To display the diff, we need to look up for the event in the respective calendar, which can take some time if calendars are big or on the network. So anything depending on looking up the calendar is done asynchronously within imip.bar.js (like composing the blue bar, determine which imip button to show or overlaying the invitation summary with the upate information). Looking up for the event already in the converter will result in a delay when displaying the email compared to messages not containing an invitation.

Given displayHTMLInMessagePane() can be made to work again for that purpose, the (imho only) way to fix this and keep showing the diff and using the current way to display the invitation summary in the message body is to read out the dom object of the rendered email, clone that, replace the invitation summary part with the updated one and rewrite the displayed email by using displayHTMLInMessagePane().

I haven't looked yet whether there are other consumers of displayHTMLInMessagePane() in c-c which are affected, too.
Flags: needinfo?(makemyday)
(In reply to [:MakeMyDay] from comment #27)
> Given displayHTMLInMessagePane() can be made to work again for that purpose,
> the (imho only) way to fix this and keep showing the diff and using the
> current way to display the invitation summary in the message body is to read
> out the dom object of the rendered email, clone that, replace the invitation
> summary part with the updated one and rewrite the displayed email by using
> displayHTMLInMessagePane().
Is there a bug for this or should be address it here?

You mean: Assuming that displayHTMLInMessagePane() can be made to work again ...?

displayHTMLInMessagePane() can be fixed by using the system principal instead of the null principal:
https://dxr.mozilla.org/comm-central/rev/706e5fdd301201b23e96be9826b7e5b953d11776/mailnews/base/src/nsMsgWindow.cpp#539
Simply use nsContentUtils::GetSystemPrincipal() in C++ code.

Personally I find using displayHTMLInMessagePane() quite hacky since you also lose the URL of what is really displayed when you use the DOM inspector, see bug 388584 comment #26:
===
With the invitation accepted, that changes to a long data URL:
data:text/html;base64,PGh0bWw+PGhlYWQ+PG1ldGEga... which (when decoded) shows the HTML for the invitation.
===

I'd much prefer to redisplay the message at the opportune moment making sure that this time the complete updated HTML is returned for the ICS parts.
Filed bug 1406574 for displayHTMLInMessagePane().
Assignee: jorgk → nobody
Status: ASSIGNED → NEW
Duplicate of this bug: 1518888
Duplicate of this bug: 1609331
You need to log in before you can comment on or make changes to this bug.