Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rpl
Copy link
Member

@rpl rpl commented Feb 13, 2024

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

@rpl rpl requested a review from Rob--W February 13, 2024 15:27
@github-actions github-actions bot added the data:webext 🎲 Compat data for Browser Extensions. https://developer.mozilla.org/Add-ons/WebExtensions label Feb 13, 2024
@rpl rpl force-pushed the fix/issue-22215-webext-theme-prosp-fx89-deprecated branch from 51f6317 to 5e08ee2 Compare February 13, 2024 15:36
"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."
Copy link
Member

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?

@@ -464,7 +464,9 @@
},
"edge": "mirror",
"firefox": {
"version_added": "62"
"version_added": "62",
"version_removed": "89",
Copy link
Collaborator

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,
          },

Copy link
Member Author

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.

Copy link
Member

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

webextensions/manifest/theme.json Outdated Show resolved Hide resolved
webextensions/manifest/theme.json Outdated Show resolved Hide resolved
@queengooborg
Copy link
Collaborator

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?

@github-actions github-actions bot added the merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. label Jul 2, 2024
Copy link

github-actions bot commented Jul 2, 2024

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. label Sep 12, 2024
@rebloor
Copy link
Collaborator

rebloor commented Sep 12, 2024

@rpl @Rob--W I was looking to merge this one, however, the BCD liniter won't let me - it reports

✖ webextensions.manifest.theme.colors.tab_background_separator - Error → feature was implemented and has since been removed from all browsers dating back two or more years ago.
✖ webextensions.manifest.theme.colors.toolbar_field_separator - Error → feature was implemented and has since been removed from all browsers dating back two or more years ago."

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?

@Rob--W
Copy link
Member

Rob--W commented Sep 20, 2024

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.

rebloor added a commit to rebloor/browser-compat-data that referenced this pull request Sep 20, 2024
rebloor added a commit that referenced this pull request Sep 20, 2024
@rebloor
Copy link
Collaborator

rebloor commented Sep 20, 2024

@queengooborg the addition of the status annotation is causing the linter to error:
'''
file:///home/runner/work/browser-compat-data/browser-compat-data/utils/walkingUtils.ts:16
*/ export const isBrowser = (obj)=>'name' in obj && 'releases' in obj;
^

TypeError: Cannot use 'in' operator to search for 'name' in false
at isBrowser (file:///home/runner/work/browser-compat-data/browser-compat-data/utils/walkingUtils.ts:16:44)
at lowLevelWalk (file:///home/runner/work/browser-compat-data/browser-compat-data/utils/walk.ts:32:13)
at lowLevelWalk.next ()
at lowLevelWalk (file:///home/runner/work/browser-compat-data/browser-compat-data/utils/walk.ts:45:20)
at lowLevelWalk.next ()
at lowLevelWalk (file:///home/runner/work/browser-compat-data/browser-compat-data/utils/walk.ts:45:20)
at lowLevelWalk.next ()
at lowLevelWalk (file:///home/runner/work/browser-compat-data/browser-compat-data/utils/walk.ts:45:20)
at lowLevelWalk.next ()
at lowLevelWalk (file:///home/runner/work/browser-compat-data/browser-compat-data/utils/walk.ts:45:20)

Node.js v18.20.4
'''
Any thoughts on how to fix?

@queengooborg
Copy link
Collaborator

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!)

@rebloor
Copy link
Collaborator

rebloor commented Sep 20, 2024

@queengooborg Thanks - I'll proceed without

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:webext 🎲 Compat data for Browser Extensions. https://developer.mozilla.org/Add-ons/WebExtensions
Projects
None yet
4 participants