Closed Bug 1367432 Opened 3 years ago Closed 3 years ago

Move menu-icons and sidebars icons to browser/themes/shared/icons/ and deduplicate them

Categories

(Firefox :: Theme, enhancement, P1)

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.6 - May 29
Tracking Status
firefox55 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(2 files)

I ran into this in bug 1354126 - the dropdown arrow for the sidebar that is used elsewhere lived in the sidebar folder. It probably makes sense to put all the icons in the same folder. We also originally added the menu-icons folder for the icons in the menu, but those should probably move into the icons/ folder, too.
Flags: qe-verify-
Whiteboard: [photon-visual] → [photon-visual] [triage]
Whiteboard: [photon-visual] [triage] → [reserve-photon-visual]
Note that these patches make the icons used in the sidebar and panel menu match the current toolbar icons, with the exception of the back button in the menu panel, where the other back*.svg files looked horrible. I assume we will just update everything else at once when we do an icon drop (bug 1355455).
Comment on attachment 8871265 [details]
Bug 1367432 - move and de-duplicate menu panel icons,

https://reviewboard.mozilla.org/r/142760/#review146860

::: browser/themes/shared/customizableui/panelUI.inc.css:1247
(Diff revision 1)
>    color: GrayText;
>  }
>  
>  .PanelUI-subView .subviewbutton-nav::after {
>    -moz-context-properties: fill;
> -  content: url(chrome://browser/skin/menu-icons/back-small.svg);
> +  content: url(chrome://browser/skin/back-small.svg);

Hmm, "small" will probably mean different things for different use cases. Better call this back-12.svg or so.

::: browser/themes/shared/customizableui/panelUI.inc.css:1307
(Diff revision 1)
>    fill: currentColor;
>  }
>  
>  photonpanelmultiview .subviewbutton[checked="true"] {
>    background: none;
> -  list-style-image: url(chrome://browser/skin/menu-icons/check.svg);
> +  list-style-image: url(chrome://browser/skin/check.svg);

Looks like you should set a fill color here.

::: browser/themes/shared/customizableui/panelUI.inc.css:2054
(Diff revision 1)
>  }
>  
>  photonpanelmultiview .PanelUI-subView .panel-header > .subviewbutton-back {
>    -moz-context-properties: fill;
>    fill: var(--arrowpanel-color);
> -  list-style-image: url(chrome://browser/skin/menu-icons/back.svg);
> +  list-style-image: url(chrome://browser/skin/back-menupanel.svg);

browser/themes/shared/icons/ is for generic, reusable icons. The name back-menupanel.svg suggests that this icon doesn't really belong there.
Attachment #8871265 - Flags: review?(dao+bmo) → review-
Comment on attachment 8871266 [details]
Bug 1367432 - move and de-duplicate sidebar icons,

https://reviewboard.mozilla.org/r/142762/#review146864

::: browser/themes/shared/jar.inc.mn:143
(Diff revision 1)
>    skin/classic/browser/privateBrowsing.svg            (../shared/icons/privateBrowsing.svg)
>    skin/classic/browser/reload.svg                     (../shared/icons/reload.svg)
>    skin/classic/browser/save.svg                       (../shared/icons/save.svg)
>    skin/classic/browser/settings.svg                   (../shared/icons/settings.svg)
>    skin/classic/browser/share.svg                      (../shared/icons/share.svg)
> +  skin/classic/browser/sidebar-close.svg              (../shared/icons/sidebar-close.svg)

Again, this doesn't seem to belong here. This is a consequence of the sidebar pushing ahead with its own style rather than using the close icon used everywhere else :/
Attachment #8871266 - Flags: review?(dao+bmo) → review-
Can you suggest alternative locations for the singular menu panel and sidebar icons, then? Because keeping menu-icons/sidebar folders for a single icon also seems silly.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #4)
> ::: browser/themes/shared/customizableui/panelUI.inc.css:1307
> (Diff revision 1)
> >    fill: currentColor;
> >  }
> >  
> >  photonpanelmultiview .subviewbutton[checked="true"] {
> >    background: none;
> > -  list-style-image: url(chrome://browser/skin/menu-icons/check.svg);
> > +  list-style-image: url(chrome://browser/skin/check.svg);
> 
> Looks like you should set a fill color here.

It works fine as-is, and I'm just moving the icon, so I don't think I should mess with that here.
(In reply to :Gijs from comment #6)
> Can you suggest alternative locations for the singular menu panel and
> sidebar icons, then? Because keeping menu-icons/sidebar folders for a single
> icon also seems silly.

back-menupanel.svg should probably just have a different name to better differentiate it from back.svg. Maybe something like scroll-left.svg? arrow-left.svg (probably not)? I believe there's also a dropdown arrow somewhere using a similar style. Might make sense to consolidate the naming scheme.

The solution for sidebar-close.svg should be to restore consistency for close icons. I'd suggest keeping the sidebar folder for now.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #8)
> (In reply to :Gijs from comment #6)
> > Can you suggest alternative locations for the singular menu panel and
> > sidebar icons, then? Because keeping menu-icons/sidebar folders for a single
> > icon also seems silly.
> 
> back-menupanel.svg should probably just have a different name to better
> differentiate it from back.svg. Maybe something like scroll-left.svg?
> arrow-left.svg (probably not)? I believe there's also a dropdown arrow
> somewhere using a similar style. Might make sense to consolidate the naming
> scheme.

Seems like you added this recently: browser/themes/shared/icons/arrow-dropdown.svg
Comment on attachment 8871265 [details]
Bug 1367432 - move and de-duplicate menu panel icons,

https://reviewboard.mozilla.org/r/142760/#review147214

::: browser/themes/shared/customizableui/panelUI.inc.css:2054
(Diff revision 2)
>  }
>  
>  photonpanelmultiview .PanelUI-subView .panel-header > .subviewbutton-back {
>    -moz-context-properties: fill;
>    fill: var(--arrowpanel-color);
> -  list-style-image: url(chrome://browser/skin/menu-icons/back.svg);
> +  list-style-image: url(chrome://browser/skin/arrow-left.svg);

We may later find that "arrow-left" is too generic, but I think it's fine for now...
Attachment #8871265 - Flags: review?(dao+bmo) → review+
Comment on attachment 8871266 [details]
Bug 1367432 - move and de-duplicate sidebar icons,

https://reviewboard.mozilla.org/r/142762/#review147216
Attachment #8871266 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50ec456995cf
move and de-duplicate menu panel icons, r=dao
https://hg.mozilla.org/integration/autoland/rev/f08aa17062e8
move and de-duplicate sidebar icons, r=dao
https://hg.mozilla.org/mozilla-central/rev/50ec456995cf
https://hg.mozilla.org/mozilla-central/rev/f08aa17062e8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.6 - May 29
You need to log in before you can comment on or make changes to this bug.