Consider adding code for checking mozilla signing key for context-fill svg
Categories
(Core :: SVG, enhancement, P3)
Tracking
()
People
(Reporter: jkt, Assigned: rpl)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
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•7 years ago
|
||
I really don't want to go down this path. I'd rather we allow people to use context-fill.
Comment 2•7 years ago
|
||
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)?
Reporter | ||
Comment 3•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
"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.
Updated•7 years ago
|
Comment 6•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
(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.
Reporter | ||
Comment 15•7 years ago
|
||
> 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•7 years ago
|
||
(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 18•7 years ago
|
||
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 20•7 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Comment 24•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 25•3 years ago
|
||
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()
.
Updated•3 years ago
|
Updated•3 years ago
|
Comment 26•3 years ago
|
||
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/c9197b3a4ba0 Allow SVG context-fill on privileged extensions. r=dholbert
Comment 27•3 years ago
|
||
Backed out changeset c9197b3a4ba0 (Bug 1394579) for causing failures in test_ext_svg_context_fill.js CLOSED TREE
Log: https://treeherder.mozilla.org/logviewer?job_id=339720988&repo=autoland&lineNumber=4124
Backout: https://hg.mozilla.org/integration/autoland/rev/01f5d81f0afe7c1958a225d981bac39f52ae93fe
Assignee | ||
Comment 28•3 years ago
|
||
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 andaSVGContext::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 withexpectAllowed: false
do fail as expected), and but I can't yet see what is preventing the context properties to be exposed whensvg.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•3 years ago
|
||
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).
Assignee | ||
Comment 30•3 years ago
|
||
(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•3 years ago
|
||
Thanks! The followup looks like it's got enough information/links (given the limited amount that we know now, at least).
Assignee | ||
Comment 32•3 years ago
|
||
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•3 years ago
|
||
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/84eb62a713ed Allow SVG context-fill on privileged extensions. r=dholbert
Comment 34•3 years ago
|
||
Backed out for failures on test_ext_svg_context_fill.js
backout: https://hg.mozilla.org/integration/autoland/rev/a9ff48afca5016daeccbbb84fed08352477dd3f9
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)
Assignee | ||
Comment 35•3 years ago
|
||
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•3 years ago
|
||
That sounds like a likely explanation. Thanks for the follow-up investigation!
Comment 37•3 years ago
|
||
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/58bea82c7a31 Allow SVG context-fill on privileged extensions. r=dholbert
Comment 38•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Description
•