Open Bug 1943520 Opened 22 days ago Updated 3 days ago

Conversations' is no longer able to fully clear the message pane - provide a specific function/option for clearing all with force clearing the web page view on the message pane

Categories

(Thunderbird :: Mail Window Front End, defect)

defect

Tracking

(thunderbird135?, thunderbird136?, thunderbird137?)

ASSIGNED
Tracking Status
thunderbird135 ? ---
thunderbird136 ? ---
thunderbird137 ? ---

People

(Reporter: standard8, Assigned: standard8)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Testing Conversations' with the latest Daily, I noticed that it is now unable to clear the message display to be able to switch to the multimessage display with the Conversations' URL loaded.

Currently Conversations is doing this:

    contentWin.messagePane._keepStartPageOpen = false;
    contentWin.messagePane.clearWebPage();
    contentWin.messagePane.clearMessage();
    // As a message will now have been displayed, don't keep the start page open.
    if (contentWin.multiMessageBrowser?.documentURI?.spec != STUB_URI) {
      MailE10SUtils.loadURI(contentWin.multiMessageBrowser, STUB_URI);
    }
    contentWin.multiMessageBrowser.hidden = false;

The _keepStartPageOpen has been changed to a private variable, so can no longer be set.

Conversations' doesn't use any of the display* methods, since they would do the wrong thing for what it needs.

I think there's two options:

  • Provide a forceCloseWebPage option to clearWebPage.
    • This would allow Conversations' to do what it needs. This would also allow the existing pattern of clearing the private property and then calling clearWebPage to be tidied up into one line.
  • Provide a dedicated display* function that does what Conversations' needs it to.

:aleca, any thoughts on the best option here?

Flags: needinfo?(alessandro)

I do not know if this is related, but would you be able to load your conversations view into the actual browser of the mailTab?

browser.tabs.update(<mailTabId>, {url: "conversationView.html"})`

You would then be independent of core and you could reduce your Experiment footprint even further.

I think we would need to modify the mailTabs.onSelectedMessagesChanged event and allow you to return an abort status, so Thunderbird does not try to do what it usually does. Similar to compose.onBeforeSend:

browser.mailTabs.onSelectedMessagesChanged.addListener((tab, messages) => {
  if (messages.messages.length > 1) {
   return {
       abort: true,
       url: "conversationView.html"
  }
})

Something like that.

(In reply to John Bieling (:TbSync) from comment #1)

I do not know if this is related, but would you be able to load your conversations view into the actual browser of the mailTab?

browser.tabs.update(<mailTabId>, {url: "conversationView.html"})`

That might be a possibility, but we would have to be able to pass a chrome URI (there's various content policy issues that stop us using a moz-extension uri currently) - or we could do the same as that, but via an experiment API.

We would also have to verify that the code for enabling/disabling menu options no longer relies on which browser is open in the message pane - but what's selected. It used to be the case, that a lot of code depended on if the web page browser or a message browser was open, but it seems like a lot of that might have been changed now.

That might be a possibility, but we would have to be able to pass a chrome URI (there's various content policy issues that stop us using a moz-extension uri currently) - or we could do the same as that, but via an experiment API.

For an official API, I cannot use a chrome URI here. I hoped you are generating your own view anyhow and do not need core resources. I will continue to think about this approach, but it will not be the fix to this bug.

(In reply to John Bieling (:TbSync) from comment #3)

That might be a possibility, but we would have to be able to pass a chrome URI (there's various content policy issues that stop us using a moz-extension uri currently) - or we could do the same as that, but via an experiment API.

For an official API, I cannot use a chrome URI here. I hoped you are generating your own view anyhow and do not need core resources. I will continue to think about this approach, but it will not be the fix to this bug.

To expand on this: Whilst, in theory, the Conversations UI is capable of being displayed in as a true WebExtension page, it can't display the messages in that case (as they get blocked by the content policy). The problem is that Conversations streams a message into an iframe, so that we can hook into Thunderbird's message streaming mechanism. This gives things like disabling JavaScript, remote content protection, calendar API, encryption handling basically for "free". We could take the body of the message & render it within Conversations, but then there would be a lot more work, and I would be concerned about the security implications.

(In reply to Mark Banner (:standard8) from comment #2)

(In reply to John Bieling (:TbSync) from comment #1)

I do not know if this is related, but would you be able to load your conversations view into the actual browser of the mailTab?

browser.tabs.update(<mailTabId>, {url: "conversationView.html"})`

I just tested the equivalent of this in a WebExtension experiment, although the page gets loaded, it then fails because browser.messageDisplay.getDisplayedMessages() is assuming that if the webBrowser is displayed, then no messages are displayed. I tried working around that, but then you start to hit other assumptions around what's actually being displayed, and it starts to get messy.

I'm going to look at the forceCloseWebPage option, I think that might be enough & the simpler option for now. Given Thunderbird is working on its on conversation style view, I don't think there's much sense in doing lots of work just to keep Conversations going.

This also tidies up some of the code to avoid having to clear the flag before calling clearWebPage.

Assignee: nobody → standard8
Status: NEW → ASSIGNED

Requesting tracking as this is add-ons bustage - for awareness following the switch to monthly releases, and hopefully quick-ish resolution.

Some add-ons might need to forcefully clear the webBrowser, but that
currently fails if the start page is shown. The only case were this
safty mechanism is actually needed is when clearAll() is called.

This patch therefore moves the check out of clearWebPage() into
clearAll(). A call to clearWebPage() now always clears the web
browwser.

Attachment #9465365 - Attachment description: Bug 1943520 - Always clear the webBrowser except if called vie clearAll() and the start page is still shown. r=standard8 → Bug 1943520 - Always clear the webBrowser except if called via
Attachment #9465365 - Attachment description: Bug 1943520 - Always clear the webBrowser except if called via → Bug 1943520 - Always clear the webBrowser except if called via clearAll() and the start page is still shown. r=standard8

I have added an alternative patch, which does not add the force flag.

BUT before trying it, can you please check if you can get the desired outcome now already by calling displayWebPage() (without params), which should already forcefully close the web browser ?

Attachment #9465365 - Attachment is obsolete: true
Attachment #9464834 - Attachment is obsolete: true

Thank you so much John for taking care of this.
I also noticed some unwanted leftovers views while working on my conversation view experiment, and I've been cleaning the message page file a bit: https://phabricator.services.mozilla.com/D231051#change-pgKsGPe9uAmO

My work is probably unrelated to these changes, but I wanted to share it just in case.

Flags: needinfo?(alessandro)

We currently set #keepStartPageOpen = false almost every time before
we call clearWebPage(). The only case were this safty mechanism is
actually needed is when clearAll() is called, which by default should
not clear the web browser if the start page is still shown. There is no
real need for the default behavior of clearWebPage() to have that
exception for the start page.

Once the web browser has been cleared, it will always be cleared
(except someting is setting #keepStartPageOpen back to true).

This patch moves setting #keepStartPageOpen = false into
clearWebPage(). The exception is directly handled in clearAll() by
checking the value of #keepStartPageOpen and skipping to clear the web
browser if needed.

For the case where clearAll() should indeed also clear the web
browser, this patch introduces an optional option.

This patch sightly improves code readability and simplifies clearing all
browsers including the web browser, if that is needed. With the current
code, one hast to call displayWebPage() with no params and then call
clearAll().

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: