Allow extension themes to use CSS gradients.
Categories
(WebExtensions :: Themes, enhancement, P3)
Tracking
(Not tracked)
People
(Reporter: emilio, Assigned: emilio, NeedInfo)
Details
Attachments
(1 file)
The AI window theme uses a full-body gradient as a background. Themes right now would need to point to an SVG image to get the same effect which is unfortunate as it'd be more inefficient.
We should probably allow gradients in the additional_backgrounds property?
|
||
Comment 1•21 days ago
|
||
Nova themes will need this too at least on the toolbox.
|
Assignee | |
Updated•18 days ago
|
|
|
Assignee | |
Comment 2•15 days ago
|
||
And use this for Alpenglow. Note that I had to allow configuring
background-size too. Needs some tests, too, of course.
|
||
Updated•15 days ago
|
|
|
Assignee | |
Comment 3•15 days ago
|
||
Luca, see comment 2, I wasn't sure what better approach for structured data would look like, and this seemed simple enough?
|
||
Comment 4•15 days ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
Luca, see comment 2, I wasn't sure what better approach for structured data would look like, and this seemed simple enough?
I've replied to this needinfo on the phabricator side here, I think that the approach I was proposing is actually a slight simplification of the approach in your patch (the draft patch I've attached to demonstrate the structure data approach I was proposing doesn't seem to need the C++/rust part of the current version of the phabricator revision and the rest only tweaked a little bit).
Let me know what do you think about the tweaks to the approach I'm proposing through that draft patch, once we have settled on the implementation approach I'll look into helping to determine what kind of test coverage we would want on the webextensions theme API and schema side.
|
|
Assignee | |
Comment 5•15 days ago
|
||
I don't have super-strong opinions other than it looks a bit funkier / harder to use than my approach. E.g. for color properties theme authors don't need to do { "color_type": "rgba", "color_params": [...] } and we extend the CSS color syntax quite a lot.
Ideally I'd like this approach to be simpler to use than an SVG image (because those are slower). So I think it's nice that you can just slap the gradient there and it just works, and it is fairly parallel to the other background properties. But I don't feel super strongly one way or the other I guess?
Not sure if the above changes your mind... Do you know who (else) might have opinions about this? If not I guess I'm ok with your approach.
ni?ing dao because he probably has more context / gut feeling about theme authors...
|
|
||
Comment 6•14 days ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
I don't have super-strong opinions other than it looks a bit funkier / harder to use than my approach. E.g. for color properties theme authors don't need to do
{ "color_type": "rgba", "color_params": [...] }and we extend the CSS color syntax quite a lot.
Well, for the color properties the fact is that all the different formats of that property that we support (we support more than one in addition to the more lax string one) are all representing and translate into the very same thing: a color
In this case the property we are trying to extend is { images: { additional_backgrounds: [...] } and at the moment it is an array of either relative urls to the image files in the extension manifest (this is usually what a static theme manifest would include at the moment) or image data (which would be what an extension using the theme API could also choose to use) and trying to fit something completely different: a css gradient.
Part of me feels fitting the gradient into the images.additional_backgrounds may be a little bit of a stretch [1], but in the interest of keeping the change as simple as we can I'm trying to think how we could make it at least explicit and visible enough which one of the entries of the images.additional_backgrounds would be a gradient and not an image (and possibly with a validation that would propagate to the addons-linter without having to duplicate validation logic and then having to keep that validation logic in sync manually in multiple places).
I can see that the proposal I pasted in phabricator yesterday may feel a bit heavy in the JSON syntax/format, but I think we may find a reasonable balance by tweaking it a little bit more to feel lighter than my previous proposal, e.g. in the following github gist I pasted an alternative variant:
With this variant the theme manifest images.additional_backgrounds would look like this in the following snippet:
"additional_backgrounds": [
"background-noodles-right.svg",
"background-noodles-left.svg",
{ "linear-gradient": "to bottom, #FF6BBA -18.096%, #FFC999 50%" }
]
To me that doesn't feel that much heavier than linear-gradient(...), but would be something that the JSONSchema can still enforce easily enough (and that to be the only place we have a list of supported CSS gradient types, similarly to my previous proposal).
How does that slightly lighter format looks to you, does it feel still adding too much friction and demotivate theme authors from using it?
With this variant, the JSONSchema would still be able to produce a reasonable validation error on a manifest including an invalid gradient type (still with no changes applied to JSONSchema.sys.mjs):
Extension is invalid
Reading manifest: Error processing theme.images.additional_backgrounds.0: Value must either: be a string value, or must either [contain the required "linear-gradient" property, contain the required "radial-gradient" property, contain the required "conic-gradient" property, contain the required "repeating-linear-gradient" property, contain the required "repeating-radial-gradient" property, or contain the required "repeating-conic-gradient" property]
As an additional side note, with the gradient just added as a plain string like the urls and image data we already support, older Firefox versions would still allow to load the theme but it would likely be visually broken in "hard to predict" ways.
With a more explicit schema instead older Firefox builds would explicitly fail with a readable error if the theme is using a gradient background and the old Firefox build didn't have support for that (the error logged would be: Extension is invalid. Reading manifest: Error processing theme.images.additional_backgrounds.0: Expected string instead of {"linear-gradient":"to bottom, #9059FF 0%, #FF4AA2 100%"}).
We should also consider hooking up to the new ThemeCSSGradient type some addons-linter logic to enforce the requirement of a strict_min_version set accordingly to the first Firefox version actually supporting the new static theme feature.
Ideally I'd like this approach to be simpler to use than an SVG image (because those are slower). So I think it's nice that you can just slap the gradient there and it just works, and it is fairly parallel to the other background properties. But I don't feel super strongly one way or the other I guess?
I feel that in the short term the incentive to use SVG images would still be there independently from which manifest syntax/format we will be settling on, e.g. when the theme author may want to make the theme to work on a broader range of Firefox versions.
If we want to more vocally advice theme authors to avoid SVG images when they could use CSS gradients instead, it may be a good idea to consider publishing a new blog post with some examples of themes using the new feature (we can ask our devrel Christos to help with that once we will be ready for the comms part).
Not sure if the above changes your mind... Do you know who (else) might have opinions about this? If not I guess I'm ok with your approach.
ni?ing dao because he probably has more context / gut feeling about theme authors...
Thanks! Let's also gather dao's thoughts about it.
[1]: e.g. I wonder if we should also consider a separate gradients.additional_backgrounds instead, which would then allow us to support gradients elsewhere in the future if we need to and keep them together into the theme manifest properties, but that may complicate things a little bit in the shorter term (e.g. we would need likely new separate gradients.additional_alignment etc. and determine where the gradients backgrounds would be compared to images.additional_backgrounds, would they be below the images?).
|
|
Assignee | |
Comment 7•14 days ago
|
||
How does that slightly lighter format looks to you, does it feel still adding too much friction and demotivate theme authors from using it?
Seems fair I suppose, definitely better :)
Part of me feels fitting the gradient into the images.additional_backgrounds may be a little bit of a stretch [1], but in the interest of keeping the change as simple as we can I'm trying to think how we could make it at least explicit and visible enough which one of the entries of the images.additional_backgrounds would be a gradient and not an image (and possibly with a validation that would propagate to the addons-linter without having to duplicate validation logic and then having to keep that validation logic in sync manually in multiple places).
I don't think that's the right approach, it should be possible to interleave gradient and non-gradient backgrounds.
As an additional side note, with the gradient just added as a plain string like the urls and image data we already support, older Firefox versions would still allow to load the theme but it would likely be visually broken in "hard to predict" ways.
It won't tho, right? Doesn't that hit the "must be a relative URL or data URI" check?
The other thing that my patch deals with is injection. With that gist, a theme author could write "linear-gradient": "red), url(chrome://....), linear-gradient(transparent,", and with your approach you could end up requesting that chrome://` image, right? Or worse, some random http image over the network.
The isValidCSSImage check deals with that and that means we should probably keep it somewhere I believe, right? If so, where would be the best place to keep it?
|
|
||
Comment 8•14 days ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
How does that slightly lighter format looks to you, does it feel still adding too much friction and demotivate theme authors from using it?
Seems fair I suppose, definitely better :)
Part of me feels fitting the gradient into the images.additional_backgrounds may be a little bit of a stretch [1], but in the interest of keeping the change as simple as we can I'm trying to think how we could make it at least explicit and visible enough which one of the entries of the images.additional_backgrounds would be a gradient and not an image (and possibly with a validation that would propagate to the addons-linter without having to duplicate validation logic and then having to keep that validation logic in sync manually in multiple places).
I don't think that's the right approach, it should be possible to interleave gradient and non-gradient backgrounds.
That was exactly my guess and the reason why I didn't bring that idea up and instead looking into variants of your proposed approach with minimal tweaks to make the schema more explicit.
As an additional side note, with the gradient just added as a plain string like the urls and image data we already support, older Firefox versions would still allow to load the theme but it would likely be visually broken in "hard to predict" ways.
It won't tho, right? Doesn't that hit the "must be a relative URL or data URI" check?
Actually no, looking to the imageDataOrStrictRelativeUrl Schemas.sys.mjs formatter I was expecting it to turn whatever is in the string into a absolute moz-extension:// URL and to only throw if the string would trigger a failure due if it looks like an absolute URL.
I gave that a try to be sure I wasn't misreading that and on the currently nightly with no changes to the current logic and theme properties validation a theme manifest with an additional_backgroungs set to ["linear-gradient(to bottom, #9059FF 0%, #FF4AA2 100%)"], the linear-gradient string is getting translated by the Schema formatter into "moz-extension://UUID/linear-gradient(to%20bottom,%20#9059FF%200%,%20#FF4AA2%20100%)".
That is then going through the LightweightThemeConsumer logic inside _setImage that does escape it through CSS.escape and so inside _setProperties method value is the escaped string "url(moz-extension\\:\\/\\/d3d1367c-28c7-4676-ba1d-1e5485057c05\\/linear-gradient\\(to\\%20bottom\\,\\%20\\#9059FF\\%200\\%\\,\\%20\\#FF4AA2\\%20100\\%\\))", and that's what it gets set on the --lwt-additional-images CSS variable.
And so no error is logged.
The other thing that my patch deals with is injection. With that gist, a theme author could write
"linear-gradient": "red), url(chrome://....), linear-gradient(transparent,", and with your approach you could end up requesting thatchrome://` image, right? Or worse, some random http image over the network.The
isValidCSSImagecheck deals with that and that means we should probably keep it somewhere I believe, right? If so, where would be the best place to keep it?
That's definitely something we should avoid, my draft was meant to be more of a proof of concept than the complete patch, as I mentioned on the phabricator side I'm not really against keeping InspectorUtils.isValidCSSImage and I didn't exclude there may be good reasons to keep that part, and this feels like a more than reasonable motivation for keeping it.
We should have a couple of option to achieve that, e.g. we could add it as a postprocess function to the ThemeCSSGradient choices (I think we'll have to apply it on each of them with the current Schemas.sys.mjs internal logic), and then also check it on the LightweightThemeConsumer.sys.mjs _imageToCss helper function as another defense layer.
As an example, in the themes.json schemas file it should be something like the following:
{
"id": "ThemeCSSGradient",
"choices": [
{
"type": "object",
"postprocess": "validCSSGradient",
"properties": { "linear-gradient": { "type": "string" } },
"additionalProperties": false
},
...
Then on the Schemas.sys.mjs file we would add a new validCSSGradient postprocessor function that may look along the line:
const POSTPROCESSORS = {
...
validCSSGradient(obj, context) {
const [gradient, params] = Object.entries(obj)[0];
const cssGradient = `${gradient}(${params})`;
if (!InspectorUtils.isValidCSSImage(cssGradient)) {
const errorMessage = "Invalid ThemeCSSGradient";
if (context.cloneScope) {
// Report the error back to the extension caller
// when the theme API is being used programmatically.
throw new context.cloneScope.Error(
errorMessage
);
}
// Report a validation error when the invalid CSS gradient
// value is originated from a static theme.
context.logError(context.makeError(errorMessage));
return { [gradient]: "" };
}
return obj;
},
and maybe an additional check in LightweightThemeConsumer.sys.mjs along the line:
function _imageToCss(aWin, aImage) {
if (typeof aImage === "object") {
if (aImage.type !== "gradient_css") {
console.warn(
`LightweightThemeConsumer: unexpected image type "${aImage.type}"`
);
return null;
}
if (!InspectorUtils.isValidCSSImage(aImage.value)) {
console.warn("Ignoring invalid ThemeCSSGradient theme properties");
return null;
}
return aImage.value;
}
return `url(${aWin.CSS.escape(aImage)})`;
}
Let me know if these feels like a reasonable approach to close that gap in the proposed variant.
Description
•