Changing layout breaks with Mail Summaries and/or Conversations

ASSIGNED
Assigned to

Status

Thunderbird
Mail Window Front End
ASSIGNED
6 years ago
3 years ago

People

(Reporter: protz, Assigned: protz)

Tracking

Trunk
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 588748 [details] [diff] [review]
Add extra checks (be paranoid)

The code that's responsible for changing the layout (classic / wide / vertical) is pretty optimistic as to the state we're in when changing the layout. In particular, it assumes that the messagepane has a valid, initialized browser, and that the attachmentList DOM node is also valid and ready to poke.

Both assumptions break when someone's playing with the multimessage pane. For instance, with Mail Summaries, if we change layouts twice in a row, because the multimessage pane remains displayed, both the assumptions above are broken.

The attached patch adds some extra checks in the two places that fail.
Attachment #588748 - Flags: review?(squibblyflabbetydoo)
(Assignee)

Comment 1

6 years ago
The try / catch can be replaced by if (document.getElementById("messagepane").destroy) if you prefer.
Could this fix bug 531397?
(Assignee)

Comment 3

6 years ago
Huh I'm not sure, the symptoms here are more like "you can change layout once, but not twice".
(Assignee)

Updated

6 years ago
Assignee: nobody → jonathan.protzenko
Status: NEW → ASSIGNED

Comment 4

6 years ago
Comment on attachment 588748 [details] [diff] [review]
Add extra checks (be paranoid)

This doesn't work for me with Mail Summaries; with just a folder selected, it shows an empty thread summary page instead of the folder summary when switching layouts. However, it *does* improve things in that it's possible to change the layout more than once now.

It's entirely possible that this is an issue with the Mail Summaries code and not with Thunderbird, but I'd want to know why this is happening before I give this r+. Unfortunately, I don't have much time to debug this...

Clearing review out for now.
Attachment #588748 - Flags: review?(squibblyflabbetydoo)
(Assignee)

Comment 5

6 years ago
Hey Jim, have you had time to look into this? I'm a little bit puzzled by the fact that document.getElementById("messagepane") seems to be a dummy after you switched layouts once, and I don't really have an idea as to why this is happening...
Other bugs when changing View > Layout:

Bug 265393 - Changing View Layout does not preserve the mail message encoding
Bug 531397 - switching Layout modes breaks feed content display entirely until restart

Comment 7

5 years ago
Ping?  Jim, or anyone, have you had a chance to look into this?

Comment 8

3 years ago
Sent email to Jonathan, then found this bug listed.
Thunderbird version 31.0
when I selected the Vertical View and new error occured in previously empty error console.

Error console:
Timestamp: Thu 28/08/14 01:26:03:PM
Error: Please do not load stuff in the multimessage browser directly, use the SummaryFrameManager instead.
Source File: resource://gre/modules/summaryFrameManager.js
Line: 85

Which is in this section of code:
 _onLoad: function(event) {
    try {
      // Make sure we're responding to the summary frame being loaded, and not
      // some subnode.
      if (event.originalTarget != this.iframe.contentDocument)
        return;

      this.callback = this.pendingCallback;
      this.pendingCallback = null;
      if (this.pendingOrLoadedUrl != this.iframe.contentDocument.location.href)
        Components.utils.reportError(
          "Please do not load stuff in the multimessage browser directly, "+
          "use the SummaryFrameManager instead.");
      else if (this.callback)
        this.callback(true);
    }

Also:
Timestamp: Thu 28/08/14 01:26:03:PM
Error: messagepane.contentWindow is undefined
Source File: resource://conversations/modules/monkeypatch.js
Line: 752
which is this line:
 if (messagepane.contentWindow.location.href == "about:blank")

Located in this section:
 let messagepane = window.document.getElementById("messagepane");
    let fightAboutBlank = function () {
      if (messagepane.contentWindow.location.href == "about:blank") {
        Log.debug("Hockey-hack");
        // Workaround the "feature" that disables the context menu when the
        // messagepane points to about:blank
        messagepane.contentWindow.location.href = "about:blank?";
      }
(Assignee)

Comment 9

3 years ago
Thanks for the additional information. I guess the current status of this bug is "there's a patch that helps but we don't know the exact cause of the failure for sure". Lacking further investigation, it looks like things are stalled. (As to me, I don't think I changed my layout even once over the past few years, so I can live with having to restart Thunderbird after changing layouts.)

Thanks,

~ jonathan

Comment 10

3 years ago
I agree with you.
I can live with it as well.
As I had some info I just thought it might be useful if and when you need it at a later date.
You need to log in before you can comment on or make changes to this bug.