1667567 - After setting folder color, Next and Previous message navigation keys (F, N) don't work in folder, disabled in Go menu

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Wayne Mery (:wsmwk)]

I do not know whether this has always been broken since the folder color feature, or a more recent regression.

Workaround: change to a different folder and come back to the original folder

Comment 1

[Wayne Mery (:wsmwk)]

Are you able to reproduce?

Comment 2

[WaltS48 [:walts48]]

I am able to reproduce using 78.3.1 on Ubuntu 18.04 LTS.

One observation I noticed when switching back to the original colored folder, is that it became outlined in the original color (white using the Dark theme), but remains the changed color when selecting a different folder again.

Also on Windows10.

Comment 3

[Alessandro Castellani [:aleca]]

One observation I noticed when switching back to the original colored folder, is that it became outlined in the original color (white using the Dark theme), but remains the changed color when selecting a different folder again.

Did you click Ok in the Folder Properties dialog before selecting another folder?
If you dismiss that dialog or you click cancel it won't apply the edited colour.

Comment 4

[WaltS48 [:walts48]]

(In reply to Alessandro Castellani (:aleca) from comment #3)

One observation I noticed when switching back to the original colored folder, is that it became outlined in the original color (white using the Dark theme), but remains the changed color when selecting a different folder again.

Did you click Ok in the Folder Properties dialog before selecting another folder?
If you dismiss that dialog or you click cancel it won't apply the edited colour.

Yes, after applying the edited color the user can't use the message navigation keys in that folder as long it is showing the edited color, and they are disabled in the Go > Next menu.

Comment 5

[Alessandro Castellani [:aleca]]

I found the issue.
In order to allow the user to properly see the picked colour, I'm removing the selection from the folder item.
This can be fixed by refocusing the selected folder when the dialog closes.
I'll take care of it.

Comment 6

[Eckard Berberich]

I'm unable to reproduce this issue in TB 78.3.1 running macOS 10.14.6.

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1667567-folder-focus.diff

This patch takes care of restoring the selection in case the user interacted with the colour picker.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9179773 [details] [diff] [review]
1667567-folder-focus.diff

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

::: mailnews/base/content/virtualFolderProperties.js
@@ +13,5 @@
>  var gSearchFolderURIs = "";
>  var gMessengerBundle = null;
>  var kCurrentColor = "";
>  var kDefaultColor = "#363959";
> +var kFolderDeselected = false;

k is for Konstant (yes), g is for Global. 
This should be g I suppose. But it should also be made a positive verb since double negations get difficult to interpret. Like gNeedToRestoreFolderSelection

@@ +367,5 @@
>    // nothing for us to do here
>  }
>  
>  /**
>   * Clear the tree selection if the user opens the color picker.

Why do we need to do that? Can we avoid it in the first place?

Comment 9

[Alessandro Castellani [:aleca]]

This should be g I suppose. But it should also be made a positive verb since double negations get difficult to interpret. Like gNeedToRestoreFolderSelection

Sounds good.

Why do we need to do that? Can we avoid it in the first place?

We can't avoid it unfortunately because if the folder is selected in the tree the user won't be able to see the selected custom colour, since the tree item will have a background highlight colour and the icon would be white.

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: 1667567-folder-focus.diff

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9179960 [details] [diff] [review]
1667567-folder-focus.diff

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

LGTM, r=mkmelin

::: mailnews/base/content/virtualFolderProperties.js
@@ +302,5 @@
> +
> +/**
> + * If the user interacted with the color picker, it means the folder was
> + * deselected to ensure a proper preview of the color, so we need to select it
> + * back to ensure consistent focus.

Well, selection. I'll change it.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/12ee21eac5ac
Restore folder tree selection after color customization. r=mkmelin DONTBUILD

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9179960 [details] [diff] [review]
1667567-folder-focus.diff

[Approval Request Comment]
Regression caused by (bug #): folder icon colors
User impact if declined: can't use certain hotkeys after folder color change

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9179960 [details] [diff] [review]
1667567-folder-focus.diff

[Triage Comment]
Approved for beta

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/eb4cc5f09c04

Comment 16

[WaltS48 [:walts48]]

Looks good to me testing the 82.0b3 release candidate on Ubuntu 18.04 LTS.

Comment 17

[Wayne Mery (:wsmwk)]

Comment on attachment 9179960 [details] [diff] [review]
1667567-folder-focus.diff

[Triage Comment]
Approved for esr78

Comment 18

[Rob Lemley [:rjl]]

Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/b5e62df98985