-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: webextensions.manifest.theme tab_background_separator and toolbar_field_separator deprecated in Firefox 89 #22216
base: main
Are you sure you want to change the base?
fix: webextensions.manifest.theme tab_background_separator and toolbar_field_separator deprecated in Firefox 89 #22216
Conversation
…r_field_separator deprecated in Firefox 89
51f6317
to
5e08ee2
Compare
webextensions/manifest/theme.json
Outdated
"version_added": "62" | ||
"version_added": "62", | ||
"version_removed": "89", | ||
"notes": "This theme property has been deprecated in Firefox 89. See the <a href='https://blog.mozilla.org/addons/2021/04/19/changes-to-themeable-areas-of-firefox-in-version-89/'>related blogpost</a> for more details." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe include the blog title in the link text to make it easier to see what the blog is about without hovering over the link?
webextensions/manifest/theme.json
Outdated
@@ -464,7 +464,9 @@ | |||
}, | |||
"edge": "mirror", | |||
"firefox": { | |||
"version_added": "62" | |||
"version_added": "62", | |||
"version_removed": "89", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpl is this correct? Given that it's deprecated, presumably, it's still available and hasn't been removed yet, So shouldn't it be flagged with:
"status": {
"deprecated": true,
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rebloor I was honestly also considering that, the reason why I went for version_removed is that these colors are going to be ignored and not used anywhere anymore since the Proton restyling, and so they basicallly behave more like a theme property that we never supported (e.g. if chrome adds support for a new theme property which we don't support, then it is also going to be similarly ignore and not be used anywhere), whereas for a deprecated property I'd expect it to still be used but produce a deprecation warning (e.g. if we would be renaming a property and keeping support for the old name for a while as a deprecated way to configure that same theme property).
@rebloor Let me know how this rationale sounds to you?
@Rob--W I'd also like to gather your perspective on this rationale for using version_removed instead of deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though there are some references to it in Firefox's source code, it is effectively unused, without doing anything.
There is one difference in comparison with a truly removed property: The theme.update
method throws for unrecognized properties, whereas theme.update
does not do anything special for this property.
I think that besides the documentation, the least that we can do is to add deprecated
to https://searchfox.org/mozilla-central/rev/1aaacaeb4fa3aca6837ecc157e43e947229ba8ce/toolkit/components/extensions/schemas/theme.json#127-130.
I'm voting for deprecated
due to the observable effect on theme.update
This PR has been sitting around for a while it seems. @rpl, do you plan to return to this PR and apply the requested changes? |
This pull request has merge conflicts that must be resolved before it can be merged. |
@rpl @Rob--W I was looking to merge this one, however, the BCD liniter won't let me - it reports
I assume that we might be able to add in two PRs, one for the version 62 addition followed by a second to cover the deprication/removal - do we want to try that or are you happy to consider abandoning? |
Do you mean two PRs, one to add them and another to list removed? Let's try that. And if it doesn't work, let's abandon this. People interested in the historical property she be able to find the answer in the blog post. |
@queengooborg the addition of the TypeError: Cannot use 'in' operator to search for 'name' in false Node.js v18.20.4 |
Web extensions cannot have status information per the schema, but that particular crash is unexpected... So long as the status block is removed, we should be good to go! (We can always discuss allowing the status block in the schema if it is needed at a later date too!) |
@queengooborg Thanks - I'll proceed without |
Summary
Updates on webextensions.manifest.theme to mark tab_background_separator and toolbar_field_separator as deprecated in Firefox 89.
Test results and supporting details
See #22215 for more context around this change.
Related issues
Fixes #22215