1377302 - Support fill="context-fill" for mozilla-signed WebExtension icons

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Tim Nguyen :ntim]

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

Comment 1

[John Gruen [:jgruen]]

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 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1377302 - Support fill='context-fill' for WebExtension browser-actions.

Review commit: https://reviewboard.mozilla.org/r/155014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155014/

Comment 3

[Matthew Wein [:mattw]]

Comment on attachment 8884081 [details]
Bug 1377302 - Support fill='context-fill' for WebExtension browser-actions.

https://reviewboard.mozilla.org/r/155014/#review160062

Comment 4

[John Gruen [:jgruen]]

wow, that was easier than i thought!

Comment 5

[Matthew Wein [:mattw]]

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 6

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8884081 [details]
Bug 1377302 - Support fill='context-fill' for WebExtension browser-actions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155014/diff/1-2/

Comment 8

[Andy McKay]

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

Comment 9

[John Gruen [:jgruen]]

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.

Comment 10

[Andy McKay]

(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 :)

Comment 11

[John Gruen [:jgruen]]

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

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8884081 [details]
Bug 1377302 - Support fill='context-fill' for WebExtension browser-actions.

https://reviewboard.mozilla.org/r/155014/#review160190

Comment 13

[Pulsebot]

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

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

[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.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/657ef58ae035

Comment 16

[fx4waldi]

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

Comment 17

[Dão Gottwald [:dao]]

(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.

Comment 18

[fx4waldi]

(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.

Comment 19

[John Gruen [:jgruen]]

(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)

Comment 20

[Tim Nguyen :ntim]

https://dxr.mozilla.org/mozilla-central/source/layout/svg/SVGContextPaint.cpp#63

The check is missing || scheme.EqualsLiteral("moz-extension")

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 22

[John Gruen [:jgruen]]

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 23

[Jared Wein [:jaws] (please needinfo? me)]

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]:

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f37efe938cd4

Comment 27

[Andrei Vaida [:avaida]]

(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.

Comment 28

[Jasmin Bom [:jsmnbom]]

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.

Comment 29

[Richard West]

Still not working in 57.0b10.

Comment 30

[Tim Weißenfels]

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.

Comment 31

[waduripiyo]

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