Bug 2036647 Add use of CSS gradients to extension themes#44551
Conversation
|
Preview URLs (2 pages)
Flaws (1) Note! 1 document with no flaws that don't need to be listed. 🎉 Found an unexpected or unresolvable flaw? Please report it here. URL:
External URLs (2)URL:
(comment last updated: 2026-07-10 18:05:51) |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
rpl
left a comment
There was a problem hiding this comment.
@rebloor looks pretty good to me, the following are just small nits and some parts to consider some small rewording for and a followup (for something related to themes and Firefox for Android that I happen to have noticed while reviewing this patch, but I'd leave larger changes for parts that we wouldn't be already changing in this pull request for a separate followup specifically related to cleaning up the theme page from old references to Firefox for Android static theme support).
| <code>"contain"</code>, or explicit width and height values (for | ||
| example, <code>"100px 200px"</code>). | ||
| </p> | ||
| <p>If not specified, defaults to <code>"auto"</code>.</p> |
There was a problem hiding this comment.
question: should we mention explicitly what happens if the number of entries do not match the number of entries in the additional_backgrounds property? I assume we use the default, auto, in that case. If that feels like already implied we may not need to specify it but I thought to bring it up to double-check it with you.
There was a problem hiding this comment.
@rpl According to Claude, we don't default to auto, rather "The implementation joins the size values as a comma-separated CSS background-size property value. That means standard CSS cycling behavior applies — if additional_backgrounds_size has fewer entries than additional_backgrounds, the size values cycle from the beginning of the array for the extra items. For example, with 3 backgrounds and "additional_backgrounds_size": ["auto", "100px"], the third item gets auto (cycling back to index 0)." Are you able to confirm? If so, it doesn't sound like the behavior that we would expect.
There was a problem hiding this comment.
@rebloor I've looked a little deeper into this and confirmed which part of the w3c specs explicitly mention this behavior:
In this section named "2.1. Layering Multiple Background Images" the specs explicitly mention that:
Each of the background images is sized, positioned, and tiled according to the corresponding value in the other background properties. The lists are matched up from the first value: excess values at the end are not used. If a property doesn’t have enough comma-separated values to match the number of layers, the UA must calculate its used value by repeating the list of values until there are enough.
And so:
- the extra background layer (corresponding to extra entries in
additional_backgroundscompared toadditional_backgrounds_size/additional_backgrounds_alignment/additional_backgrounds_tiling) cycles back to index 0 of the size list (and the ones that may be following it will be matched with the other indexes available and then cycling back to index 0 if necessary) - extra values in
additional_backgrounds_size/additional_backgrounds_alignment/additional_backgrounds_tilingare going to be ignored - if
additional_background_sizeis completely omitted then every background image layer would be expected to default toauto(based on https://www.w3.org/TR/css-backgrounds-3/#background-size)
There was a problem hiding this comment.
@rpl Also confirmed this while testing the additional background items. I'll update the docs before Monday.
| <p><code>Array</code> of <code>String</code></p> | ||
| </td> | ||
| <td> | ||
| <p>Optional</p> |
There was a problem hiding this comment.
Nit: the other entry seems to have a period at the end of the Optional string, let's consider to add it here too for consistency (technically also on the properties description in the first table of the section https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/theme#syntax too).
There was a problem hiding this comment.
The more consistent approach across the entire web extensions documentation set is to not have the period. So I've removed them here.
There was a problem hiding this comment.
Thanks, sounds great to me, I'm also all for keeping consistency (thanks for double-checking what was the most common convention to match).
…/github.com/rebloor/content into Bug-2036647-CSS-gradients-in-extension-themes
rebloor
left a comment
There was a problem hiding this comment.
Incoming with the next commit
| <p><code>Array</code> of <code>String</code></p> | ||
| </td> | ||
| <td> | ||
| <p>Optional</p> |
There was a problem hiding this comment.
The more consistent approach across the entire web extensions documentation set is to not have the period. So I've removed them here.
| <code>"contain"</code>, or explicit width and height values (for | ||
| example, <code>"100px 200px"</code>). | ||
| </p> | ||
| <p>If not specified, defaults to <code>"auto"</code>.</p> |
There was a problem hiding this comment.
@rpl According to Claude, we don't default to auto, rather "The implementation joins the size values as a comma-separated CSS background-size property value. That means standard CSS cycling behavior applies — if additional_backgrounds_size has fewer entries than additional_backgrounds, the size values cycle from the beginning of the array for the extra items. For example, with 3 backgrounds and "additional_backgrounds_size": ["auto", "100px"], the third item gets auto (cycling back to index 0)." Are you able to confirm? If so, it doesn't sound like the behavior that we would expect.
rpl
left a comment
There was a problem hiding this comment.
@rebloor the updates looks good to me, in the review comments below there are a couple more details (related to theme_frame not being an array), besides that the only other remaining item is to describe correctly the behavior to expect when the additional_backgrounds and additional_backgrounds_size/additonal_backgrounds_tiling/additonal_backgrounds_alignment arrays length mismatch (which is confirmed based on what the w3d specs are explicitly mentioning, I've added some notes about where those behaviors are mentioned in the specs in this github comment #44551 (comment)), thanks for bringing that up!
| in the array on top, the last image in the array at the bottom. | ||
| An array of additional background items, each being either the path to an image asset packaged in the extension or <a href="#css_gradient_syntax">CSS gradient</a>, to be added to | ||
| the header area and displayed behind the | ||
| <code>"theme_frame":</code> items. These items layer the first item in |
There was a problem hiding this comment.
Nit: items -> item (give that theme_frame can only provide one, unlike additonal_backgrounds which is expected to be an array0
There was a problem hiding this comment.
| <code>"theme_frame":</code> items. These items layer the first item in | |
| <code>"theme_frame":</code> item. These items layer the first item in |
| <code>"theme_frame":</code> items. These items layer the first item in | ||
| the array on top and the last item at the bottom. CSS gradients are | ||
| supported from Firefox 153. |
There was a problem hiding this comment.
Given that theme_frame isn't an array but a single path to the packaged image asset or a single gradient, I feel like the "These items layer the first item in the array on top and the last item at the bottom" may actually belong to the additional_backgrounds property instead (given that one is expected to be an array).
If I'm not mistaken theme_frame is layered on top of whatever the theme would be listing in additional_backgrounds, @rebloor would you mind to give that a try and then adjust the theme_frame description to mention the actual expected behavior when both additional_backgrounds and theme_frame are specified in a static theme?
There was a problem hiding this comment.
@rpl amended this to clarify that "these additional background items" behave as documented (and I tested to confirm). I've also included a note that if you set theme_frame to a CSS gradient, it hides the additional background items.
| <p><code>Array</code> of <code>String</code></p> | ||
| </td> | ||
| <td> | ||
| <p>Optional</p> |
There was a problem hiding this comment.
Thanks, sounds great to me, I'm also all for keeping consistency (thanks for double-checking what was the most common convention to match).
Description
Addresses the dev-docs-needed requirements of Bug 2036647 "Allow extension themes to use CSS gradients" with:
Related issues and pull requests
Related browser compatibility updates in PR mdn/browser-compat-data#29910.