Support fill="context-fill" for WebExtension icons

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Theme
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: ntim, Assigned: jaws)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55+ fixed, firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 1

5 months ago
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.

Updated

5 months ago
Flags: needinfo?(amckay)
Comment hidden (mozreview-request)

Comment 3

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

https://reviewboard.mozilla.org/r/155014/#review160062
Attachment #8884081 - Flags: review+

Updated

5 months ago
Assignee: nobody → jaws

Comment 4

5 months ago
wow, that was easier than i thought!

Comment 5

5 months ago
mozreview-review
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?
(Assignee)

Comment 6

5 months ago
mozreview-review
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 hidden (mozreview-request)

Updated

5 months ago
Flags: needinfo?(amckay)

Comment 8

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

Comment 9

5 months ago
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

5 months ago
(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

5 months ago
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

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

https://reviewboard.mozilla.org/r/155014/#review160190
Attachment #8884081 - Flags: review?(dao+bmo) → review+

Comment 13

5 months ago
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.
tracking-firefox55: --- → ?

Comment 15

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/657ef58ae035
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 16

5 months ago
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.

Updated

5 months ago
Component: WebExtensions: General → Theme
Product: Toolkit → Firefox
Target Milestone: mozilla56 → ---

Updated

5 months ago
Target Milestone: --- → Firefox 56

Comment 18

5 months ago
(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

5 months ago
(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)
Flags: needinfo?(jaws)
(Reporter)

Comment 20

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

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

Updated

5 months ago
Depends on: 1379464

Comment 21

5 months ago
Tracked for 55 based on comment 1 and Screenshots will go live in 55. Please nominate a patch for uplift to Beta55 soon.
status-firefox55: --- → affected
tracking-firefox55: ? → +

Comment 22

5 months ago
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]:
Flags: needinfo?(jaws)
Attachment #8884081 - Flags: approval-mozilla-beta?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> [String changes made/needed]: none

Updated

5 months ago
Blocks: 1380119
Blocks: 1380120
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.
Attachment #8884081 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/f37efe938cd4
status-firefox55: affected → fixed
(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.
Flags: qe-verify-
Blocks: 1388907
Blocks: 1396462
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

a month ago
Still not working in 57.0b10.
You need to log in before you can comment on or make changes to this bug.