1457781 - Use warning.svg instead of Warning.png on Windows

Status: RESOLVED FIXED

(Toolkit :: Themes, enhancement, P3)

Description

[Tim Nguyen :ntim]

Remove toolkit/themes/windows/global/icons/Warning.png: https://dxr.mozilla.org/mozilla-central/search?q=%2FWarning.png&redirect=false


and instead of chrome://global/skin/icons/Warning.png use chrome://global/skin/icons/warning.svg

Comment 1

[Manish [:manishkk]]

'warning.svg' doesn't exist.
https://dxr.mozilla.org/mozilla-central/search?q=%2FWarning.svg&redirect=false

Comment 2

[Manish [:manishkk]]

Do I need to remove this line also from 'jar.mn'
https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/jar.mn#54

Comment 3

[Tim Nguyen :ntim]

> 'warning.svg' doesn't exist.
> https://dxr.mozilla.org/mozilla-central/search?q=%2FWarning.svg&redirect=false

You can refer to:
chrome://global/skin/icons/warning.svg

(In reply to Manish Kumar [:manishkk] from comment #2)
> Do I need to remove this line also from 'jar.mn'
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> jar.mn#54

Yep.

Comment 4

[Manish [:manishkk]]

Attachment: Patch_Bug1457781

Please review

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8985542 [details] [diff] [review]
Patch_Bug1457781

>--- a/toolkit/themes/windows/global/global.css
>+++ b/toolkit/themes/windows/global/global.css
>@@ -67,7 +67,7 @@ window.dialog {
> 
> .alert-dialog #infoIcon,
> .alert-icon {
>-  list-style-image: url("chrome://global/skin/icons/Warning.png");
>+  list-style-image: url("chrome://global/skin/icons/warning.svg");

Do you also need to set a size? Warning.png is 32*32px but warning.svg is 16*16px by default.

Also, do you need to set a color? toolkit/themes/shared/icons/warning.svg doesn't have a default fill (unlike browser/themes/shared/warning.svg).

You'll need to remove this too: https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/themes/windows/global/global.css#82-86

Comment 6

[Korina]

Hi I'd like to finish this bug. I've never contributed to the open-source community before and this looks like a great bug to start with.

Comment 7

[Dão Gottwald [:dao]]

Okay, let me know if you have questions.

Comment 8

[Korina]

Hi Dao, 

How do I test my changes? Where is this warning displayed on Windows? I'm asking to test if we need to specify a default size. 

Thanks, 

Korina

Comment 9

[Korina]

Dao, 

To answer your question about specifying the color- the default is black. This is the same as warning.svg inside browser/themes/shared/warning.svg. Does it need to be a different color? 

Thanks,

Korina

Comment 10

[Korina]

Attachment: mypatch.diff

Comment 11

[Dão Gottwald [:dao]]

(In reply to Korina from comment #9)
> Dao, 
> 
> To answer your question about specifying the color- the default is black.
> This is the same as warning.svg inside browser/themes/shared/warning.svg.
> Does it need to be a different color? 
> 
> Thanks,
> 
> Korina

browser/themes/shared/warning.svg is yellow by default because it uses fill="context-fill #FFBF00" whereas toolkit/themes/shared/icons/warning.svg uses fill="context-fill". We can update the toolkit icon to use #FFBF00 as well.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 9007647 [details] [diff] [review]
mypatch.diff

> .alert-dialog #infoIcon,
> .alert-icon {
>-  list-style-image: url("chrome://global/skin/icons/Warning.png");
>+  list-style-image: url("chrome://global/skin/icons/warning.svg");

Can you set width and height to 32px here instead of changing the size in the SVG file?

>--- a/toolkit/themes/windows/global/jar.mn
>+++ b/toolkit/themes/windows/global/jar.mn
>@@ -45,14 +45,14 @@ toolkit.jar:
>   skin/classic/global/icons/Error.png                      (icons/Error.png)
>   skin/classic/global/icons/collapse.png                   (icons/collapse.png)
>   skin/classic/global/icons/expand.png                     (icons/expand.png)
>   skin/classic/global/icons/Landscape.png                  (icons/Landscape.png)
>   skin/classic/global/icons/Portrait.png                   (icons/Portrait.png)
>   skin/classic/global/icons/Print-preview.png              (icons/Print-preview.png)
>   skin/classic/global/icons/Search-close.png               (icons/Search-close.png)
>   skin/classic/global/icons/Question.png                   (icons/Question.png)
>-  skin/classic/global/icons/sslWarning.png                 (icons/sslWarning.png)
>-  skin/classic/global/icons/Warning.png                    (icons/Warning.png)
>+  skin/classic/global/icons/sslWarning.png  
>+  skin/classic/global/icons/warning.svg               (icons/Warning.png)

Don't need to add warning.svg here, it's already packaged in shared/: https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/toolkit/themes/shared/jar.inc.mn#48
Also, the sslWarning.png line is messed up...

Comment 13

[Korina]

(In reply to Dão Gottwald [::dao] from comment #12)
> Comment on attachment 9007647 [details] [diff] [review]
> mypatch.diff
> 
> > .alert-dialog #infoIcon,
> > .alert-icon {
> >-  list-style-image: url("chrome://global/skin/icons/Warning.png");
> >+  list-style-image: url("chrome://global/skin/icons/warning.svg");
> 
> Can you set width and height to 32px here instead of changing the size in
> the SVG file?
> 
> >--- a/toolkit/themes/windows/global/jar.mn
> >+++ b/toolkit/themes/windows/global/jar.mn
> >@@ -45,14 +45,14 @@ toolkit.jar:
> >   skin/classic/global/icons/Error.png                      (icons/Error.png)
> >   skin/classic/global/icons/collapse.png                   (icons/collapse.png)
> >   skin/classic/global/icons/expand.png                     (icons/expand.png)
> >   skin/classic/global/icons/Landscape.png                  (icons/Landscape.png)
> >   skin/classic/global/icons/Portrait.png                   (icons/Portrait.png)
> >   skin/classic/global/icons/Print-preview.png              (icons/Print-preview.png)
> >   skin/classic/global/icons/Search-close.png               (icons/Search-close.png)
> >   skin/classic/global/icons/Question.png                   (icons/Question.png)
> >-  skin/classic/global/icons/sslWarning.png                 (icons/sslWarning.png)
> >-  skin/classic/global/icons/Warning.png                    (icons/Warning.png)
> >+  skin/classic/global/icons/sslWarning.png  
> >+  skin/classic/global/icons/warning.svg               (icons/Warning.png)
> 
> Don't need to add warning.svg here, it's already packaged in shared/:
> https://searchfox.org/mozilla-central/rev/
> d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/toolkit/themes/shared/jar.inc.mn#48
> Also, the sslWarning.png line is messed up...


I don't think setting the height and width attributes within the CSS file will work. I've attempted it manually in the browser tools and have read about it. Is there a reason we don't want to set the size within the SVG file?

Comment 14

[Korina]

Thank you for pointing out that sslWarning line- I've fixed it.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Korina from comment #13)
> I don't think setting the height and width attributes within the CSS file
> will work.

Hmm, I don't see why this wouldn't work.

> Is there a reason we don't want to set the size within the SVG
> file?

Consistency with other icons, warning.svg and help.svg in particular. 16*16px is our default size for most icons.

Comment 16

[Korina]

(In reply to Dão Gottwald [::dao] from comment #15)
> (In reply to Korina from comment #13)
> > I don't think setting the height and width attributes within the CSS file
> > will work.
> 
> Hmm, I don't see why this wouldn't work.

If you set the width and height of the SVG using developer tools, you can see it has no impact on the true size. I've worked a little with SVG's before and read a few long articles about manipulating SVG's. This stack overflow has a simple explanation for why the height and width attributes won't work: 
https://stackoverflow.com/questions/39056537/why-dont-my-svg-images-scale-using-the-css-width-property

Comment 17

[Dão Gottwald [:dao]]

warning.svg certainly can be scaled. For instance, we do it in about:networking:

https://searchfox.org/mozilla-central/rev/9e7995b3c384945740985107a4de601b27904718/toolkit/themes/shared/aboutNetworking.css#89-90

Comment 18

[Korina]

(In reply to Dão Gottwald [::dao] from comment #17)
> warning.svg certainly can be scaled. For instance, we do it in
> about:networking:
> 
> https://searchfox.org/mozilla-central/rev/
> 9e7995b3c384945740985107a4de601b27904718/toolkit/themes/shared/
> aboutNetworking.css#89-90

Ok thank you for that reference! I've tried to apply something similar but once again I'm not sure how to test this locally. Do you know where this warning icon appears? I've posted a patch as well.

Comment 19

[Korina]

Attachment: mypatch.diff

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 9008836 [details] [diff] [review]
mypatch.diff

>--- a/toolkit/themes/shared/icons/warning.svg
>+++ b/toolkit/themes/shared/icons/warning.svg
>@@ -1,6 +1,6 @@
> <!-- This Source Code Form is subject to the terms of the Mozilla Public
>    - License, v. 2.0. If a copy of the MPL was not distributed with this
>    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
>-  <path fill="context-fill" d="M14.742 12.106L9.789 2.2a2 2 0 0 0-3.578 0l-4.953 9.91A2 2 0 0 0 3.047 15h9.905a2 2 0 0 0 1.79-2.894zM7 5a1 1 0 0 1 2 0v4a1 1 0 0 1-2 0zm1 8.25A1.25 1.25 0 1 1 9.25 12 1.25 1.25 0 0 1 8 13.25z"/>
>+  <path fill="#FFBF00" d="M14.742 12.106L9.789 2.2a2 2 0 0 0-3.578 0l-4.953 9.91A2 2 0 0 0 3.047 15h9.905a2 2 0 0 0 1.79-2.894zM7 5a1 1 0 0 1 2 0v4a1 1 0 0 1-2 0zm1 8.25A1.25 1.25 0 1 1 9.25 12 1.25 1.25 0 0 1 8 13.25z"/>

Should be fill="context-fill #FFBF00"

> .alert-dialog #infoIcon,
> .alert-icon {
>-  list-style-image: url("chrome://global/skin/icons/Warning.png");
>+  list-style-image: url("chrome://global/skin/icons/warning.svg");
> }
> 
>+.alert-icon::before {
>+  background-size: 2em; 
>+}

list-style-image works differently than background-image, so background-size won't work here. Setting width and height in the same rule where you set list-style-image should work.

I think you can test this by loading chrome://pippki/content/resetpassword.xul in a tab.

Comment 21

[Korina]

Ok thanks for the clarifications! One more (probably simple) question- do I need to run ./mach build every time? Is there a way to  just build the code I changed or is that done already? Thanks.

Comment 22

[Korina]

Attachment: mypatch2.diff

Comment 23

[Dão Gottwald [:dao]]

If you only change front-end code (like CSS or Javascript) and not compiled code (like C++), you can use artifact builds which are very fast:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 9008844 [details] [diff] [review]
mypatch2.diff

This patch is messed up because it contains .patch and .diff files that you seem to have saved in toolkit/themes/. Please remove these and save your patches somewhere else, outside of your mozilla-central working directory.

Comment 25

[Korina]

Attachment: mypatch3.diff

This patch has the discussed changes and is saved outside of the mozilla-source directory.

Comment 26

[Tim Nguyen :ntim]

Comment on attachment 9008867 [details] [diff] [review]
mypatch3.diff

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

::: toolkit/themes/windows/global/global.css
@@ +72,5 @@
>  .alert-dialog #infoIcon,
>  .alert-icon {
> +  list-style-image: url("chrome://global/skin/icons/warning.svg");
> +  width: 32px; 
> +  height: 32px; 

nit: remove trailing whitespace

::: toolkit/themes/windows/global/icons/warning.svg
@@ +1,4 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg"  width="32" height="32" viewBox="0 0 16 16">

This file isn't needed as it's already packaged in shared/, please remove it.

Comment 27

[Korina]

Attachment: mypatch.diff

Removed toolkit/themes/windows/global/icons/warning.svg since it was already in the shared directory. 

I wasn't sure what trailing whitespace there was to remove.

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 9008984 [details] [diff] [review]
mypatch.diff

The trailing spaces are after the semicolons:

> .alert-dialog #infoIcon,
> .alert-icon {
>-  list-style-image: url("chrome://global/skin/icons/Warning.png");
>+  list-style-image: url("chrome://global/skin/icons/warning.svg");
>+  width: 32px; 
----------------^
>+  height: 32px; 
-----------------^

Also please remove Warning.png:

hg rm toolkit/themes/windows/global/icons/Warning.png

Comment 29

[Korina]

I ran the hg command but Warning.png was deleted on my local copy. There are warning-16.png warning-64.png and warning-large.png. Did you still want those?

Comment 30

[Korina]

Attachment: patch.diff

Removed warning.png and removed trailing whitespace

Comment 31

[Dão Gottwald [:dao]]

(In reply to Korina from comment #29)
> I ran the hg command but Warning.png was deleted on my local copy. There are
> warning-16.png warning-64.png and warning-large.png. Did you still want
> those?

Not as part of this bug.

Comment 32

[Dão Gottwald [:dao]]

Comment on attachment 9009227 [details] [diff] [review]
patch.diff

Looks good. Thanks!

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ac9b5094f7e
Use warning.svg instead of Warning.png on Windows. r=dao

Comment 34

[Korina]

Tim and Dao, 

Thank you both so much for helping me with my first bug!

Comment 35

[Dão Gottwald [:dao]]

You're welcome :)

Comment 36

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7ac9b5094f7e