1698030 - De-duplicate warning.svg icons

Status: VERIFIED FIXED

(Toolkit :: Themes, task, P5)

Description

[Tim Nguyen :ntim]

toolkit/themes/shared/icons/warning.svg
browser/themes/shared/warning.svg
browser/themes/shared/controlcenter/warning.svg

All these 3 are more-or-less the same icon.

The browser ones have a configurable stroke, where toolkit one is a cut out.

Comment 1

[Tim Nguyen :ntim]

DevTools has its own versions too:
devtools/client/themes/images/alert-small.svg
devtools/client/themes/images/alert-tiny.svg
devtools/client/themes/images/alert.svg

Also, it's worth mentioning browser/themes/shared/warning.svg is used as app menu badge, if a different icon is still needed there, worth renaming it to badge-warning.svg or whatever convention is used for the update badge.

Comment 2

[Tim Nguyen :ntim]

I recommend keeping only the cut out version (toolkit/themes/shared/icons/warning.svg), since that's what proton is going to be using.

Comment 3

[Molly Howell (she/her) (no longer active)]

browser/themes/shared/controlcenter/warning.svg has already been removed in bug 1707086.

The devtools clones were intentionally created in bug 1699858; Proton didn't apply to devtools, but we updated some of the shared icons that devtools was using, so copies of those were made to prevent having two different icon styles. I think that rationale stills applies, so I don't think those could be easily deduplicated at the moment.

That leaves toolkit/themes/shared/icons/warning.svg and browser/themes/shared/warning.svg, which do appear to be identical except that the one in browser/ has colors in it and the one in toolkit/ does not; not an especially meaningful difference. Sam, do you know of a particular reason why both of these are still here, or can one just be removed?

Comment 4

[Sam Foster [:sfoster] (he/him)]

(In reply to Molly Howell (she/her) [:mhowell] from comment #3)

That leaves toolkit/themes/shared/icons/warning.svg and browser/themes/shared/warning.svg, which do appear to be identical except that the one in browser/ has colors in it and the one in toolkit/ does not; not an especially meaningful difference. Sam, do you know of a particular reason why both of these are still here, or can one just be removed?

I don't know a specific reason. Ideally we always want the fill color to come from the CSS with these icons, but the difference between the browser and toolkit variations is that the browser one has a fallback color hardcoded in there. Its possible that is there because it was difficult to target with a CSS rule in some of its uses?

So, ideally we'd remove browser/themes/shared/warning.svg and update all references to it with toolkit/themes/shared/warning.svg, but we have to be super careful that each of those references provides an appropriate context-fill color to avoid regressions.

Comment 5

[Molly Howell (she/her) (no longer active)]

Okay, thanks for the info. I think I'll give it a go, and just be very careful that there's always a color present, like you said.

Comment 6

[Molly Howell (she/her) (no longer active)]

Attachment: Bug 1698030 - Remove a redundant warning icon. r=sfoster

The only difference between the icon that was removed and the one kept is that
the removed one has a default fill color in the SVG. This meant everywhere the
icon was replaced, we have to make sure that a fill color is defined in CSS.
In a few cases, that necessitated adding a new class. In a few others, colors
were already being defined for the icon, so there was no need to add any here.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a5e93469ad9
Remove a redundant warning icon. r=preferences-reviewers,Gijs

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5a5e93469ad9

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7c177be697c6
Port bug 1698030 - Remove a redundant warning icon. rs=bustage-fix

Comment 10

[Peter Magyari (Desktop QA)]

Verified using Nightly 92.0a1 (20210729093615) on Windows, MacOS and Ubuntu.