Closed Bug 1457781 Opened 2 years ago Closed 2 years ago

Use warning.svg instead of Warning.png on Windows

Categories

(Toolkit :: Themes, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: ntim, Assigned: korina.houghtaling)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 6 obsolete files)

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
Priority: -- → P3
Assignee: nobody → 1991manish.kumar
> '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.
Flags: needinfo?(ntim.bugs)
Attached patch Patch_Bug1457781 (obsolete) — Splinter Review
Please review
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(ntim.bugs)
Attachment #8985542 - Flags: review?(dao+bmo)
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
Attachment #8985542 - Flags: review?(dao+bmo) → review-
Assignee: 1991manish.kumar → nobody
Keywords: good-first-bug
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.
Okay, let me know if you have questions.
Assignee: nobody → korina.houghtaling
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
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
Attached patch mypatch.diff (obsolete) — Splinter Review
Attachment #9007647 - Flags: review+
Attachment #9007647 - Flags: feedback+
(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 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...
Attachment #9007647 - Flags: review+
Attachment #9007647 - Flags: feedback+
Attachment #8985542 - Attachment is obsolete: true
(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?
Thank you for pointing out that sslWarning line- I've fixed it.
(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.
(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
(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.
Attached patch mypatch.diff (obsolete) — Splinter Review
Attachment #9008836 - Flags: review+
Attachment #9008836 - Flags: feedback+
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.
Attachment #9008836 - Flags: review+
Attachment #9008836 - Flags: feedback+
Attachment #9007647 - Attachment is obsolete: true
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.
Attached patch mypatch2.diff (obsolete) — Splinter Review
Attachment #9008844 - Flags: review+
Attachment #9008844 - Flags: feedback+
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 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.
Attachment #9008836 - Attachment is obsolete: true
Attached patch mypatch3.diff (obsolete) — Splinter Review
This patch has the discussed changes and is saved outside of the mozilla-source directory.
Attachment #9008867 - Flags: review+
Attachment #9008867 - Flags: feedback+
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.
Attachment #9008867 - Flags: review+
Attachment #9008867 - Flags: feedback+
Attachment #9008844 - Attachment is obsolete: true
Attached patch mypatch.diff (obsolete) — Splinter Review
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.
Attachment #9008984 - Flags: review+
Attachment #9008984 - Flags: feedback+
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
Attachment #9008984 - Flags: review+
Attachment #9008984 - Flags: feedback+
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?
Attached patch patch.diffSplinter Review
Removed warning.png and removed trailing whitespace
Attachment #9009227 - Flags: review+
Attachment #9009227 - Flags: feedback+
(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 on attachment 9009227 [details] [diff] [review]
patch.diff

Looks good. Thanks!
Attachment #9009227 - Flags: review+
Attachment #9009227 - Flags: feedback+
Attachment #9008984 - Attachment is obsolete: true
Attachment #9008867 - Attachment is obsolete: true
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ac9b5094f7e
Use warning.svg instead of Warning.png on Windows. r=dao
Tim and Dao, 

Thank you both so much for helping me with my first bug!
You're welcome :)
https://hg.mozilla.org/mozilla-central/rev/7ac9b5094f7e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1493412
You need to log in before you can comment on or make changes to this bug.