1692764 - Right clicking on a folder mode header makes the tree unresponsive

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect, P2)

Description

[Wayne Mery (:wsmwk)]

I assume this is a regression of sorts.

1. enable Folder Pane Toolbar
2. enable a second type of folder, Favorites
3. click on All Folders
4. File > Compact Folders
5. click any folder

Results: message list does not change. must restart Thunderbird

Expected results: Compact menu item should probably be disabled when folder type is focused.

Comment 1

[Wayne Mery (:wsmwk)]

Walt, can you reproduce this?

Step 4 can also be just as simple as right clicking on All Folders or Favorite Folders - no action needs to be selected (none is presented).
Still nothing in error console.

Somehow I managed in one iteration to get folder switching to work again without restarting. I think it involved creating a new message. In another iteration I right clicked on an account and picked "New Folder" which seems to consistently reverse the folder switching problem.

Comment 2

[WaltS48 [:walts48]]

Yes, I can reproduce this using Thunderbird 86.0b3 on Fedora 33 Workstation.

I used Unread and All Folders for my test.

Didn't get it to work by creating a new message or new folder.

Comment 3

[Alessandro Castellani [:aleca]]

I'm able to reproduce this only with a right click.
It seems that the return false; on the opening of the context menu freezes the UI, regardless if the File > Compact Folders is used.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: 1692764-folder-compact-header.diff

All right, this is tricky.

It seems that returning false when right clicking on a Folder Mode Header, causes the tree to sort of freeze to the point where selecting other rows doesn't change the thread view.

I fixed it by allowing the context menu to be shown also on the Header Modes, but showing only the "Search Messages" item, which is the only one that works without a properly selected folder.
I think this is acceptable as an intermediate step to fix this issue, and also the initial step for bug 1686306, where I'm planning to implement sorting options for folder modes, with menu items in the context menu to move up and down the selected mode.

The fillFolderPaneContextMenu() is very messy, but I didn't want to update the code too much and only focus on the part to make the tree work.
If this gets approved, I'd like to do a quick follow up to clean up the code which needs a lot of love.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9205580 [details] [diff] [review]
1692764-folder-compact-header.diff

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

::: mail/base/content/folderPane.js
@@ +756,5 @@
>     * Returns an array of nsIMsgFolders corresponding to the current selection
> +   * in the tree.
> +   *
> +   * @param {boolean} includeHeaders - If folder mode headers should be included
> +   *   in the returned array.

But the headers are not folders, so for the one case you want them, maybe add a special function to get only those, so that we don't confuse things?

::: mail/base/content/mailContextMenus.js
@@ +279,5 @@
>    if (!folders.length) {
>      return false;
>    }
>  
> +  let haveAnyVirtualFolders = folders.some(

you could just inline this one as well

@@ +361,4 @@
>    function checkCanGetMessages(folder) {
> +    if (!folder) {
> +      return false;
> +    }

Is it ever null?

@@ +470,5 @@
>    }
>  
>    // --- Set up the delete folder menu item.
>    function checkCanDeleteFolder(folder) {
> +    if (!folder) {

when is it null?

::: mail/base/content/msgMail3PaneWindow.js
@@ +1482,5 @@
>   *     leave the selection as is.  If it is part of the selection and
>   *     aSingleSelect then we create a transient single-row selection.
>   */
>  function ChangeSelectionWithoutContentLoad(event, tree, aSingleSelect) {
> +  // Return if we don't have a view. This might if the user right clicks on the

might happen?

Comment 6

[Magnus Melin [:mkmelin]]

Did you figure out why returning false makes it freeze?

Comment 7

[Alessandro Castellani [:aleca]]

(In reply to Magnus Melin [:mkmelin] from comment #6)

Did you figure out why returning false makes it freeze?

No idea, and it baffles me.
Do you have any hint?

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1692764-mode-header-right-click.diff

I found the issue!

Right clicking on a mode header grabs the selection without properly restoring it to the original treechildren like it happens for any other element.
So the tree remains stuck on a selection that is not really a folder and it's not able to move it to another clicked item.

Ensuring that we restore the original selection if the user right clicks on a mode header fixes the problem.
I'll defer the code improvements to the generation of the context menu to another bug since this fix is very simple.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9206516 [details] [diff] [review]
1692764-mode-header-right-click.diff

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

Thanks for digging into it! r=mkmelin

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bebb326ced3f
Fix right click freezing the folder pane when a Mode Header is selected. r=mkmelin