1394579 - Consider adding code for checking mozilla signing key for context-fill svg

Status: RESOLVED FIXED

(Core :: SVG, enhancement, P3)

Description

[Jonathan Kingston [:jkt] he/him]

Currently I think the signing checks are done in jpm modules rather than the c++ so when layout/svg/SVGContextPaint.cpp checks the extension is available to context-fill I don't think we have code to do that.

This bug is to request this as extensions like Lightbeam aren't a mozilla.com or lightbeam extension so this would lead to a huge whitelist which isn't ideal.

Continuing to extend what Bug 1388907 did isn't a great solution.

Comment 1

[Andy McKay]

I really don't want to go down this path. I'd rather we allow people to use context-fill.

Comment 2

[Johann Hofmann [:johannh]]

Bug 1396462 was filed about removing the "@testpilot-" check (which I agree should be done).

This is getting quite absurd, IMO, considering the problem we're talking about. jwatt, how strongly do you feel about just allowing context-fill for moz-extension:// URLs without explicitly documenting (or officially supporting) the feature (ie. would you reconsider https://bugzilla.mozilla.org/show_bug.cgi?id=1379464#c3)?

Comment 3

[Jonathan Kingston [:jkt] he/him]

Creating a unique signing key for all Mozilla extensions (additional to the keys we have) would allow us to dog food features like this and would also be the most robust over time.
I actually think this is the better solution than chromes experimental apis: https://developer.chrome.com/extensions/experimental.

However this is a very heavyweight solution for just an icon colour certainly.

I also agree Bug 1396462 should happen at some point and this bug was to provide an alternate solution to that, however perhaps in this instance just exposing the colours to all would be better here.

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Johann Hofmann [:johannh] from comment #2)
> jwatt, how strongly do you feel about just allowing context-fill for
> moz-extension:// URLs without explicitly documenting (or officially
> supporting) the feature (ie. would you reconsider
> https://bugzilla.mozilla.org/show_bug.cgi?id=1379464#c3)?

Whether we document it or officially support it isn't really all that relevant. What's important is whether or not web extensions use it, and, if it's available, third parties will document it and it will be used.

The lack of suitability of -moz-context-properties + context-fill for web extensions and the web platform is unchanged. It a hack that we want to replace with a better mechanism and not get stuck supporting. As far as I can see we either plan in time to come up with, standardize and implement a less hacky mechanisms, or else we implement a cheap and sane way for the SVGContextPaint.cpp code to check whether an extension was signed by Mozilla (or more specifically, if we can easily rewrite the extension to use a future replacement for this mechanism).

Comment 5

[Andy McKay]

"The lack of suitability of -moz-context-properties + context-fill for web extensions and the web platform is unchanged". Is an assumption that the two are the same and I would argue that they are not. We have a clear and known way to clean up deprecated APIs when we find better ways to go. For example: we can contact all the add-on authors and let them know when things change.

Comment 6

[Jet Villegas (inactive)]

I would like to get us to a proper policy for what gets exposed to webextensions. The recurring "please expose feature x" debates aren't ideal. I agree there are use cases, and I agree they're valid, but I disagree that it's up to us (Layout team) to decide our webextension API design/exposure policy on a case-by-case basis. We have policies in place for exposing web-facing features (e.g., spec, intent to implement, etc.) and we should have something in place here, especially if we expect extensions for/from other browsers to work.

Comment 8

[Manuel Reimer]

IMHO there should be no checking for "mozilla signing". This feature is useful for all developers!
You use this feature for your extensions, as you found out that it is useful or even impossible to do some things without it.
Firefox offers a "dark theme", so if you create icons that match with the others on the Firefox UI (as I did for all my extensions), then the icon nearly disappears in the "dark theme" mode.
"context-fill" would be the best solution here, as the icon adapts for the selected theme.
Just add a switch to the "manifest.json" to enable/disable this and disable it by default. If a developer enables this feature for his addon, he knows what he does.
And in the meantime, I'll do a shitty workaround by adding a checkbox to my Addon settings to toggle between a dark and a light icon, as there doesn't even seem to be a way to find out about the choosen theme in Firefox...

Comment 9

[Manuel Reimer]

Forgot to add a link to the issue, a user filed for one of my extensions:
https://github.com/M-Reimer/undoclosetab/issues/18
This includes a screenshot which shows the problem.
And this basically covers all my extensions, as I always use a "gray color" which matches the other icons in Firefox in the "default theme".

Comment 10

[Jonathan Watt [:jwatt]]

(In reply to Andy McKay [:andym] from comment #5)
> "The lack of suitability of -moz-context-properties + context-fill for web
> extensions and the web platform is unchanged". Is an assumption that the two
> are the same and I would argue that they are not. We have a clear and known
> way to clean up deprecated APIs when we find better ways to go. For example:
> we can contact all the add-on authors and let them know when things change.

Past experience with legacy add-ons has shown that contacting add-on authors will not result in them all updating their add-ons. Some subset will not, and then, depending on how many don't and the number of users they have, we're either forced to:

 * keep supporting the old feature
 * fix the add-ons ourselves (if that's practical and even an option)
 * break those add-ons and upset users

None of these options are great. I agree that removing a feature from add-ons is not necessarily as hard as removing it from the Web, but it may end up being hard enough that there's no practical difference for a particular feature. We don't really have a way to know in advance.

Given that we know this feature is non-standard and not looked upon favorably by the SVG WG (not that they've settled on a better alternative yet), I don't want to end up in a bad spot here.

FWIW Chrome also seems to have this problem. For example, the 1Password icon is virtually invisible with the Emma Bridgewater theme. If Chrome hasn't found the impact on extensions to be so widespread and serious as to require them to implement a solution, I'd be interested to know if (and why) the situation is somehow worse for Firefox.

Comment 11

[Manuel Reimer]

I understand that "context-fill" is non-standard, but wouldn't it at least be possible to offer information about the current theme to extensions? If I knew about what color Firefox would add to the builtin icons (and when themes switch) I would just fetch the SVG content, run some regular expressions on it to replace one color and then set the result to my browserAction icon as data:-URL.

Just want to add this option here, as my initial bug got marked as duplicate of this one. Would be great to see at least some initial solution for Firefox 57.

> FWIW Chrome also seems to have this problem. For example, the 1Password icon
> is virtually invisible with the Emma Bridgewater theme. If Chrome hasn't
> found the impact on extensions to be so widespread and serious as to require
> them to implement a solution, I'd be interested to know if (and why) the
> situation is somehow worse for Firefox.

Is this really a valid question? Is there any reason why Firefox can't be better than Chrome?
At least I am not bothering at all about "Chrome compatibility". I just want to make nice, useful extensions for Firefox users.

Comment 12

[Johann Hofmann [:johannh]]

(In reply to Manuel Reimer from comment #11)
> I understand that "context-fill" is non-standard, but wouldn't it at least
> be possible to offer information about the current theme to extensions? If I
> knew about what color Firefox would add to the builtin icons (and when
> themes switch) I would just fetch the SVG content, run some regular
> expressions on it to replace one color and then set the result to my
> browserAction icon as data:-URL.
> 
> Just want to add this option here, as my initial bug got marked as duplicate
> of this one. Would be great to see at least some initial solution for
> Firefox 57.
> 
> > FWIW Chrome also seems to have this problem. For example, the 1Password icon
> > is virtually invisible with the Emma Bridgewater theme. If Chrome hasn't
> > found the impact on extensions to be so widespread and serious as to require
> > them to implement a solution, I'd be interested to know if (and why) the
> > situation is somehow worse for Firefox.
> 
> Is this really a valid question? Is there any reason why Firefox can't be
> better than Chrome?
> At least I am not bothering at all about "Chrome compatibility". I just want
> to make nice, useful extensions for Firefox users.

I agree we shouldn't settle with the best Chrome has to offer, that isn't the same as compatibility.

On the other hand, given the (understandable) resistance from platform side to expose this feature, we should consider going different routes to enable extensions to provide "brighttext" logos.

Andy, do you think we should add a new bug for that (potentially adding a new field to manifest.json > icons) and close this one?

Comment 13

[Jonathan Watt [:jwatt]]

FWIW this, or something like it, is probably the direction we should go in for a solution that could be exposed to web extensions and web content, and appears to be what the SVG WG was converging towards:

https://tabatkins.github.io/specs/svg-params/

It's certainly more tricky to implement than -moz-context-properties/context-*, there are some issues to work out (url()-modifiers don't help markup attributes like <img>'s 'src' attribute, so we'll need a solution for that), and the idea of changing the meaning of '#' in URIs is a bit scary (although annevk seems to think it's doable). However, it's a lot more generic both in the sense that we're not limited to exposing a small number of properties, and in the sense that it could also potentially be used to solve the same "problem" for HTML.

Comment 14

[Jonathan Watt [:jwatt]]

(In reply to Manuel Reimer from comment #11)
> > FWIW Chrome also seems to have this problem. For example, the 1Password icon
> > is virtually invisible with the Emma Bridgewater theme. If Chrome hasn't
> > found the impact on extensions to be so widespread and serious as to require
> > them to implement a solution, I'd be interested to know if (and why) the
> > situation is somehow worse for Firefox.
> 
> Is this really a valid question? Is there any reason why Firefox can't be
> better than Chrome?

I don't mean to suggest that we shouldn't do better than Chrome. I ask the question because the answer would be helpful in understanding the problem space better which is useful when trying to make hard trade-offs between "put crappy API out there that we may regret" and "put some of our extension authors it a difficult spot". If the answer is "Chrome doesn't suffer from this problem [to the same extend] because of ..." then that provide extra push to find a solution *now* for Firefox since we want to at least provide feature parity where we can. But if the answer is "Firefox doesn't suffer from this issue any more than other browsers do" then I'd lean more in favor of wanting to find a non-hacky solution that we won't regret and that we can standardize and solve for everybody.

Comment 15

[Jonathan Kingston [:jkt] he/him]

> https://tabatkins.github.io/specs/svg-params/

Wow that param syntax is ugly, anyway this might then block: https://bugzilla.mozilla.org/show_bug.cgi?id=1405780 it doesn't however extend the meaning of "media fragments" as drafted in other CSS specs so I'm not sure if there is a bigger issue there.

However there are two somewhat separate issues we are discussing here:
1. Passing colours in CSS to a SVG
2. Using native chrome colours in a manifest specified SVG

2. Is what this issue is discussing and doesn't really relate to the -moz-context-properties and just context-* on the receiving SVG element.

Is there any interim solution like:

1. Using currentColor?
2. Defining custom properties that the SVG could use like:

fill="-moz-context-fill currentColor #000"

Given the fallback in SVG for these, would proposing this to developers work? (I have not checked all browsers on this fallback)

3. Using a permission like "experimental-icon-fill" etc

> If the answer is "Chrome doesn't suffer from this problem"

Chrome don't ship versions that suffer from this by default do they, for example our dev edition

Comment 16

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Kingston [:jkt] from comment #15)
> However there are two somewhat separate issues we are discussing here:
> 1. Passing colours in CSS to a SVG
> 2. Using native chrome colours in a manifest specified SVG

This bug may have been filed for #2, but the only platform supported mechanism to achieve that is #1, and the Summary for this bug explicitly identifies changes to the mechanism to #1.

> 2. Is what this issue is discussing and doesn't really relate to the
> -moz-context-properties and just context-* on the receiving SVG element.
> 
> Is there any interim solution like:
> 
> 1. Using currentColor?
> 2. Defining custom properties that the SVG could use like:
> 
> fill="-moz-context-fill currentColor #000"
> 
> Given the fallback in SVG for these, would proposing this to developers
> work? (I have not checked all browsers on this fallback)

currentColor doesn't help here - it specifically means "the value of the 'color' property on this element", and isn't something we can change.

Allowing 'context-fill' or other keyword to be used in an embedded SVG to obtain a color from the embedding context requires platform support, and is specifically what I'm unhappy exposing and getting stuck supporting. The code to support that turned out to be a lot more involved and gnarly to get working than anticipated.

I think the most acceptable interim solution to me would be to hack up a way to allow the frontend code to set CSS variables in the SVG images that it embeds. As long as the only API that web extensions can see/use is some CSS variables that are defined for them, then we don't need to worry about whether the mechanisms that expose set those variable change radically.

> 3. Using a permission like "experimental-icon-fill" etc

It's not clear to me how that would help us. It wouldn't force add-on authors to update their add-ons. And it doesn't help us identify add-ons using context-fill/stroke much more than searching their source would.

> > If the answer is "Chrome doesn't suffer from this problem"
> 
> Chrome don't ship versions that suffer from this by default do they, for
> example our dev edition

I don't know, but the fact that our dev edition defaults to a dark theme is a good point.

Comment 17

[Jonathan Watt [:jwatt]]

Doesn't the fix for bug 1404568 mitigate this issues sufficiently?

Comment 18

[Johann Hofmann [:johannh]]

Uh, yeah, I didn't even know that existed (it's not documented on MDN). IMO that obsoletes this discussion and we should just disallow context-fill from all browserAction icons. (I realize having the exact color from context-fill is nice, but come on).

Comment 19

[Jonathan Kingston [:jkt] he/him]

I thought API was going to be flagged for removal? :andym?

Comment 20

[Andy McKay]

Bug 1404568 helps quite a lot. It allows the majority of add-on authors to be able to cope with setting a light and dark icon. However there's still a couple of problems: 

* the background of a browser action does not always match the theme, as detailed in bug 1408577
* the background might not be light or dark, but rather whatever the user wants through lightweight themes or the webextensions theming API.
* it doesn't work with page actions, sidebars (which currently don't get themed anyway)

It looks like bug 1329242 missed dev-doc-needed, so we added that in.

I'd like to recommend SVG as the best way to do icons and having an icon that react to the background in some way, would mean we could remove a lot of the work we have to do maintain this in the long term.

Comment 22

[rugk]

BTW I would really like to use something like that for the SVG icons of the WebExtension. See https://discourse.mozilla.org/t/use-theme-colors-for-svg-favicons/29165

When changing the theme (e.g. with Firefox Color) and your add-on is designed following the Photon guidelines – as it is recommended, this is a pity otherwise.
I does not have to be "context-fill", but some way to get icons looking nice in the theme.

Comment 23

[Val Packett]

Please allow context-fill for all extensions. Even when you figure out the "less hacky way" or whatever, and remove context-fill, the colors would just fall back to black. It's not like JS APIs that are hard to remove without breaking things, it's just a color value.

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1394579 - Allow SVG context-fill on privileged extensions. r?#firefox-svg-reviewers!

Comment 25

[Luca Greco [:rpl] [:luca] [:lgreco]]

I've added as a see also Bug 1710781 and mozilla/multi-account-containers#2046, in both cases it seems we are tempted to add more checks based on the addon ids to allow use of the SVG context-fill on their extension icon.

The patch I just attached (D114957) adds a check for privileged extensions instead, hoping we can avoid to keep adding more addon is based special cases, but it doesn't remove yet the existing checks for @mozilla.com / @mozilla.org addon id suffixes, because it may be more reasonable to remove them as a follow up after double-checking that the line extension that were leveraging those checks have been signed to be able to use the IsPrivileged().

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9197b3a4ba0
Allow SVG context-fill on privileged extensions. r=dholbert

Comment 27

[Noemi Erli[:noemi_erli]]

Backed out changeset c9197b3a4ba0 (Bug 1394579) for causing failures in test_ext_svg_context_fill.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=c9197b3a4ba0299d9cf21f43be6d30873a1b46ff&selectedTaskRun=UhkI-5qhTkWM-fqzv4j9oA.0

Log: https://treeherder.mozilla.org/logviewer?job_id=339720988&repo=autoland&lineNumber=4124

Backout: https://hg.mozilla.org/integration/autoland/rev/01f5d81f0afe7c1958a225d981bac39f52ae93fe

Comment 28

[Luca Greco [:rpl] [:luca] [:lgreco]]

It looks that the failure linked in comment 27 is android only.

I can definitely reproduce the same failure locally (confirmed by doing an android x86 full build and running the xpcshell test into the x86 emulator), from a quick look it seems that on an Android build nsSVGStyle::ExposesContextProperties is always returning false and aSVGContext::GetContextPaint returning null are and we never get to call to SVGContextPaint::IsAllowedForImageFromURI that the xpcshell test is meant to cover.

Personally I think that it would be ok to skip the xpcshell test on Android builds for now, but I'd like to put an inline comment nearby the skipif to briefly explain why we are skipping it on Android, and possibly mention a follow up issue filed upfront to investigate it further.

I'm happy to file the new followup issue and update the patch to skip the xpcshell test on Android, but I'd like to hear :dholbert perspective first, so that we can also include some more context about the reasons behind the different behavior on Android and what the followup is meant to do about it.

:dholbert wdyt? do you have some more details about the reasons for the different behavior I'm observing on Android?

As an additional side notes:

• I did notice that if I flip the pref svg.context-properties.content.enabled to true, nsSVGStyle::ExposesContextProperties does then return true and aSVGContext::GetContextPaint does return a SVGEmbeddingContextPaint pointer (the test does still fail as expected, because the pref does allow the context fill on any image url and so the test cases with expectAllowed: false do fail as expected), and but I can't yet see what is preventing the context properties to be exposed when svg.context-properties.content.enabled is set to false.

• One difference in the Android build that may be related to the different behavior observed is that on Android the Extension pages runs in the main process instead of running in a child webextensions process as they do by default in desktop builds.

Comment 29

[Daniel Holbert [:dholbert]]

That's odd, but not entirely surprising. I don't think this is intended to not work on Android; but I also don't think we use this on Android at all, since the use-case is for Gecko-rendered frontend content, and IIRC we don't really have any of that on Android.

So, yeah - seems fine to land this with the test annotated as skipped on Android, but I do think it'd be worth having a followup bug filed to track that (referenced alongside the skip annotation).

Comment 30

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Daniel Holbert [:dholbert] from comment #29)

That's odd, but not entirely surprising. I don't think this is intended to not work on Android; but I also don't think we use this on Android at all, since the use-case is for Gecko-rendered frontend content, and IIRC we don't really have any of that on Android.

So, yeah - seems fine to land this with the test annotated as skipped on Android, but I do think it'd be worth having a followup bug filed to track that (referenced alongside the skip annotation).

I totally agree, I just filed Bug 1711502 to track the follow up (let me know if the issue as described is clear enough to be actionable, so that it can actually be more easily picked up when we will need it or have time to investigate it).

I'll update the patch to skip the new test file on Android, reference Bug 1711502 on the skip-if config line and push the patch to autoland again.

Comment 31

[Daniel Holbert [:dholbert]]

Thanks! The followup looks like it's got enough information/links (given the limited amount that we know now, at least).

Comment 32

[Luca Greco [:rpl] [:luca] [:lgreco]]

I pushed the patch to autoland but unfortunately it was conflicting with a patch that was not on central yet but was already on autoland and it did never get pushed to autoland (the conflicting patch was the one landed from Bug 1674383, which was unfortunately also adding a test into the same xpcshell.ini file and exactly in the context of the diff part of this patch).

From a quick look to the oranges tracked on Bug 1674383's autoland patch, it looks that it should be able to reach mozilla-central pretty soon, I'll wait a bit to see if I can just rebase the patch attached to this issue on top of mozilla-central and then push it to autoland again (otherwise, as a last resort, I may rebase this patch on top of autoland instead).

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84eb62a713ed
Allow SVG context-fill on privileged extensions. r=dholbert

Comment 34

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on test_ext_svg_context_fill.js

backout: https://hg.mozilla.org/integration/autoland/rev/a9ff48afca5016daeccbbb84fed08352477dd3f9

push: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=X9s7DzFEQsern_vuntv83g.0&revision=84eb62a713ed2d8405ba800a9c930bac26d6cc0f&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer?job_id=340025114&repo=autoland&lineNumber=2662

[task 2021-05-18T13:20:13.415Z] 13:20:13 INFO - TEST-START | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_svg_context_fill.js
[task 2021-05-18T13:20:19.660Z] 13:20:19 WARNING - TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_svg_context_fill.js | xpcshell return code: 0
[task 2021-05-18T13:20:19.661Z] 13:20:19 INFO - TEST-INFO took 6243ms
[task 2021-05-18T13:20:19.661Z] 13:20:19 INFO - >>>>>>>
[task 2021-05-18T13:20:19.662Z] 13:20:19 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2021-05-18T13:20:19.663Z] 13:20:19 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2021-05-18T13:20:19.664Z] 13:20:19 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2021-05-18T13:20:19.665Z] 13:20:19 INFO - running event loop
[task 2021-05-18T13:20:19.665Z] 13:20:19 INFO - xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_svg_context_fill.js | Starting check_remote
[task 2021-05-18T13:20:19.666Z] 13:20:19 INFO - (xpcshell/head.js) | test check_remote pending (2)

Comment 35

[Luca Greco [:rpl] [:luca] [:lgreco]]

I looked a bit into this and I think that in the end the failure is just being triggered by the fact that Bug 1708384 landed on mozilla-central in the meantime (in particular this patch).

That patch wasn't landed yet on central when I branched out the hg bookmark that I was working on while preparing the patch attached to this issue, and so I think that the test case was passing as expected without Bug 1708384, but it is not expected to be able to run successfully on top of the changes from Bug 1708384 (because the new test case is loading the svg image into an extension page which is technically a content page, and that seems to be the new expected behavior if I'm reading Bug 1708384 patches correctly).

I think that the right choice is to rework the test to don't use a content page for testing the expected behaviors, I'm going to look into that.

After I have got the new version of the test, I may also try to run the new version of the test on Android, if it turns out the issue on Android was actually the same we may close Bug 1711502 and let the run the test on Android build too.

Comment 36

[Daniel Holbert [:dholbert]]

That sounds like a likely explanation. Thanks for the follow-up investigation!

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58bea82c7a31
Allow SVG context-fill on privileged extensions. r=dholbert

Comment 38

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/58bea82c7a31