1699586 - De-duplicate twisty/arrow icons listed in allowed-dupes.mn

Status: VERIFIED FIXED

(Toolkit :: Themes, task, P3)

Description

[Tim Nguyen :ntim]

These two are the same, but one is proton style, one is photon style:
toolkit/themes/shared/icons/menu-arrow-left.svg
toolkit/themes/shared/icons/arrow-left.svg

Not quite the same but, there's also:
toolkit/themes/shared/icons/twisty-collapsed-rtl.svg
toolkit/themes/shared/icons/twisty-collapsed.svg

Comment 1

[Itiel]

If possible, I'd like to have separate SVGs for RTL, because this allows to be more versatile, e.g. when using background-image, when currently mirroring the image AND placing it on the correct side is impossible atm.

Comment 2

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

(In reply to Itiel from comment #1)

If possible, I'd like to have separate SVGs for RTL, because this allows to be more versatile, e.g. when using background-image, when currently mirroring the image AND placing it on the correct side is impossible atm.

Yeah we have those assets, and unless we have specific concerns about icons flickering in as they load or something, I would rather use the correct icon vs. flipping it in CSS as a rule.

Comment 3

[:Gijs (he/him)]

(In reply to Tim Nguyen :ntim from comment #0)

These two are the same, but one is proton style, one is photon style:
toolkit/themes/shared/icons/menu-arrow-left.svg
toolkit/themes/shared/icons/arrow-left.svg

menu-arrow-left has been removed, afaict.

Not quite the same but, there's also:
toolkit/themes/shared/icons/twisty-collapsed-rtl.svg
toolkit/themes/shared/icons/twisty-collapsed.svg

The RTL version is in allowed-dupes.mn - I guess it could be reconciled with whatever the other copy is. If we want to maintain useful naming, override directives in jar.mn can help make the same icon available under other names (so that you can still source it as twisty-collapsed-rtl.svg from CSS, but we don't package it twice)

Comment 4

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

Attachment: Bug 1699586 - De-duplicate small down arrow icons, rename as arrow-down-12.svg. r?harry

Comment 5

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

Attachment: Bug 1699586 - De-duplicate small left arrow icons, rename as arrow-left-12.svg. r?harry

Depends on D117571

Comment 6

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

Attachment: Bug 1699586 - De-duplicate small right arrow icons, rename as arrow-right-12.svg r?harry

Depends on D117574

Comment 7

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

Attachment: Bug 1699586 - De-duplicate default down arrow icons, rename as arrow-down.svg. r?harry

Depends on D117575

Comment 8

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

Attachment: Bug 1699586 - De-duplicate default up arrow icons, rename as arrow-up.svg r?harry

Depends on D117576

Comment 9

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

Attachment: Bug 1699586 - Rename default right arrow icon to arrow-right.svg for naming consistency with its other directions. Use it rather than flipping the left arrow. r?itiel

Depends on D117577

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/faedaeb2a10f
De-duplicate small down arrow icons, rename as arrow-down-12.svg. r=harry,preferences-reviewers,desktop-theme-reviewers,thecount,Itiel
https://hg.mozilla.org/integration/autoland/rev/bd33ea428223
De-duplicate small left arrow icons, rename as arrow-left-12.svg. r=harry,desktop-theme-reviewers,Itiel
https://hg.mozilla.org/integration/autoland/rev/4c4d46cb250d
De-duplicate small right arrow icons, rename as arrow-right-12.svg r=harry,desktop-theme-reviewers,thecount,Itiel
https://hg.mozilla.org/integration/autoland/rev/b23c2cdf6055
De-duplicate default down arrow icons, rename as arrow-down.svg. r=harry,desktop-theme-reviewers,Itiel
https://hg.mozilla.org/integration/autoland/rev/2b0292c56e66
De-duplicate default up arrow icons, rename as arrow-up.svg r=harry,desktop-theme-reviewers,Itiel
https://hg.mozilla.org/integration/autoland/rev/aeeaa1d05965
Rename default right arrow icon to arrow-right.svg for naming consistency with its other directions. Use it rather than flipping the left arrow. r=Itiel,desktop-theme-reviewers

Comment 11

[Cristian Brindusan [:cbrindusan]]

Backed out 6 changesets (bug 1699586) for causing bc failures in browser_ext_tabs_hide.js.
https://hg.mozilla.org/integration/autoland/rev/38eeee8d7858faf0436f027b22e520b31515d587

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=aeeaa1d0596516aef70ef5ab3912738ab0c3b715&selectedTaskRun=MgHzxw_HRouDVmzhOrr8QA.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=343556564&repo=autoland&lineNumber=4108

Comment 12

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

(In reply to Cristian Brindusan [:cbrindusan] from comment #11)

Backed out 6 changesets (bug 1699586) for causing bc failures in browser_ext_tabs_hide.js.
https://hg.mozilla.org/integration/autoland/rev/38eeee8d7858faf0436f027b22e520b31515d587

Thanks, looks like that test just expects the old filename and should be an easy fix.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d3c1fe20fe9
De-duplicate small down arrow icons, rename as arrow-down-12.svg. r=harry,preferences-reviewers,desktop-theme-reviewers,thecount,Itiel
https://hg.mozilla.org/integration/autoland/rev/25a77358d2bc
De-duplicate small left arrow icons, rename as arrow-left-12.svg. r=harry,desktop-theme-reviewers,Itiel
https://hg.mozilla.org/integration/autoland/rev/91f70d7ff2b1
De-duplicate small right arrow icons, rename as arrow-right-12.svg r=harry,desktop-theme-reviewers,thecount,Itiel
https://hg.mozilla.org/integration/autoland/rev/621c35b393a2
De-duplicate default down arrow icons, rename as arrow-down.svg. r=harry,desktop-theme-reviewers,Itiel,extension-reviewers,mstriemer
https://hg.mozilla.org/integration/autoland/rev/e492a3c410b1
De-duplicate default up arrow icons, rename as arrow-up.svg r=harry,desktop-theme-reviewers,Itiel
https://hg.mozilla.org/integration/autoland/rev/8d42246a3ee8
Rename default right arrow icon to arrow-right.svg for naming consistency with its other directions. Use it rather than flipping the left arrow. r=Itiel,desktop-theme-reviewers

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/1d3c1fe20fe9
https://hg.mozilla.org/mozilla-central/rev/25a77358d2bc
https://hg.mozilla.org/mozilla-central/rev/91f70d7ff2b1
https://hg.mozilla.org/mozilla-central/rev/621c35b393a2
https://hg.mozilla.org/mozilla-central/rev/e492a3c410b1
https://hg.mozilla.org/mozilla-central/rev/8d42246a3ee8

Comment 15

[Dorel Luca [:dluca]]

Backed out 6 changesets (bug 1699586) for Browser-chrome failures in browser/base/content/test/performance/browser_startup_images.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer?job_id=343676609&repo=autoland&lineNumber=4645

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=8d42246a3ee8b9dcba6662b337278433fe41c769

Backout:
https://hg.mozilla.org/integration/autoland/rev/b819ffa1d645367ff23a5880ea43c3cdddb010ed

Comment 16

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

As I understand it, the test that failed saw that arrow-down-12.svg got loaded, but was never shown. And this just started happening with my patch.
These patches just rename images basically, there shouldn't be any functional changes. I checked, but there are no entries in knownUnshownImages for any of the previous names for this small down arrow icon, so this is a bit odd.

I'll try and narrow down which use of this asset is getting flagged. In the process I just noticed a couple of non-CSS references which I missed, so the backout was probably warranted regardless.

Comment 17

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

I tracked this down to: https://searchfox.org/mozilla-central/source/toolkit/themes/shared/arrowscrollbox.css#17
It pre-loads the image, but never shows it. I confirmed this by adding % override rules to the jar.inc.mn to give a bunch of unique aliases to the image, and updating each reference so it requests one of these unique chrome:// urls.

Unless there's another allow-list using the asset's previous name that I've not found, this seems like a legit perf bug, but not some new regression from my patches. I'll push to try though and do some retriggers before attempting to land again.

Comment 18

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

Try: https://treeherder.mozilla.org/jobs?repo=try&revision=31ef8893f85e108add2e07b1aa05bb29269d937d&selectedTaskRun=en3Ry3hFS5C8Jdgrr6ZDag.0

This shows an apparent permafail: TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_images.js | Loaded image chrome://global/skin/icons/arrow-down-12.svg should have been shown.

I've tried shuffling those selectors around to be really explicit about which image we want for which case:

#scrollbutton-up > .toolbarbutton-icon {
  list-style-image: url("chrome://global/skin/icons/arrow-up-12.svg");
}

#scrollbutton-down > .toolbarbutton-icon {
  list-style-image: url("chrome://global/skin/icons/arrow-dropdown-12.svg");
}

:host([orient="horizontal"]) > #scrollbutton-up:-moz-locale-dir(ltr) > .toolbarbutton-icon,
:host([orient="horizontal"]) > #scrollbutton-down:-moz-locale-dir(rtl) > .toolbarbutton-icon {
  list-style-image: url("chrome://global/skin/icons/arrow-left.svg");
}

:host([orient="horizontal"]) > #scrollbutton-up:-moz-locale-dir(rtl) > .toolbarbutton-icon,
:host([orient="horizontal"]) > #scrollbutton-down:-moz-locale-dir(ltr) > .toolbarbutton-icon {
  list-style-image: url("chrome://global/skin/icons/menu-arrow.svg");
}

For me, I now get a failure: Loaded image chrome://global/skin/icons/menu-arrow.svg should have been shown. It seems its the right arrow on the tab strip which loads but is never shown. That makes sense, and is the kind of thing I might expect this test to catch, but seems bogus - if the transforms to reuse and flip these arrows are all that the test is safe-guarding, it seems counter-productive. Regardless, it's orthogonal to the consolidation and de-duping this bug is focused on so I'll file a follow-up and revert those transform: scale*(-1) changes

Comment 19

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

(In reply to Sam Foster [:sfoster] (he/him) from comment #18)

For me, I now get a failure: Loaded image chrome://global/skin/icons/menu-arrow.svg should have been shown. It seems its the right arrow on the tab strip which loads but is never shown.

In fact: https://searchfox.org/mozilla-central/source/browser/base/content/test/performance/browser_startup_images.js#34, allow-lists the left arrow icon. By this logic, replacing the scaleX(-1) rule with its proper icon should probably add arrow-right.svg to the allow-list. That seems like a plausible option. But perhaps still a distraction from the goal of this bug.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ec2c2980bc0
De-duplicate small down arrow icons, rename as arrow-down-12.svg. r=harry,preferences-reviewers,desktop-theme-reviewers,thecount,Itiel
https://hg.mozilla.org/integration/autoland/rev/05473e77e42d
De-duplicate small left arrow icons, rename as arrow-left-12.svg. r=harry,desktop-theme-reviewers,Itiel
https://hg.mozilla.org/integration/autoland/rev/41cd945ed244
De-duplicate small right arrow icons, rename as arrow-right-12.svg r=harry,desktop-theme-reviewers,thecount,Itiel
https://hg.mozilla.org/integration/autoland/rev/4a51eedcdf0e
De-duplicate default down arrow icons, rename as arrow-down.svg. r=harry,desktop-theme-reviewers,Itiel,extension-reviewers,mstriemer
https://hg.mozilla.org/integration/autoland/rev/f52548e24463
De-duplicate default up arrow icons, rename as arrow-up.svg r=harry,desktop-theme-reviewers,Itiel
https://hg.mozilla.org/integration/autoland/rev/6e75b494338d
Rename default right arrow icon to arrow-right.svg for naming consistency with its other directions. Use it rather than flipping the left arrow. r=Itiel,desktop-theme-reviewers

Comment 21

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

I filed bug 1718159 to track the flipped arrow icons issue in the arrowscrollbox.

QA Notes:

This patch updates icon paths to point to a single canonical icon source. There should be no visible/visual changes. Some notes on which icons changed and where to find them:

about:newTab arrows

• browser/components/newtab/data/content/assets/glyph-arrow.svg
• browser/components/newtab/data/content/assets/glyph-arrowhead-down-12.svg
• browser/components/newtab/data/content/assets/glyph-arrowhead-down-16.svg
• browser/components/newtab/data/content/assets/glyph-caret-right.svg
• browser/components/newtab/data/content/assets/topic-show-more-12.svg

Check about:newtab Personalize menu, "More Recommendations" link in the footer

toolkit/themes/shared/icons/menu-arrow-left.svg

The "back" button in slide-y panels, like the AppMenu panel

"Twisty" arrows

• toolkit/themes/shared/icons/twisty-collapsed.svg
• toolkit/themes/shared/icons/twisty-expanded.svg

This icon is used everywhere we have a XUL tree that has folding and unfolding rows. Here's a list of those places:
Library window
Bookmarks / History sidebar
about:sessionrestore (when choosing which tabs / windows to restore)
about:welcomeback (when choosing which tabs / windows to restore)
Bookmarks Panel (folder selector)
about:preferences (certificate viewer)
about:performance
about:processes
"Disclosure"-type buttons in dialogs (I'm not 100% certain this type is ever used anywhere)

• toolkit/themes/shared/icons/twisty-collapsed-rtl.svg
• toolkit/themes/shared/icons/twisty-expanded-rtl.svg

Same as twisty-collapsed.svg, in RTL builds/configurations

Find arrows

• toolkit/themes/shared/icons/find-next-arrow.svg
• toolkit/themes/shared/icons/find-previous-arrow.svg

The Findbar

toolkit/themes/shared/icons/menu-arrow.svg

Panels (subview navigation button icons)
Context menus (Windows 10), for submenus

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2ec2c2980bc0
https://hg.mozilla.org/mozilla-central/rev/05473e77e42d
https://hg.mozilla.org/mozilla-central/rev/41cd945ed244
https://hg.mozilla.org/mozilla-central/rev/4a51eedcdf0e
https://hg.mozilla.org/mozilla-central/rev/f52548e24463
https://hg.mozilla.org/mozilla-central/rev/6e75b494338d

Comment 23

[Rares Doghi, Desktop QA]

Hi Sam, I tried to verify the fix for this issue but I cant seem to find the arrows from a few sections you mentioned like:

Check about:newtab Personalize menu, "More Recommendations" link in the footer - The only More recommendations link I found was at the bottom of the newtab page and not in the Personalize menu, and the > arrow displayed at the bottom of the page seems to be a part of the More Recommendations link.
I cannot find all of these :
browser/components/newtab/data/content/assets/glyph-arrow.svg
browser/components/newtab/data/content/assets/glyph-arrowhead-down-12.svg
browser/components/newtab/data/content/assets/glyph-arrowhead-down-16.svg
browser/components/newtab/data/content/assets/glyph-caret-right.svg
browser/components/newtab/data/content/assets/topic-show-more-12.svg

about:welcomeback (when choosing which tabs / windows to restore) - I cannot find any arrows here
about:processes - I cannot find any arrows here either
"Disclosure"-type buttons in dialogs (I'm not 100% certain this type is ever used anywhere) - Im not sure where I can find any examples of these buttons, maybe you know where I can find these ?

Comment 24

[Rares Doghi, Desktop QA]

Molly do you have any idea where I can find these icons ? Can you please check Comment 23 ?

Comment 25

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

I cannot find all of these :
browser/components/newtab/data/content/assets/glyph-arrow.svg
browser/components/newtab/data/content/assets/glyph-arrowhead-down-12.svg
browser/components/newtab/data/content/assets/glyph-arrowhead-down-16.svg
browser/components/newtab/data/content/assets/glyph-caret-right.svg
browser/components/newtab/data/content/assets/topic-show-more-12.svg

I noticed some .icon rules and images in about:newtab that seemed to be unused. Perhaps they were used in the past or were there for future plans. Maybe :thecount can clarify on these.

about:welcomeback (when choosing which tabs / windows to restore) - I cannot find any arrows here
IIRC if you restore a session with multiple windows, there's an arrow to expand the list of tabs under each window.

about:processes - I cannot find any arrows here either
Agreed, I think we can skip this one

"Disclosure"-type buttons in dialogs (I'm not 100% certain this type is ever used anywhere) - Im not sure where I can find any examples of these buttons, maybe you know where I can find these ?

Yeah, I'm not sure either. I got this list from mconley's initial work on bug 1699889. I don't know who verified that.

Comment 26

[Scott [:thecount] Downe]

From my investigation, most of the .icon rules are used in context menus, if you set the pref browser.newtabpage.activity-stream.newNewtabExperience.enabled to false.

There are also some of these icons used in about:preferences#home

Attaching screenshots of the icons in use.

Does that account for all the missing icons?

Comment 27

[Scott [:thecount] Downe]

Attachment: icons-1.png

Comment 28

[Scott [:thecount] Downe]

Attachment: icons-2.png

Comment 29

[Molly Howell (she/her) (mostly inactive)]

(looks like the question here has been answered)

Comment 30

[Rares Doghi, Desktop QA]

Yes, I can Confirm this Issue Verified as Fixed, except theres still the Disclosure Type Buttons I cant seem to find and bug 1699889 does not seem to have any information on these either.

Mike ? Molly ? are there any disclosure buttons I could take a look at ? are these used anywhere in Firefox ?

Comment 31

[Molly Howell (she/her) (mostly inactive)]

If you mean the "twisty" arrows, it looks like those are used directly in about:processes and about:performance, as well as the Synced Tabs sidebar. That's probably not all, but the rest are a bit less easy for me to find.

Comment 32

[Rares Doghi, Desktop QA]

Based on all these comments I can Confirm this issue Verified as Fixed in our latest Nightly build 91.0a1 (2021-07-07) on Mac, Windows and Ubuntu.