1545562 - Missing icon in Converter Dialog for Windows and macOS

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Alessandro Castellani [:aleca]]

The converter dialog styled in Bug 1539141 shows the icon only on Linux.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: converter-dialog.patch

It seems that I'm not able to load the chrome://global/skin/global.css file into the XHTML dialog, as well as the chrome://global/skin/icons/warning-48.png icons.

I solved the missing icon problem by replacing the background-image with chrome://global/skin/icons/warning-large.png.

Any suggestions on how to solve this issue?

Comment 2

[Richard Marti (:Paenglab)]

Maybe you should add a

html { background-color: -moz-Dialog; }

to have the correct background color to the body { color: -moz-DialogText; } of converterDialog.css.
This makes the dialog also more looking like a dialog.

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: converter-dialog.patch

Added -moz-Dialog background color to the dialog.

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9059575 [details] [diff] [review]
converter-dialog.patch

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

::: mail/base/content/converterDialog.css
@@ +27,4 @@
>  }
>  
>  .infoIcon {
> +  background-image: url("chrome://global/skin/icons/warning-large.png");

Now you're linking to an icon which doesn't exist on Linux. Cross-platform stuff is way more complicated than it should be. :(

Comment 5

[Alessandro Castellani [:aleca]]

(In reply to Geoff Lankow (:darktrojan) from comment #4)

Now you're linking to an icon which doesn't exist on Linux.

Can you help me understand the current structure and do's and dont's related to the cross-platform implementation in TB?

How come the chrome://global/skin/** assets are not loaded in macOS, but the chrome://global/skin/icons/warning-large.png is loaded, but not on Linux?

How are those assets defined and how do I know which is available in a platform without having to load the patch on different computers?

Comment 6

[Geoff Lankow (:darktrojan)]

The skin folders can be annoying because, in general, there's a different one for each platform. In the case of global, the contents are from toolkit/themes/linux/global, toolkit/themes/osx/global, or toolkit/themes/windows/global. (It gets worse, sometimes the manifest says to pack the contents in a way that doesn't relate to where they live in the tree.)

The real problem you're hitting with this patch is that somebody decided Linux should use native icons wherever possible (hence the moz-icon://stock/gtk-dialog-warning bit), and therefore the Linux theme has no warning-large.png, but the others do.

I guess what you should do is create a converterDialog.css file for each platform as there is for sanitizeDialog.css.

Comment 7

[Alessandro Castellani [:aleca]]

Awesome, thank you so much for the walk-through, I'll probably print what you wrote and keep it close by :D
I'll create the CSS variations like you suggested, and test on Linux and macOS.
Unfortunately, I don't have a Windows computer to test how it looks, can I assume it will look fairly close to the macOS one in terms of icons and spacing?
Cheers

Comment 8

[Geoff Lankow (:darktrojan)]

I usually ask Richard to check everything looks okay cross-platform.

Edit: That said, it's probably fine, the same as OS X but with a different font and a different icon.

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: convert-dialog.patch

I created the CSS variations for the various platforms and left the base file handling the shared style.

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: convert-dialog.patch

Forgot to add those extra files created.

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9059978 [details] [diff] [review]
convert-dialog.patch

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

I'm happy with this if Richard is.

Comment 12

[Richard Marti (:Paenglab)]

Comment on attachment 9059978 [details] [diff] [review]
convert-dialog.patch

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

Looks good, thanks.
Please describe in the patch description what you are doing in this patch and add the reviewer. Then Jörg doesn't need to do this while landing.

::: mail/themes/linux/mail/converterDialog.css
@@ +3,5 @@
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +.infoIcon {
> +  background-image: url("moz-icon://stock/gtk-dialog-warning?size=dialog");
> +}
\ No newline at end of file

Please add a LF at the end of all three new CSS files.

::: mailnews/base/prefs/content/converterDialog.xhtml
@@ +13,5 @@
> +  <head>
> +    <meta charset="UTF-8" />
> +    <title>&converterDialog.title;</title>
> +    <link rel="stylesheet" href="chrome://messenger/skin/converterDialog.css" type="text/css" />
> +    <link rel="stylesheet" href="chrome://messenger/content/converterDialog.css" type="text/css" />

Could you exchange the order of the two stylesheet links? Then it would be easier, if needed, to override a common rule with the platform stylesheet when it's applied after the common stylesheet.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9059978 [details] [diff] [review]
convert-dialog.patch

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

::: mailnews/base/prefs/content/converterDialog.xhtml
@@ +13,5 @@
> +  <head>
> +    <meta charset="UTF-8" />
> +    <title>&converterDialog.title;</title>
> +    <link rel="stylesheet" href="chrome://messenger/skin/converterDialog.css" type="text/css" />
> +    <link rel="stylesheet" href="chrome://messenger/content/converterDialog.css" type="text/css" />

Or perhaps even better @import the content/converterDialog.css in the skin converterDialog.

This is also used sometimes.

Comment 14

[Alessandro Castellani [:aleca]]

Attachment: convert-dialog.patch

Updated patch, with comment and review.
I'm importing the base CSS file in each skin related file as suggested by Magnus.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cfed55c5d8d0
Fix missing icon in Converter Dialog for Windows and macOS. r=darktrojan,Paenglab