Closed Bug 1472740 Opened 6 years ago Closed 5 years ago

Remove aliased property names from themes

Categories

(WebExtensions :: Themes, task, P1)

Product:
WebExtensions
Component:
Themes
Type:
task

Tracking

(firefox70 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: aswan, Assigned: rpl)

References

Details

(Keywords: dev-doc-complete)

Attachments

(7 files)

Bug 1472740 - Warn on deprecated static theme LWT aliases usage. r?ntim
47 bytes, text/x-phabricator-request
Details | Review
Bug 1472740 - Avoid LWT aliases in theme API tests that are not specifically about LWT aliases. r?ntim,jaws!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1472740 - Remove usage of deprecated lwt aliased from browser_ext_manager.js. r?zombie!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1472740 - Remove usage of deprecated lwt aliased from browser_webapi_theme.js. r?ntim,robwu!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1472740 - Remove support for deprecated lwt aliases from WebExtensions theme API. r?ntim!,robwu!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1472740 - Remove usage of deprecated lwt aliases from Fennec robocop tests. r?gbrown!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1472740 - Remove usage of deprecated lwt aliases from telemetry tests. r?janerik!
47 bytes, text/x-phabricator-request
Details | Review 
Carrying this over from discussion on bug 1466196.
Apparently themes have some aliased property names (see bug 1330340).  As a result, we cannot completely validate themes using the schema for multiple reasons:
1. The schema cannot express that either a property and its alias are mutually exclusive
2. If a mandatory property has an alias, we cannot express that one or the other must be present

Since themes must be validated both in the browser and on AMO, this means writing additional code and keeping it in sync between the two, which is a recipe for bugs.

Moreover, aliases make themes marginally more difficult to document, for new users to understand (which name should they use?)

So, are the aliases really all that valuable?  If we removed them, that would address the validation-on-AMO issue.  And if that's something we can agree on, now (ie, before we start accepting them on AMO) would be a great time to do it.
The aliases added in bug 1466196 make theme creation a bit easier for anyone familiar with lightweight themes, but the cost of supporting them sounds high, requiring validation code in Firefox and on AMO.  I support their removal, preferably before AMO transitions from LWT's to static themes.

For any extension currently using the theme API, we can continue to support the aliases for now, emit a console warning that they are end-of-life, and plan to deprecate them within six months.
I'm personally for keeping only the Google Chrome names (theme_frame/frame/tab_background_text), since they give us the benefit of compatibility with Chrome.

Unfortunately, it seems like the Firefox names (headerURL/accentcolor/textcolor) are more popular at the moment.
Also, toolbar_text and bookmark_text are currently aliased to each other, but that's less of an issue, because they're not mandatory.

Another issue we have with aliases is that theme.getCurrent() returns whatever property name that was originally specified by the theme author.
(In reply to Tim Nguyen :ntim from comment #3)
> Another issue we have with aliases is that theme.getCurrent() returns
> whatever property name that was originally specified by the theme author.

That's just one more reason not to use aliases at all.
Assignee: nobody → aswan
Priority: -- → P1
Summary: Consider removing aliased property names from themes → Remove aliased property names from themes
Since this didn't get done in before AMO started accepting themes, I think we now have to do a lengthier process to deprecate any aliases names before removing them.  Resetting priority for re-triage.
Assignee: aswan → nobody
Priority: P1 → --
Priority: -- → P2
This patch doesn't currently prevent a static theme which uses the LWT aliases from
being installed successfully but, as the first step for their deprecation and removal,
it starts to log a warning message when these aliases are being used (e.g. when
installing the static theme extension from "about:debugging", these warnings are being
shown to the theme author).

A similar linting warning is going to be emitted on AMO submissions
(See https://github.com/mozilla/addons-linter/issues/2259), and it will
be turned it into a linting error once AMO should start to prevent new static
theme submittions from using the LWT aliases.
Keywords: leave-open
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Note that we also have "bookmark_text" (chrome name) as an alias to "toolbar_text". The "toolbar_text" name makes more sense in terms of how the property is used, but bookmark_text is actually supported by chrome.

For this one, I really have no preference which to keep.
(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #8)
> Note that we also have "bookmark_text" (chrome name) as an alias to
> "toolbar_text". The "toolbar_text" name makes more sense in terms of how the
> property is used, but bookmark_text is actually supported by chrome.
> 
> For this one, I really have no preference which to keep.

Since we've elected to go with Chrome for all of the other field names, it probably doesn't make sense to have a single field name that is incompatible with Chrome, even if the field is optional.  We should go with bookmark_text.

Luca, can you add this one to the patch, too?
Flags: needinfo?(lgreco)
Depends on: 1508425
Depends on: 1508428
https://wiki.mozilla.org/WebExtensions/DeprecationPolicy

Per the deprecation policy above, this change should adhere to the following schedule:

announcement (dev-addons, MDN) - 65
linter warning - 65
firefox warning - 65
...
remove from firefox - 69
linter error - 69
update MDN - 69

This means that the next Firefox ESR (68) will also be the last version of Firefox to support the aliased properties.
This patch replace the LWT aliases with their related non-deprecated alias in all the theme API tests
that don't seem to be specifically testing the LWT aliases (e.g. browser_ext_themes_lwtsupport.js is
leaved unmodified for this reason).

The main reason to replace them in the "not stricly LWT-related" tests before their final removal
(currently planned for Firefox 69) is that the deprecation warnings will make these tests more
noisy (and so they may be making harder to investigate failures, without any actual gain in terms
of coverage).

Depends on D12297
(In reply to Mike Conca [:mconca] from comment #9)
> (In reply to Tim Nguyen :ntim (please use needinfo?) from comment #8)
> > Note that we also have "bookmark_text" (chrome name) as an alias to
> > "toolbar_text". The "toolbar_text" name makes more sense in terms of how the
> > property is used, but bookmark_text is actually supported by chrome.
> > 
> > For this one, I really have no preference which to keep.
> 
> Since we've elected to go with Chrome for all of the other field names, it
> probably doesn't make sense to have a single field name that is incompatible
> with Chrome, even if the field is optional.  We should go with bookmark_text.
> 
> Luca, can you add this one to the patch, too?

Sure thing, I added toolbar_text to the deprecated aliases in attachment 9026093 [details].

As a side note, after I looked into it I've been a bit in doubt about the toolbar_text deprecation,
mostly because it provides consistency with the name of the additional "toolbar_*" theme color properties
supported on Firefox:

- https://searchfox.org/mozilla-central/search?q=toolbar_&case=false&regexp=false&path=theme.json

(but none of them seems to be shared with Chrome, at least based on the list I've seen on
https://github.com/Patrick-Batenburg/GoogleChromeThemeCreationGuide#color-elements-1, 
and so they are not in the aliases "basket").
Flags: needinfo?(lgreco)
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/7b5c486601e5
Warn on deprecated static theme LWT aliases usage. r=ntim,jaws
https://hg.mozilla.org/integration/autoland/rev/5ad19f215939
Avoid LWT aliases in theme API tests that are not specifically about LWT aliases. r=ntim,jaws
Depends on: 1511947

Please reconsider NOT to deprecate "toolbar_text".

"bookmark_text" is not suitable wording for the text color of the toolbar.
"toolbar_text" is much more semantic.

(In reply to Kazumasa Hasegawa (Kazz) from comment #15)

Please reconsider NOT to deprecate "toolbar_text".

"bookmark_text" is not suitable wording for the text color of the toolbar.
"toolbar_text" is much more semantic.

Agreed. The color is used outside of the bookmarks toolbar, so bookmark_text would be misleading. It's fine to support bookmark_text as an alias to be compatible with Chrome themes, but compatibility doesn't need to go both ways as we aim to make Firefox more extensible than Chrome.

The same goes for textcolor / tab_background_text. tab_background_text is fine as an alias for compatibility, but in reality that color is used more broadly than just for background tabs.

(In reply to Dão Gottwald [::dao] from comment #16)

(In reply to Kazumasa Hasegawa (Kazz) from comment #15)

Please reconsider NOT to deprecate "toolbar_text".

"bookmark_text" is not suitable wording for the text color of the toolbar.
"toolbar_text" is much more semantic.

Agreed. The color is used outside of the bookmarks toolbar, so bookmark_text would be misleading. It's fine to support bookmark_text as an alias to be compatible with Chrome themes, but compatibility doesn't need to go both ways as we aim to make Firefox more extensible than Chrome.

The same goes for textcolor / tab_background_text. tab_background_text is fine as an alias for compatibility, but in reality that color is used more broadly than just for background tabs.

The argument makes sense to me, but on the other hand, aliases make it harder for theme validation, which is why this change was decided. I don’t have a strong opinion tbh, but maybe we can figure out a solution that let’s us have descriptive names and compatibility with chrome without making it a pain to validate.

Based on comment 10, we should proceed on ripping off the deprecated aliases in Firefox 69, and so pretty soon.

We leaved this issue open to attach and then land the patch that will remove the deprecated aliases from the API schemas (which will then turn any usage of those aliases into errors raised by the linter on new addon submission, once the addons-linter will have imported the updated 69beta schemas).

Are we ready to proceed to the "API schema LWT aliases rip off"?
e.g. have we reached an agreement about what to do about toolbar_text? (if it has to stay or if it can go away with the other ones)

Would you mind to proceed on creating the new patch?
(if no one can work on the new patch, I'm also willing to prepare and attach a new patch to this issue, but I would still need your help to decide what we want to do about toolbar_text).

Flags: needinfo?(ntim.bugs)

Hey Andrew,
we planned to completely remove support for the LWT theme aliases from the API schemas in Firefox 69,
and so I was wondering: is there any of the "AMO converted LWT themes" still using the deprecated aliases?

Flags: needinfo?(awilliamson)

(In reply to Tim Nguyen :ntim from comment #17)

The argument makes sense to me, but on the other hand, aliases make it harder for theme validation, which is why this change was decided. I don’t have a strong opinion tbh, but maybe we can figure out a solution that let’s us have descriptive names and compatibility with chrome without making it a pain to validate.

It's not clear to me why aliases make theme validation much harder. Generally it doesn't seem unreasonable for a validator to handle such things. What in particular is the problem here?

(In reply to Luca Greco [:rpl] from comment #18)

Are we ready to proceed to the "API schema LWT aliases rip off"?
e.g. have we reached an agreement about what to do about toolbar_text?

No, we have not. Speaking from the Firefox theme perspective, I can't promise how will implement these things if the Chrome aliases become the primary and only names. We could either limit where they apply to reflect their names, which would likely break themes, or we could misuse them beyond their intended context, which would confuse theme authors targeting Firefox going forward. Either way seems problematic.

we planned to completely remove support for the LWT theme aliases from the API schemas in Firefox 69,
and so I was wondering: is there any of the "AMO converted LWT themes" still using the deprecated aliases?

No, we repacked all the themes listed on AMO , (changed the manifest in the .xpi and as saved it as a new version). Both the ones we migrated from LWT, and user uploads since then. There will be some versions created as unlisted and privately distributed out there but that's beyond our control.

Flags: needinfo?(awilliamson)

(In reply to Dão Gottwald [::dao] from comment #20)

(In reply to Tim Nguyen :ntim from comment #17)

The argument makes sense to me, but on the other hand, aliases make it harder for theme validation, which is why this change was decided. I don’t have a strong opinion tbh, but maybe we can figure out a solution that let’s us have descriptive names and compatibility with chrome without making it a pain to validate.

It's not clear to me why aliases make theme validation much harder. Generally it doesn't seem unreasonable for a validator to handle such things. What in particular is the problem here?

I think that the main issue is that the API schema for the theme manifest properties is not currently providing any information about which are the aliases and what to do when both the aliased and aliases properties are specified in the manifest at the same time (and both Firefox and the addons-linter use the API schemas to validate the manifest and the parameters supported by the API methods).

(In reply to Luca Greco [:rpl] from comment #18)

Are we ready to proceed to the "API schema LWT aliases rip off"?
e.g. have we reached an agreement about what to do about toolbar_text?

No, we have not. Speaking from the Firefox theme perspective, I can't promise how will implement these things if the Chrome aliases become the primary and only names. We could either limit where they apply to reflect their names, which would likely break themes, or we could misuse them beyond their intended context, which would confuse theme authors targeting Firefox going forward. Either way seems problematic.

Personally I don't disagree, as I mentioned in comment 12 I think that "toolbar_text" is a more consistent name (considering the other "toolbar_*" theme color properties we support), and so I would personally consider it more like a default value (to be used for all toolbar text, including the bookmarks if there is no "bookmark_text" that overrides it specifically for the bookmarks) instead of considering the "bookmark_text" and "toolbar_text" as aliases for each other.

(In reply to Luca Greco [:rpl] [:luca] from comment #18)

Based on comment 10, we should proceed on ripping off the deprecated aliases in Firefox 69, and so pretty soon.

We leaved this issue open to attach and then land the patch that will remove the deprecated aliases from the API schemas (which will then turn any usage of those aliases into errors raised by the linter on new addon submission, once the addons-linter will have imported the updated 69beta schemas).

Are we ready to proceed to the "API schema LWT aliases rip off"?
e.g. have we reached an agreement about what to do about toolbar_text? (if it has to stay or if it can go away with the other ones)

There doesn't seem to be an agreement AFAIK.

Would you mind to proceed on creating the new patch?
(if no one can work on the new patch, I'm also willing to prepare and attach a new patch to this issue, but I would still need your help to decide what we want to do about toolbar_text).

Sorry, I don't have time to look into this right now.

Flags: needinfo?(ntim.bugs)
Flags: needinfo?(philipp)

Reading this issue, it seems Mike has already made a product decision and the arguments that have come after that did not add much counter points aside from consistency with other toolbar_ properties. I think we all agree that toolbar_text makes more sense and that the decision was mainly for compatibility reasons.

That said maybe it makes sense to revisit the premise on how much more difficult it is to make use of aliases. To recap from comment 0 and comment 3:

  • The schema is not capable of mutually exclusive properties or required aliases
  • The schema needs to be kept in sync in AMO and the browser
  • Documentation is slightly more difficult
  • theme.getCurrent() returns whatever the author specified, making aliases more difficult

It seems that we are making the decision to remove aliases mainly on technical merits, so let's try to see the developer and product perspective:

  • Aliases allow us to make the names easier to understand and more appropriate for the UI they are changing
  • At the same time, having aliases that are mutually exclusive if they don't have the same value is challenging for the product and the developer
  • We have other toolbar_ properties, so consistency in that regard is favorable
  • Supporting the Chrome names is essential for compatibility

Altogether, I feel that despite the potential downsides we'd be providing a better experience for developers if we allow the use of aliases, as long as we have a sufficient number of safeguards against mutually exclusive values. This would require improvements to the schema validation and linter though and I'd like to understand how difficult this really is.

My suggested changes:

  • Retain the toolbar_text alias to bookmark_text
  • Fix the validation to detect aliases correctly
  • Make theme.getCurrent() return both proper and aliased values, despite what the author specified
  • Provide some sort of automation or documentation that keeps us from having the schemas run out of sync
Flags: needinfo?(philipp)

Raising priority to P1 (especially given that this was something that should have been done in 69).

Type: enhancement → task
Priority: P2 → P1

Added the addons-linter pull request to the issue "See Also" field.

See Also: → https://github.com/mozilla/addons-linter/pull/2694
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/f7b52e107b78
Remove usage of deprecated lwt aliases from Fennec robocop tests. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/e746ce73765f
Remove usage of deprecated lwt aliases from telemetry tests. r=janerik
https://hg.mozilla.org/integration/autoland/rev/5bb4739662d3
Remove usage of deprecated lwt aliased from browser_ext_manager.js. r=zombie
https://hg.mozilla.org/integration/autoland/rev/87346cd601fa
Remove usage of deprecated lwt aliased from browser_webapi_theme.js. r=ntim,robwu
https://hg.mozilla.org/integration/autoland/rev/40d1a7318a94
Remove support for deprecated lwt aliases from WebExtensions theme API. r=ntim,robwu
Regressions: 1567684
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox70: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Regressions: 1567947
Depends on: 1570715

BCD needs to be updated:

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/theme#Colors

  • accentcolor should be marked as unsupported since 70, referring to "frame" instead.
  • textcolor should be marked as unsupported since 70, referring to "tab_background_text" instead.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/theme#Imag

  • headerURL should be marked as unsupported since 70, referring to "theme_frame" instead.
  • A new row needs to be added, called "theme_frame".

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/theme#images

  • The theme_frame section contains "Alias for headerURL, provided for Chrome compatibility.".
    Starting from Firefox 70, this is the only supported property. The theme_frame and headerURL rows needs to be merged in a meaningful way.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/theme#Chrome_compatibility

  • The section misleadingly suggests that headerURL, accentcolor and textcolor are supported and recommended aliases of the theme properties in Chrome. The only valid property in the table is "toolbar_text". I suggest to remove the table, and put the version-specific notes in the BCD or other sections in the MDN page.
Keywords: dev-doc-needed

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #35)

The theme's browser-compat-data should also be updated:

I will start on it later today.

Tracked in MDN/Sprints #2061

Hello,

Will this fix require manual validation? If yes, please provide some steps to reproduce in order to correctly test it and also, please set the "qe-verify+" flag. Otherwise, could the "qe-verify-" flag be added? Thanks!

Flags: needinfo?(lgreco)
Flags: needinfo?(lgreco) → qe-verify-

Docs work has been done; see https://github.com/mdn/sprints/issues/2142 for a description of exactly what we did. Please can I have a review?

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: