Closed Bug 1698030 Opened 9 months ago Closed 4 months ago

De-duplicate warning.svg icons

Categories

(Toolkit :: Themes, task, P5)

task
Points:
1

Tracking

()

VERIFIED FIXED
92 Branch
Tracking Status
firefox92 --- verified

People

(Reporter: ntim, Assigned: mhowell)

References

(Blocks 1 open bug)

Details

(Whiteboard: [proton-icons] [proton-cleanups])

Attachments

(1 file)

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.

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.

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

Blocks: proton-icons
Whiteboard: [proton-icons]
Priority: -- → P5
Whiteboard: [proton-icons] → [proton-icons] [proton-cleanups]

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?

Flags: needinfo?(sfoster)

(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.

Flags: needinfo?(sfoster)

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.

Assignee: nobody → mhowell
Status: NEW → ASSIGNED
Points: --- → 1

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.

Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a5e93469ad9
Remove a redundant warning icon. r=preferences-reviewers,Gijs
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/7c177be697c6
Port bug 1698030 - Remove a redundant warning icon. rs=bustage-fix

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

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.