Use warning.svg instead of Warning.png on Windows

RESOLVED FIXED in Firefox 64

Status

()

P3
normal
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: ntim, Assigned: korina.houghtaling)

Tracking

(Blocks: 1 bug, {good-first-bug})

unspecified
mozilla64
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

11 months ago
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

Updated

11 months ago
Priority: -- → P3
Assignee: nobody → 1991manish.kumar
(Reporter)

Comment 3

9 months ago
> '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)
Posted patch Patch_Bug1457781 (obsolete) — Splinter Review
Please review
Flags: needinfo?(ntim.bugs)
(Reporter)

Updated

9 months ago
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
(Assignee)

Comment 6

7 months ago
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
(Assignee)

Comment 8

7 months ago
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
(Assignee)

Comment 9

7 months ago
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
(Assignee)

Comment 10

7 months ago
Posted 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
(Assignee)

Comment 13

7 months ago
(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?
(Assignee)

Comment 14

7 months ago
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.
(Assignee)

Comment 16

7 months ago
(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
(Assignee)

Comment 18

6 months ago
(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.
(Assignee)

Comment 19

6 months ago
Posted 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
(Assignee)

Comment 21

6 months ago
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.
(Assignee)

Comment 22

6 months ago
Posted 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
(Assignee)

Comment 25

6 months ago
Posted 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+
(Reporter)

Comment 26

6 months ago
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+
(Reporter)

Updated

6 months ago
Attachment #9008844 - Attachment is obsolete: true
(Assignee)

Comment 27

6 months ago
Posted 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+
(Assignee)

Comment 29

6 months ago
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?
(Assignee)

Comment 30

6 months ago
Posted 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

Comment 33

6 months ago
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
(Assignee)

Comment 34

6 months ago
Tim and Dao, 

Thank you both so much for helping me with my first bug!
You're welcome :)

Comment 36

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7ac9b5094f7e
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1493412
You need to log in before you can comment on or make changes to this bug.