1684666 - Customization of focussed/selected items (folders and messages) via theme

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Description

[Nyquister Oux]

Attachment: thunderbird-focussed-item.jpg

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/87.0.4280.88 Safari/537.36

Steps to reproduce:

I'd like customize a border and background color of focussed/selected items in the list of folders and messages.

Actual results:

Actually there is no solution to style this elements via theme, so this makes these elements very ugly if a dark theme is used (screenshot attached).

Expected results:

It would be great to have a possibility of change background color and border of focussed/selected items in the future versions of Thunderbird.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1684666-add-sidebar_highlight_border.patch

This patch copies the toolkit ext-theme.js and theme.json to our tree and adds the needed parts for the sidebar_highlight_border. I added this property also to our dark/light themes also they are the same as the defaults. Like this we are free if we want to change only one value, either in default or in the theme.

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9196812 [details] [diff] [review]
1684666-add-sidebar_highlight_border.patch

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

The JavaScript side looks good, other than a couple of tiny nits.
I'd recommend asking for a feedback or an extra review of TbSync or another add-on peer to be sure I haven't missed anything as I'm not really acquainted with the extensions source code.

::: mail/components/extensions/parent/ext-theme.js
@@ +34,5 @@
> +class Theme {
> +  /**
> +   * Creates a theme instance.
> +   *
> +   * @param {string} extension Extension that created the theme.

nit: We add a dash "-" after the attribute name before the explanation.
Just to keep it consistent. Please do it for all the other JSDocs comments.

@@ +53,5 @@
> +
> +    if (startupData && startupData.lwtData) {
> +      Object.assign(this, startupData);
> +    } else {
> +      // TODO(ntim): clean this in bug 1550090

Update this to reflect the possible code change, something like:
// TODO: Update this part after bug 1550090

@@ +87,5 @@
> +   * Loads a theme by reading the properties from the extension's manifest.
> +   * This method will override any currently applied theme.
> +   *
> +   * @param {Object} details Theme part of the manifest. Supported
> +   *   properties can be found in the schema under ThemeType.

This variation is not passed to the load() method as attribute, so we can remove it from the comment.

::: mail/components/extensions/schemas/theme.json
@@ +88,5 @@
> +              },
> +              "headerURL": {
> +                "$ref": "ImageDataOrExtensionURL",
> +                "optional": true,
> +                "deprecated": "Unsupported images property, use 'theme.images.theme_frame', this alias is ignored in Firefox >= 70."

Should we replace whenever it says Firefox with Thunderbird?
Do we know if these parameter where ever supported in the past?

Comment 3

[Richard Marti (:Paenglab)]

Attachment: 1684666-add-sidebar_highlight_border.patch

Fixed the comments.

John, please can you check if the newly added files are in sync with the TB files? They're copies from FX and working since long time. I only added the property sidebar_highlight_border.

Are you also doing the documentation? If yes, we need to add this property sidebar_highlight_border as it is TB specific.

Comment 4

[John Bieling (:TbSync)]

Thanks for your work on this, I like the idea of having a dedicated theme API for Thunderbird very much! This patch does not include the usage of the new color, right? I could not find a reference to sidebar_highlight_border. Can I test it?

As we include this API only now, I suggest to remove the 3 deprecated parameters from the schema and also eliminate the bookmark_text alias from the schema and the implementation, as it does not apply to us, if you agree.

What I really really would like to see (but maybe it is beyond the scope of this patch), is to get descriptions added to the colors.properties entries in the schema files, which indicate where it is used. This is something I have no knowledge of and which I have not documented yet (see here, very poor)

Our API documentation is generated from these schema files and if we get our own theme API, its type definitions including its descriptions will be accessible like so:
https://webextension-api.thunderbird.net/en/78/contacts.html#contactnode

If you happen to know where some of these color definitions are used, it would be great if you could fill in a short description.

Comment 5

[John Bieling (:TbSync)]

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

As we include this API only now, I suggest to remove the 3 deprecated parameters from the schema and also eliminate the bookmark_text alias from the schema and the implementation, as it does not apply to us, if you agree.

I just checked the code of add-ons listed on ATN and some of them use bookmark_text. So we cannot remove it without rendering them broken (as the manifest will no longer be valid). Same applies to the deprecated elements. So we need to keep the schema as it is.

Comment 6

[Richard Marti (:Paenglab)]

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

Thanks for your work on this, I like the idea of having a dedicated theme API for Thunderbird very much! This patch does not include the usage of the new color, right? I could not find a reference to sidebar_highlight_border. Can I test it?

The default themes have it in their manifest.json. When you select a treechildren, the border around it uses this colour.

Comment 7

[John Bieling (:TbSync)]

Attachment: test_theme.xpi

Test theme using "sidebar_highlight_border".

Comment 8

[John Bieling (:TbSync)]

(In reply to Richard Marti (:Paenglab) from comment #6)

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

Thanks for your work on this, I like the idea of having a dedicated theme API for Thunderbird very much! This patch does not include the usage of the new color, right? I could not find a reference to sidebar_highlight_border. Can I test it?

The default themes have it in their manifest.json. When you select a treechildren, the border around it uses this colour.

I do not see that happen. I applied the patch and tried to use the new color definition in a test theme (attached). Can you help me to see what I am missing?

Comment 9

[Richard Marti (:Paenglab)]

Attachment: 1684666-add-sidebar_highlight_border.patch

Hmm, somehow the ThemeVariableMap.jsm change didn't go it into the patch. Re-requesting review with the only change of adding the ThemeVariableMap.jsm change.

Comment 10

[Richard Marti (:Paenglab)]

Attachment: test_theme.xpi

With only this parameter the trees aren't themed because at least the "sidebar_text" parameter is needed to enable the tree theming. I added more parameters for the trees.

Comment 11

[Richard Marti (:Paenglab)]

Attachment: 1684666-theme.json-descriptions.patch

This is only the JSON file with the descriptions added. Would this be okay like this?

Comment 12

[Alessandro Castellani [:aleca]]

Comment on attachment 9197147 [details] [diff] [review]
1684666-add-sidebar_highlight_border.patch

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

Looks good!

Comment 13

[John Bieling (:TbSync)]

Comment on attachment 9197150 [details] [diff] [review]
1684666-theme.json-descriptions.patch

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

Thank you for your effort!

Comment 14

[John Bieling (:TbSync)]

Even with the updated patch, I do not get the red border which the attached theme should have. On Windows. Any idea?

Edit: Did not see the comment that "sidebar_text" is needed. Works!

Comment 15

[Richard Marti (:Paenglab)]

Attachment: 1684666-add-sidebar_highlight_border.patch

Folded both patches together.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/85f291dbdace
Add a sidebar_highlight_border property to the theme schema. r=aleca,TbSync

Comment 17

[Richard Marti (:Paenglab)]

Attachment: 1684666-add-sidebar_highlight_border-ESR.patch

[Approval Request Comment]
User impact if declined: For theme authors it isn't possible to style the border around selected treechildrens
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): should be low

Comment 18

[Wayne Mery (:wsmwk)]

Will take this on esr after time on beta 86

Comment 19

[Wayne Mery (:wsmwk)]

Comment on attachment 9197347 [details] [diff] [review]
1684666-add-sidebar_highlight_border-ESR.patch

[Triage Comment]
Approved for esr78

Comment 20

[Rob Lemley [:rjl]]

Thunderbird 78.7.1:
https://hg.mozilla.org/releases/comm-esr78/rev/f47fbacf8200

Comment 21

[Rob Lemley [:rjl]]

Thunderbird 78.7.1:
https://hg.mozilla.org/releases/comm-esr78/rev/1ef5b612ee43

Comment 22

[Nyquister Oux]

Thank you very much. Everything works great, just like I dreamed of :)

However, I have an additional question: Is it now possible to modify the border of a "selected" but not "highlighted" element?

I mean the situation: I select / highlight a specific folder, then click on the specific message and highlight it. As a result, the folder is no longer "highlighted" but still "selected" and still has a border around it, which is not affected by the "sidebar_highlight_border" parameter.

It would be great, but if there is no such possibility, it is still very good. Thank you again.

Comment 23

[Richard Marti (:Paenglab)]

We set this only when the tree is focused:

:root[lwt-tree] #threadTree treechildren::-moz-tree-row(untagged, selected, focus, current),
:root[lwt-tree] tree:not(#threadTree) treechildren::-moz-tree-row(selected, focus, current) {
  border-color: var(--sidebar-highlight-border-color, var(--item-focus-selected-border-color));
}

You can try to override the unfocused state with adding the above code without the focus in your theme's experiment CSS. I haven't tested if this works.