See https://bugzilla.mozilla.org/show_bug.cgi?id=1329242#c15

Calling out that this bug would be required to let Screenshots handle state changes to the icon coming from the browserAction.setIcon method gracefully for users in both light and dark themes. It also seems like a more elegant way to handle icon color in SVG icons since it future proofs icons against possible changes to theme colors.

Comment on attachment 8884081 [details] Bug 1377302 - Support fill='context-fill' for WebExtension browser-actions. https://reviewboard.mozilla.org/r/155014/#review160062

wow, that was easier than i thought!

Comment on attachment 8884081 [details] Bug 1377302 - Support fill='context-fill' for WebExtension browser-actions. https://reviewboard.mozilla.org/r/155014/#review160068 ::: browser/themes/shared/toolbarbutton-icons.inc.css:12 (Diff revision 1) > -:-moz-any(@primaryToolbarButtons@) { > +.toolbarbutton-1 { > -moz-context-properties: fill; > fill: var(--toolbarbutton-icon-fill); > } > > toolbar[brighttext] :-moz-any(@primaryToolbarButtons@) { Should :-moz-any(@primaryToolbarButtons@ be changed here also?

Comment on attachment 8884081 [details] Bug 1377302 - Support fill='context-fill' for WebExtension browser-actions. https://reviewboard.mozilla.org/r/155014/#review160070 ::: browser/themes/shared/toolbarbutton-icons.inc.css:12 (Diff revision 1) > -:-moz-any(@primaryToolbarButtons@) { > +.toolbarbutton-1 { > -moz-context-properties: fill; > fill: var(--toolbarbutton-icon-fill); > } > > toolbar[brighttext] :-moz-any(@primaryToolbarButtons@) { Yes, thanks for catching that.

I think screenshots will also need this uplifting to beta, so please remember to ask for that.

Andym... Screenshots 10.3 is in Beta currently. I'll open a bug with a patch to do a dot bump with new icons icons and we will request uplift for a version when i can test against this patch to ensure we're good with color.

(In reply to [:jgruen] from comment #9) > Andym... Screenshots 10.3 is in Beta currently. Yeah so this patch will need uplifting if it works :)

Andym, yes! I'm pretty sure i wrote my last comment with one foot in another meeting and one foot out the door :)

Comment on attachment 8884081 [details] Bug 1377302 - Support fill='context-fill' for WebExtension browser-actions. https://reviewboard.mozilla.org/r/155014/#review160190

Pushed by mwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/657ef58ae035 Support fill='context-fill' for WebExtension browser-actions. r=dao,mattw

[Tracking Requested - why for this release]: Screenshots needs this to allow for their toolbar icon to match the default toolbar icons that ship in Firefox when using Firefox with a dark theme. Screenshots is shipping in v55.

If "svg.context-properties.content.enabled" is set to false (default), the icon is allways black.

(In reply to fx4waldi from comment #16) > If "svg.context-properties.content.enabled" is set to false (default), the > icon is allways black. When loaded in a content process, yes. This bug is about toolbar button icons.

(In reply to Dão Gottwald [::dao] from comment #17) > (In reply to fx4waldi from comment #16) > > If "svg.context-properties.content.enabled" is set to false (default), the > > icon is allways black. > > When loaded in a content process, yes. This bug is about toolbar button > icons. The problem occurs on the toolbar buttons added by the webExtension. I tested on a clean profile in Windows 10.

(In reply to fx4waldi from comment #16) > If "svg.context-properties.content.enabled" is set to false (default), the > icon is allways black. Can confirm after testing on Screenshots. Flipping the pref to true seems to resolve the issue, but it is not resolved by default. I'm not sure what side effects defaulting to true would have globally. I made a demo webextension to try to completely isolate the what i'm seeing: https://github.com/johngruen/webext-demo In the svg file you can see that I'm using `fill="context-fill"`. With the `svg.context-properties.content.enabled` defaulted to false, my icon shows up as black. Toggle the pref to true, reload the webextension in about:debugging and it turns white. Note, I'm on 56.0a1 (2017-07-09) (64-bit)

https://dxr.mozilla.org/mozilla-central/source/layout/svg/SVGContextPaint.cpp#63 The check is missing || scheme.EqualsLiteral("moz-extension")

Tracked for 55 based on comment 1 and Screenshots will go live in 55. Please nominate a patch for uplift to Beta55 soon.

Ritu, calling out that https://bugzilla.mozilla.org/show_bug.cgi?id=1379464 would need uplift as well, since this issue still requires a manual pref flip to enable context set icons

Comment on attachment 8884081 [details] Bug 1377302 - Support fill='context-fill' for WebExtension browser-actions. Approval Request Comment [Feature/Bug causing the regression]: not a regression, needed to support various themes with system add-ons (Screenshots) in v55. [User impact if declined]: Screenshots will show the wrong color icon if users enable a "dark" theme [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: no, see bug 1379464 [List of other uplifts needed for the feature/fix]: yes, bug 1379464 [Is the change risky?]: no [Why is the change risky/not risky?]: simple CSS change to the front-end browser code that allows fill colors to get used by add-ons [String changes made/needed]:

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23) > [String changes made/needed]: none

Comment on attachment 8884081 [details] Bug 1377302 - Support fill='context-fill' for WebExtension browser-actions. Web Extension support, seems low risk, let's uplift for beta 9.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23) > [Is this code covered by automated tests?]: no > [Has the fix been verified in Nightly?]: no > [Needs manual test from QE? If yes, steps to reproduce]: no, see bug 1379464 Setting qe-verify- based on Jared's assessment on manual testing needs.

This bug is not fixed. Tested using a new profile in 57.0b8. Still requires `svg.context-properties.content.enabled` to be flipped for icons to properly change colour.

Still not working in 57.0b10.

Still not working in 59.0b12. Still requires `svg.context-properties.content.enabled` to be flipped for icons to properly change color. Please fix this.

bug 1512878 has been opened for enabling this for ALL webextension toolbar icons, rather than just mozilla in-house webextensions