1625289 - Make the search dialogs themeable with themeableDialog.css

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Description

[Richard Marti (:Paenglab)]

The search Addresses and Search Messages dialogs are already themeable. But they still use the system buttons/menulists. With themeableDialog.css we can now theme theme too.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1625289-themeable-search-dialogs.patch

It was a bit tricky because this dialogs used a mic of header and content in one area. This patch works with normal LW-themes and WE-themes too.

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9136120 [details] [diff] [review]
1625289-themeable-search-dialogs.patch

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

Overall this looks good but there are some inconsistent alignments we should fix since we're updating the UI of these dialogs.

- The menulist doesn't show the chevron to indicate the presence of submenus.
- The checkboxes radio buttons are not aligned to the "Search for messages" label as that label comes with a margin-inline-start of 60x;
- Increase the spacing between the "Match" radio buttons as the second radio button is too attached to the label of the first one.
- We should get rid off of the status bar and print progress bar (in the address book search) and result messages at the bottom in the body of the dialog. (Is this possible?)

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: Screen Shot 2020-03-26 at 2.26.23 PM.png

A screenshot of the menulist issue on macos.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: 1625289-themeable-search-dialogs.patch

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

Comment on attachment 9136120 [details] [diff] [review]
1625289-themeable-search-dialogs.patch

Review of attachment 9136120 [details] [diff] [review]:

Overall this looks good but there are some inconsistent alignments we should
fix since we're updating the UI of these dialogs.

• The menulist doesn't show the chevron to indicate the presence of submenus.
  Checked on Linux and Windows but forgot to do on Mac and here it doesn't work. :-( fixed
• The checkboxes radio buttons are not aligned to the "Search for messages"
  label as that label comes with a margin-inline-start of 60x;
  fixed
• Increase the spacing between the "Match" radio buttons as the second radio
  button is too attached to the label of the first one.
  I used 18px. Okay?
• We should get rid off of the status bar and print progress bar (in the
  address book search) and result messages at the bottom in the body of the
  dialog. (Is this possible?)
  There was wrong code which stopped the display of the search result. And it seems the progressmeter was never used. I leaved the statusbar for consistency with the message search but removed the progressmeter. With the statusbar where is a dedicated place to show the result. It would be possible to remove the statusbar and put the result into the content but then we need to define the space for the text and before the first search this area would be empty. When no space is reserved, the whole content would jump when the result would be shown. What do you think?

Comment 5

[Alessandro Castellani [:aleca]]

Comment on attachment 9136303 [details] [diff] [review]
1625289-themeable-search-dialogs.patch

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

Looks better, thanks, but I think we need to tweak the style a little bit more.
As you can see from the next screenshot, the selected richlistitem looks a bit weird with the box shadow and the grey background.
I'd leave the highlight status as it is right now, using the highlight color for the entire row background.

Also, the fact that menulists have a --button-background which has 0.2 alpha, causes the color to shift when the row is highlighted.
I saw you edited those variables for the dark mode, so I think we should update them for the default theme as well.
What do you think?

I'm okay with leaving the status bar for now, we can explore dropping it in a follow up bug.

::: mail/themes/linux/mail/searchDialog.css
@@ +25,5 @@
>  treechildren::-moz-tree-cell-text(subjectCol) {
>    padding-inline-start: 0;
>  }
>  
> +#checkSearchSubFolders {

Add the #checkSearchOnline as well

::: mail/themes/osx/mail/searchDialog.css
@@ +11,5 @@
>  #searchTerms > vbox {
>    font: menu;
>  }
>  
> +#checkSearchSubFolders {

Also here the #checkSearchOnline should be included.

Comment 6

[Alessandro Castellani [:aleca]]

Attachment: Screen Shot 2020-03-27 at 9.07.05 AM.png

Comment 7

[Richard Marti (:Paenglab)]

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

Comment on attachment 9136303 [details] [diff] [review]
1625289-themeable-search-dialogs.patch

Review of attachment 9136303 [details] [diff] [review]:

Looks better, thanks, but I think we need to tweak the style a little bit
more.
As you can see from the next screenshot, the selected richlistitem looks a
bit weird with the box shadow and the grey background.
I'd leave the highlight status as it is right now, using the highlight color
for the entire row background.

When the entire row is highlighted, the buttons/menulists are blue too because of their transparency. I could do only a box shadow for the selected, not focused row like the highlight focus. When selected and focused it would only show the highlight box shadow.

Also, the fact that menulists have a --button-background which has 0.2
alpha, causes the color to shift when the row is highlighted.
I saw you edited those variables for the dark mode, so I think we should
update them for the default theme as well.

Where did I edit them?

Comment 8

[Alessandro Castellani [:aleca]]

Here

:root:not([lwt-tree]) #searchTermBox:-moz-lwtheme-brighttext,
:root:not([lwt-tree]) #searchResults:-moz-lwtheme-brighttext {
  --button-background: #d2d2d3;
  --button-background-hover: #c2c2c3;
  --button-background-active: #b2b2b3;

Comment 9

[Richard Marti (:Paenglab)]

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

Here

:root:not([lwt-tree]) #searchTermBox:-moz-lwtheme-brighttext,
:root:not([lwt-tree]) #searchResults:-moz-lwtheme-brighttext {
  --button-background: #d2d2d3;
  --button-background-hover: #c2c2c3;
  --button-background-active: #b2b2b3;

This is when a dark LW-theme (the simple ones with only a background image and text colour) is used. Then the richlist and the tree are still light. This colours aren't transparent but for the default theme I need transparent ones to work under dark Linux themes too.

Comment 10

[Alessandro Castellani [:aleca]]

Mhh, this is tricky.
Could we use the background color highlight for the selected list, and with CSS force change the background color of child buttons and menulists to a flat grey without transparency?
And handle the dark variation as well.

If this gets too tricky, we can simply remove the inset drop shadow, which looks very strange, and simply highlight the selected richlistitem with the grey background.

Comment 11

[Richard Marti (:Paenglab)]

Attachment: 1625289-themeable-search-dialogs.patch

Okay, this should work now.

Comment 12

[Alessandro Castellani [:aleca]]

Comment on attachment 9136420 [details] [diff] [review]
1625289-themeable-search-dialogs.patch

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

Perfect, thank you so much!
r+

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ff8590f85d5e
Make the search dialogs themeable with themeableDialog.css. r=aleca DONTBUILD