1739139 - Move the topsites icon used by about:newtab to the shared icon library in browser/themes/shared/icons

Status: RESOLVED FIXED

(Firefox :: New Tab Page, task, P3)

Description

[sirri]

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

[BugBot [:suhaib / :marco/ :calixte]]

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

[Alin Ilea, Desktop QA]

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.

Comment 3

[sirri]

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

[Amy Churchwell [:amy]]

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

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

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.

Comment 6

[Anshuman Singh]

I would like to work on this issue.If it is available.

Comment 7

[manisha270417]

I would like to work on this bug, Please provide me a clear description of the task that needs to be done here.

Comment 8

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

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.

Comment 9

[manisha270417]

Attachment: Bug 1739139 - Move and rename the topsite icon. r?sfoster

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae91be7a2cca
Move and rename the topsite icon. r=sfoster,desktop-theme-reviewers

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ae91be7a2cca

Comment 12

[Ed Lee :Mardak]

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

[Ed Lee :Mardak]

Oh. It's about:preferences#home that's trying to load the image. Notice the missing icon next to "Shortcuts"