Move the topsites icon used by about:newtab to the shared icon library in browser/themes/shared/icons
Categories
(Firefox :: New Tab Page, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox101 | --- | fixed |
People
(Reporter: sirrichi98, Assigned: manisha270417, Mentored)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file)
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/95.0.4638.69 Safari/537.36
Steps to reproduce:
From Update icons on about:newtab r=sfoster,mardak.
https://phabricator.services.mozilla.com/D113687
Actual results:
The folder browser/components/newtab/data/content/assets
still contains icons that do not correspond to the existing icons from the Proton Icons list.
Expected results:
Icons could be renamed accordingly https://docs.google.com/spreadsheets/d/19RmW8kEoNkhyiZvQNV9hG0rIkhD4tQIMtVuKDZXoLTQ/edit#gid=0 and tested.
Comment 1•3 years ago
|
||
The Bugbug bot thinks this bug should belong to the 'Firefox::New Tab Page' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.
Comment 2•3 years ago
|
||
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:94.0) Gecko/20100101 Firefox/94.0
Hi,
Thank you for opening this enhancement. I will set this as New and waiting for the developers opinion about it.
Thanks for your report.
The exact icons to look for would be
browser/components/newtab/data/content/assets/glyph-topsites-16.svg
browser/components/newtab/data/content/assets/glyph-trending-16.svg
browser/components/newtab/data/content/assets/glyph-pin-12.svg
,
browser/components/newtab/data/content/assets/glyph-cancel-16.svg
browser/components/newtab/data/content/assets/glyph-edit-16.svg
Comment 4•3 years ago
|
||
Hi Sam, is this part of a larger ongoing project? Not familiar with the spreadsheet that is linked, not sure if someone is working on this.
Comment 5•3 years ago
|
||
We did have a project to update a lot of icons for Proton/Fx89. We tried to consolidate cases of duplicates / near duplicates / should-really-be-the-same-icon before updating them, and I think that's the context here. It looks like these had their contents updated in Bug 1703027. In a lot of cases we had multiple uses for icons so we favored placing/updating the new icon in browser/themes/shared/icons or toolkit/themes/shared/icons and having everyone reference the same icon from one canonical path. As there weren't other uses of these icons at the time, they were updated in place in the new tab assets directory. That's a little surprising looking at that list - these all look like potentially common icons and I can think of a couple of places where they should be used that bear investigating.
We decided against a mass rename to enforce a naming convention on these icons, instead keeping existing names and only renaming to resolve ambiguity or make them more reusable. That's probably what I would do here - move the icons to browser/themes/shared/icons, and rename them. As 16x16 is our default icon size, we drop the -16 from the filename. We also have filled and line/outlined variations of a lot of icons, and we decided the outline would be the default state where both exist, so you get iconname.svg and iconname-fill.svg, iconname-12.svg, iconname-fill-12.svg etc.
Updated•3 years ago
|
Updated•3 years ago
|
Comment 6•3 years ago
|
||
I would like to work on this issue.If it is available.
Assignee | ||
Comment 7•3 years ago
|
||
I would like to work on this bug, Please provide me a clear description of the task that needs to be done here.
Assignee | ||
Updated•3 years ago
|
Comment 8•3 years ago
•
|
||
There are several parts to this that I think are best handled in individual bugs/patches. So I'm going to morph this one to be specifically about the glyph-topsites-16.svg icon used by newtab:
browser/components/newtab/data/content/assets/glyph-topsites-16.svg
should be moved to browser/themes/shared/icons
and named topsites.svg
.
- please use
hg mv
or its git equivalent to move the file so we retain commit history. - You will need to update the
browser/themes/shared/jar.inc.mn
manifest (please maintain alphabetical sorting) to include the new .svg file - you'll need to find and update all references to point at
chrome://browser/skin/topsites.svg
- See the docs for hacking on newtab for details on how to edit CSS and refresh the bundle
You should always build and run with your changes to verify them. In this case I don't believe there is an easy way to see the .icon-topsites
class in use, but you can check that chrome://browser/skin/topsites.svg
resolve to the correct icon when entered into the url bar.
Updated•3 years ago
|
Assignee | ||
Comment 9•3 years ago
|
||
Updated•3 years ago
|
Comment 10•3 years ago
|
||
Comment 11•3 years ago
|
||
bugherder |
Comment 12•3 years ago
|
||
Hmm… odd? I was running some tests on a clean mozilla-central and had some test failures:
./mach test browser_policy_set_homepage.js
…
Mozilla crash reason: Missing chrome or resource URLs: chrome://activity-stream/content/data/content/assets/glyph-topsites-16.svg
…
browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js
CRASH browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js - application terminated with exit code 0
Doing a ./mach clobber
doesn't seem to help. When running the build and opening chrome://activity-stream/content/css/activity-stream.css
, I do see it referencing .icon.icon-topsites { background-image: url("chrome://browser/skin/topsites.svg");
I'll just skip this test for now.
Comment 13•3 years ago
|
||
Oh. It's about:preferences#home that's trying to load the image. Notice the missing icon next to "Shortcuts"
Description
•