Closed Bug 1739139 Opened 8 months ago Closed 3 months ago

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)

Firefox 95
task

Tracking

()

RESOLVED FIXED
101 Branch
Tracking Status
firefox101 --- fixed

People

(Reporter: sirrichi98, Assigned: manisha.singh2019, 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.

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.

Component: Untriaged → New Tab Page

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true

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

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.

Flags: needinfo?(sfoster)

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.

Flags: needinfo?(sfoster)
Severity: -- → S3
Priority: -- → P3
Mentor: dharvey
Keywords: good-first-bug

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

Flags: needinfo?(dharvey)

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

Flags: needinfo?(achurchwell)

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.

Type: enhancement → task
Flags: needinfo?(dharvey)
Flags: needinfo?(achurchwell)
Summary: update new listed icons in browser/component → Move the topsites icon used by about:newtab to the shared icon library in browser/themes/shared/icons
Mentor: dharvey → sfoster
Assignee: nobody → manisha.singh2019
Status: NEW → ASSIGNED
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae91be7a2cca
Move and rename the topsite icon. r=sfoster,desktop-theme-reviewers
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 101 Branch

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.

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

Regressions: 1766070
You need to log in before you can comment on or make changes to this bug.