Closed Bug 1377302 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Theme, defect)

Product:
Firefox
Component:
Theme
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox55 + fixed
firefox56 --- fixed

People

(Reporter: ntim, Assigned: jaws)

References

Details

Attachments

(1 file)

Bug 1377302 - Support fill='context-fill' for WebExtension browser-actions.
59 bytes, text/x-review-board-request
dao
: review+
lizzard
: approval-mozilla-beta+
Details 
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.
Flags: needinfo?(amckay)
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+
Assignee: nobody → jaws
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.
Flags: needinfo?(amckay)
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
Attachment #8884081 - Flags: review?(dao+bmo) → review+
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: --- → ?
https://hg.mozilla.org/mozilla-central/rev/657ef58ae035
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.
Component: WebExtensions: General → Theme
Product: Toolkit → Firefox
Target Milestone: mozilla56 → ---
Target Milestone: --- → Firefox 56
(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)
Flags: needinfo?(jaws)
https://dxr.mozilla.org/mozilla-central/source/layout/svg/SVGContextPaint.cpp#63

The check is missing || scheme.EqualsLiteral("moz-extension")
Depends on: 1379464
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: ? → +
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
Blocks: 1380119
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+
(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: 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.
Still not working in 57.0b10.
Summary: Support fill="context-fill" for WebExtension icons → Support fill="context-fill" for mozilla-signed WebExtension icons
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: