618324 - Apply a filter to extension icons that makes them monochrome on Mac

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect)

Description

[Markus Stange [:mstange]]

Attachment: screenshot

See bug 616472 for background discussion.

Comment 1

[Markus Stange [:mstange]]

Attachment: wip

This seems to work ok, except for the fact that it makes filtered icons in the customization panel invisible...

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 496818 [details]
screenshot

ok, this turned out *way* better than I expected it to.  You guys were right, I was wrong :)  I think we should go ahead and land this if we can.  For the most part it looks like it will work, there are however a few icons that are unintelligible (like the one in between the seahorse and the downthemall arrow), so we should provide developers a way to opt out if they choose to.

Comment 3

[Markus Stange [:mstange]]

Attachment: v1

I've used haspinstripeartwork="true" for the turn-off attribute name, but I'm open to better suggestions :)

Comment 4

[Unknown W. Brackets]

(In reply to comment #3)
> Created attachment 497262 [details] [diff] [review]
> v1
> 
> I've used haspinstripeartwork="true" for the turn-off attribute name, but I'm
> open to better suggestions :)

If it were me, I'd go for more generic - like originalartwork="true".  As a developer, if there were more automagical effects like this applied to my buttons, I'd either want them on or off - not have to pick and choose each one.

That's just me.  Kinda like how when you start styling an input in HTML, it goes to 1990's mode.

-[Unknown]

Comment 5

[Giorgio Maone [:ma1]]

IMHO it should be opt-in, especially if at it seems (look at the ABP icon) it strips off any antialias making most icons messy looking.

Attribute name should be obvious and self-descriptive, rather than referencing specific themes. Something like mononochrome="true" or multicolor="false".

Comment 6

[Mike Kaply [:mkaply]]

I think the Firefox folks want optout because they don't want color icons to disturb the look of Mac...

Comment 7

[Nils Maier [:nmaier]]

(In reply to comment #6)
> I think the Firefox folks want optout because they don't want color icons to
> disturb the look of Mac...

A forced (opt-out) filter looks far worse than some colors in many instances; off the 10 add-on icons in the screenshot only 3-4 look reasonably well compared to the originals IMO
On top of that this sudden change will only confuse authors about why their icons look very different all of a sudden.
I'd even say: WONTFIX this bug and let add-on authors provide specific mac-aware icons. Just amend the icon-related articles on devmo. amo-editors can encourage authors to use monochrome icons for mac.

And since when are mac apps all-mononchrome? Just because Safari and other Apple apps have monochrome icons doesn't mean everybody else has to use monochrome icons, too...

Comment 8

[Giorgio Maone [:ma1]]

(In reply to comment #7)

> And since when are mac apps all-mononchrome? Just because Safari and other
> Apple apps have monochrome icons doesn't mean everybody else has to use
> monochrome icons, too

Not to mention some extensions (ABP, NoScript, CookieSafe, FoxyProxy, just to name a few) use color semantically to communicate status information. They would become not just ugly, but broken.

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

I agree that it should be opt-in. Add-on authors will not expect this, and many don't test frequently (or at all) on Mac to realize what's going on. Making a last-minute change that affects so many add-ons is not a good idea.

Comment 10

[Markus Stange [:mstange]]

Well, part of the system is already opt-in: You have to specify the toolbarbutton-1 class to get the button shape in the first place. The monochrome filter will only be applied to toolbarbutton-1 buttons.

(In reply to comment #7)
> And since when are mac apps all-mononchrome? Just because Safari and other
> Apple apps have monochrome icons doesn't mean everybody else has to use
> monochrome icons, too...

Monochrome-ness (monochromocity?) is part of the button style we're using, and if we want to look like a Mac app, forcing the glyphs to be monochrome makes a lot of sense. Those Mac apps that use colored icons in their toolbars don't use a button; they put the icon directly on the toolbar instead. Which, incidentally, is exactly the appearance that extension icons get when they don't set class="toolbarbutton-1".

(In reply to comment #5)
> Attribute name should be obvious and self-descriptive, rather than referencing
> specific themes.

The reason I thought having "pinstripe" in the attribute name was a good idea was that pinstripe is the only theme that applies the filter.

Comment 11

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to comment #10)
> Well, part of the system is already opt-in: You have to specify the
> toolbarbutton-1 class to get the button shape in the first place. The
> monochrome filter will only be applied to toolbarbutton-1 buttons.

That class is pretty standard for toolbar buttons done right. It's the recommendation in almost every tutorial I've seen, and buttons won't look right without it. Saying that this is some form of opt-in is disingenuous at best.

Comment 12

[Markus Stange [:mstange]]

(In reply to comment #11)
> and buttons won't look right without it.

Depending on how you define "right", right?

Faaborg: Would you be happy with an opt-in solution, too?

Comment 13

[:Paolo Amadini]

Comment on attachment 497262 [details] [diff] [review]
v1

I also believe that an opt-in solution is better, not all add-on icons are usable when filtered to monochrome. This would also work well with bug 616472.

Personally, I think a class is more suited for opting in to a styling feature. For example, if an add-on does not have a dedicated monochrome icon and wants the Mac theme (or a custom theme) to generate it, it could use a declaration similar to this one:

<toolbarbutton class="chromeclass-toolbar-additional
                      toolbarbutton-1
                      toolbarbutton-generate-monochrome">

>+++ b/browser/base/content/browser.xul
>+# Adds <svg:filter id="pinstripe-monochrome-icon-filter">:
>+#include ../../../toolkit/themes/pinstripe/global/monochrome-icon-filter.svg

If this is not needed for performance reasons, I'd simply keep the filter in a separate file.

>+++ b/browser/themes/pinstripe/browser/browser.css
>+@-moz-document url-prefix(chrome://global/) {
>+  /* The toolbar customization panel document lives in chrome://global/ and
>+   * can't reference the filter in browser.xul. */
>+  .toolbarbutton-1:not([haspinstripeartwork="true"]):not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-icon,
>+  .toolbarbutton-1:not([haspinstripeartwork="true"]) > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
>+    filter: url(chrome://global/skin/monochrome-icon-filter.svg#pinstripe-monochrome-icon-filter);
>+  } 
>+}

It seems that the issue here is with loading the filter from a different XUL file instead of an SVG file. I'd simply include the separate filter file in "browser/content". The following rule works for me both on the browser window and on the customization panel:

.toolbarbutton-generate-monochrome > .toolbarbutton-icon,
.toolbarbutton-generate-monochrome > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
  filter: url(chrome://browser/content/monochrome-icon-filter.svg#pinstripe-monochrome-icon-filter);
}

Comment 14

[Markus Stange [:mstange]]

(In reply to comment #13)
> >+++ b/browser/base/content/browser.xul
> >+# Adds <svg:filter id="pinstripe-monochrome-icon-filter">:
> >+#include ../../../toolkit/themes/pinstripe/global/monochrome-icon-filter.svg
> 
> If this is not needed for performance reasons, I'd simply keep the filter in a
> separate file.

It's needed for "blinking" reasons: if the filter is in a separate file, it won't be loaded before the first paint of the browser window, so when the window is shown the filtered icons will be invisible at first and then appear a moment later.

Comment 15

[:Paolo Amadini]

(In reply to comment #14)
> (In reply to comment #13)
> > >+++ b/browser/base/content/browser.xul
> > >+# Adds <svg:filter id="pinstripe-monochrome-icon-filter">:
> > >+#include ../../../toolkit/themes/pinstripe/global/monochrome-icon-filter.svg
> > 
> > If this is not needed for performance reasons, I'd simply keep the filter in a
> > separate file.
> 
> It's needed for "blinking" reasons: if the filter is in a separate file, it
> won't be loaded before the first paint of the browser window, so when the
> window is shown the filtered icons will be invisible at first and then appear a
> moment later.

Good, so if I understand correctly the only remaining change is to move the
separate file from toolkit to browser and apply the filter only to icons in
buttons with the "toolbarbutton-generate-monochrome" class.

Markus, do you think you can do this during the next week? It's the last
week available to try to get this change in the last Firefox 4 beta. This
change is less important than bug 626382, because custom themes or extensions
could include a monochrome filter themselves if they want, but it would still
be a nice addition.

Comment 16

[Markus Stange [:mstange]]

Attachment: v2

Comment 17

[Markus Stange [:mstange]]

This version is off by default and uses the "toolbarbutton-generate-monochrome" class as opt-in.

Comment 18

[:Paolo Amadini]

Comment on attachment 506092 [details] [diff] [review]
v2

Looks great! Thank you. One nit:

>+++ b/browser/themes/pinstripe/browser/browser.css
>+.toolbarbutton-generate-monochrome:not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-icon,
>+  .toolbarbutton-generate-monochrome:not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-icon,

Since there is no default button with the toolbarbutton-generate-monochrome class, the ":not" selectors can be omitted.

Comment 19

[Markus Stange [:mstange]]

Attachment: v3

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 506189 [details] [diff] [review]
v3

This would now need to be made Lion-aware.

I don't really like "toolbarbutton-generate-monochrome", but can't think of a better name. (It may or may not make sense to pick a name that indicates that the theme may apply whatever effect it wants, not just monochrome.)

Last but not least, I'm not sure this is worth it at all. We can't really encourage the use of the class, given the somewhat poor visual quality in many cases.

Comment 21

[Markus Stange [:mstange]]

Closing out this really old bug. I don't think we want this.