1567684 - Some themes are broken by the removal of the deprecated properties

Status: VERIFIED FIXED

(WebExtensions :: Developer Outreach, defect, P2)

Description

[u643549]

Attachment: bugzilla.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

Updated to 70.0a1 and issue arose after restarting

Successfully reproduced in new profile using preset themes in FireFox Color. Tab bar theming is broken.

Actual results:

Tab bar defaults to white background despite FireFox Color.

Expected results:

Theme should have followed prior configuration from FireFox Color

Comment 1

[Asif Youssuff]

9:19.73 INFO: No more inbound revisions, bisection finished.
9:19.73 INFO: Last good revision: 6332d065c0486b815b95f57d5e9cd51d11c03099
9:19.73 INFO: First bad revision: 40d1a7318a944bcb1af9b48b52a2fc3a00c11248
9:19.73 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6332d065c0486b815b95f57d5e9cd51d11c03099&tochange=40d1a7318a944bcb1af9b48b52a2fc3a00c11248

Luca, your patch seems to cause this regression. Can you please look into this?

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Gradientus_screenshot.png

I see a similar problem with the Gradientus dynamic theme extension. The unfocused tabs' titles are very hard to read because they are black text on a color background instead of gray text. See the attached screenshot.

https://addons.mozilla.org/en-US/firefox/addon/gradientus

https://github.com/cynthiapereira/webextension-gradientus

Comment 3

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

This is a regression in 70 Nightly.

The bug reporter saw this bug on Windows and I see it on macOS.

Comment 4

[Tim Nguyen :ntim]

Hi Andreas,

Would it be possible to run a mass migration for broken static themes (eg. Rainbow blur ? I think it makes sense given that the AMO theme editor used the deprecated theme property names at some point.

As for dynamic themes, would it be possible to email all the relevant developers ?
It would also be nice to send PRs/file issues to any repository we can find, eg.: https://github.com/search?q=accentcolor+filename%3Abackground.js+language%3AJavaScript&type=Code (I believe mdn/webextension-examples is part of those results)

As for Firefox Color, I thought this was already handled, but maybe there's some storage migration that needs to be done too.

Comment 5

[Tim Nguyen :ntim]

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

As for Firefox Color, I thought this was already handled, but maybe there's some storage migration that needs to be done too.

Regarding Firefox Color, there was some WIP regarding this but it was never merged: https://github.com/mgillespy/FirefoxColor/tree/763-update-deprecated-properties

Comment 6

[Andreas Wagner [:TheOne] [use NI]]

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

Hi Andreas,

Would it be possible to run a mass migration for broken static themes (eg. Rainbow blur ? I think it makes sense given that the AMO theme editor used the deprecated theme property names at some point.

Most static themes have been migrated a few months back. We might re-run the migration command to migrate the stragglers once https://github.com/mozilla/addons-linter/pull/2694 is live.

As for dynamic themes, would it be possible to email all the relevant developers ?
It would also be nice to send PRs/file issues to any repository we can find, eg.: https://github.com/search?q=accentcolor+filename%3Abackground.js+language%3AJavaScript&type=Code (I believe mdn/webextension-examples is part of those results)

I am not sure whether we have the capacity to do so at the moment, both for identifying relevant developers, sending out communication and handling the feedback we get (which, from past experience, can be of significant volume). Deferring to Jorge and Caitlin, on whether we should make time for this.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Luca, your patch seems to cause this regression. Can you please look into this?

(as already mentioned in Bug 1567684 comment 4 and Bug 1567684 comment 5) the regressed themes are still using the deprecated LWT aliases.

Unfortunately the addons-linter rules are not going to raise any linting errors or warnings when a dynamic theme is using the deprecated LWT aliases.

The theme API is also often called by passing theme objects defined elsewhere (instead of being passed as literal object), which doesn't help to be able to validate the parameter as part of an eslint rule (e.g. Firefox Color calls browser.theme.updates using theme objects stored in the storage.local API, gradientus calls browser.theme.updates with theme objects part of a themes object literal defined in the background page)

Regarding Firefox Color, there was some WIP regarding this but it was never merged: https://github.com/mgillespy/FirefoxColor/tree/763-update-deprecated-properties

It seems to be also tracked by the Firefox Color issue:

• Update properties that will be deprecated in Firefox 69 #763
  (and there is also the duplicate issue: Update for Firefox 69? #798)

And the related Firefox Color pull request (not merged yet):

• Fixes 763: update properties deprecated in Firefox 69 #803

Comment 8

[Chris Peterson [:cpeterson]]

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

As for dynamic themes, would it be possible to email all the relevant developers ?
It would also be nice to send PRs/file issues to any repository we can find, eg.: https://github.com/search?q=accentcolor+filename%3Abackground.js+language%3AJavaScript&type=Code (I believe mdn/webextension-examples is part of those results)

I filed an issue for the Gradientus dynamic theme describing the fix:

https://github.com/cynthiapereira/webextension-gradientus/issues/10

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

We discussed what to do here. Caitlin will add details.

Comment 10

[Caitlin Neiman [:caitmuenster]]

Hey all! Here's the plan:

• :eviljeff will re-run a script to update themes on AMO
• We'll publish a post on the Add-ons Blog about deprecated theme properties
• We'll also email developers of dynamic themes to let them know about deprecated theme properties

Re: Color -- Melanie is no longer working on the project and her PR needs a bit of updating before it can be merged. If someone would like to get that over the finish line, please go ahead and do so! I am still looking for a resource to pick up the rest of the Color work.

Comment 11

[Tim Nguyen :ntim]

I've updated some of my old Mozilla Hacks blog posts to use the new property names.

There are also a bunch of docs on MDN that need updating, I only found those, but there might be more:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/Themes
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/Themes/Theme_concepts
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/theme

Chris, do you know someone on your team who could take care of updating those ?

Comment 12

[Tim Nguyen :ntim]

There was some incorrect information on those MDN pages, notably that:

• you needed to use frame and frame_inactive instead of accentcolor when only the latter is needed
• the alias for textcolor was tab_text when it's actually tab_background_text

Please put me/someone familiar with the topic in the loop for technical review next time these docs are updated.

It also mentioned the following incorrect info, but that's our fault for the short notice:

• The properties were removed in Firefox 70 as opposed to 69
• toolbar_text is no longer deprecated/removed

Anyway, I've updated the doc regarding manifest.json to fix this incorrect info. Chris, would it be possible to get an editorial review ? Also, the 2 other docs I mentioned (and potentially others) have to be updated as well.

Thanks!

Comment 13

[Chris Mills [:cmills]]

We missed this one! Thanks for bring it to our attention Tim!

Irene is the responsible person on my team for WebExt docs. Irene, can you please update the required docs and review Tim's changes? Thanks!

Comment 14

[Irene Smith]

I will take care of it. I'm tracking these tasks in:

MDN/Sprints #1867

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

:eviljeff can you let us know here in a comment when you've run the script?
Caitlin can you also circle back and comment with the link when you've posted?
And, Irene, if you could do the same please :)

After that I think we can close this issue.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See What Do You Triage for more information

Comment 18

[Andrew Williamson [:eviljeff]]

I haven't been involved in running the script this time (I wrote the script and ran it the first time a few months ago).

Comment 19

[Irene Smith]

(In reply to Liz Henry (:lizzard PTO till Aug 10 n-i to :pascald instead) from comment #16)

:eviljeff can you let us know here in a comment when you've run the script?
Caitlin can you also circle back and comment with the link when you've posted?
And, Irene, if you could do the same please :)

After that I think we can close this issue.

Of course. I will post on this bug as soon as I'm done.

Comment 20

[Caitlin Neiman [:caitmuenster]]

Liz, the transactional email has gone out and the blog post has been published: https://blog.mozilla.org/addons/2019/07/25/upcoming-deprecations-in-firefox-70/

:eviljeff, sorry I think we didn't loop you in. Can you run the script again to catch themes built since April that use the deprecated properties?

Comment 21

[Andreas Wagner [:TheOne] [use NI]]

The migration script finished. 66 add-ons failed migration.

Out of those:

• 59 failed because they were set to invisible, so we didn't touch them.
• 4 failed because their manifest was invalid, most likely they added comments in the JSON file, which is officially not allowed.
• 3 failed because of a timeout to the signing endpoint

Out of the 7 non-disabled ones, only https://reviewers.addons.mozilla.org/en-US/firefox/addon/arc-dark-theme-we/ has more than just a few users. Caitlin, I'll leave it up to you whether you want to reach out to them manually.

Comment 22

[Andrew Williamson [:eviljeff]]

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

• toolbar_text is no longer deprecated/removed

AMO is using bookmark_text rather than toolbar_text when generating themes in the wizard, should we change it? If toolbar_text is the new default we can use that instead. (Is it a problem we repacked all the themes and replaced toolbar_text?)

Comment 23

[Tim Nguyen :ntim]

(In reply to Andrew Williamson [:eviljeff] from comment #22)

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

• toolbar_text is no longer deprecated/removed

AMO is using bookmark_text rather than toolbar_text when generating themes in the wizard, should we change it? If toolbar_text is the new default we can use that instead. (Is it a problem we repacked all the themes and replaced toolbar_text?)

If I understand correctly, in bug 1472740, we decided to not deprecate toolbar_text since it reflects better what's styled, but also keep support for bookmark_text since it's Chrome's name.

I don't know what's preferred as "new default" for AMO themes though. Forwarding question to Philipp.

Comment 24

[Caitlin Neiman [:caitmuenster]]

Thanks Andreas -- I'd like to reach out to the folks whose themes failed migrations. Can you send me a list of their email addresses & URLs to the themes that failed?

Comment 25

[Andreas Wagner [:TheOne] [use NI]]

(In reply to Caitlin Neiman [:caitmuenster] from comment #24)

Thanks Andreas -- I'd like to reach out to the folks whose themes failed migrations. Can you send me a list of their email addresses & URLs to the themes that failed?

Shared in a spreadsheet.

Comment 26

[Dão Gottwald [:dao]]

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

(In reply to Andrew Williamson [:eviljeff] from comment #22)

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

• toolbar_text is no longer deprecated/removed

AMO is using bookmark_text rather than toolbar_text when generating themes in the wizard, should we change it? If toolbar_text is the new default we can use that instead. (Is it a problem we repacked all the themes and replaced toolbar_text?)

If I understand correctly, in bug 1472740, we decided to not deprecate toolbar_text since it reflects better what's styled, but also keep support for bookmark_text since it's Chrome's name.

I don't know what's preferred as "new default" for AMO themes though. Forwarding question to Philipp.

toolbar_text is preferred since it's more accurate. We don't want to confuse theme authors. bookmark_text only exists for Chrome compatibility.

Comment 28

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Caitlin Neiman [:caitmuenster] from comment #10)

Re: Color -- Melanie is no longer working on the project and her PR needs a bit of updating before it can be merged. If someone would like to get that over the finish line, please go ahead and do so! I am still looking for a resource to pick up the rest of the Color work.

Hey Caitlin,
I took Melanie's PR and updated it a little bit (mainly to bring back the toolbar_text property, given that is not on the deprecation path anymore):

• https://github.com/mozilla/FirefoxColor/pull/819

I also briefly tried the fixed version locally and I noticed that the theme property normalization is happening on the "companion website" side, and so to fully fix the issue, once the PR has been merged, we must be sure to both publish the updated xpi and update the color.firefox.com "companion website".

In the PR I already asked ntim to give one last round of review to the PR (and then merge it, as I don't have any permission in that github repository).

Would you mind to ensure that who is managing the companion website is going to be aware that we will need to deploy the updated version (once the PR has been accepted and merged)?

(and let me know if there is anything else that I can do to help this Firefox Color issue to be fully closed).

Comment 29

[Caitlin Neiman [:caitmuenster]]

Luca, you are the best! Thank you!

Yes, as soon as the PR is merged we'll deploy the updated version. We currently don't have anyone staffed to run deployments. I think I can do it but it would be good to have a buddy in case something goes wrong.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Talked with Caitlin; a patch for this issue is on the way.

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

I'm thinking this might need a release note for beta 70 with a link to the blog post. Can you suggest wording for the release note?

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Here's what I've got now: Aliased theme properties have been removed, which may affect some dynamic themes. (with a link to the post)

Comment 33

[Caitlin Neiman [:caitmuenster]]

Love it. This may also affect some non-dynamic themes, so we might want to just say "which may affect some themes" and then link to https://blog.mozilla.org/addons/2019/07/25/upcoming-deprecations-in-firefox-70/.

Comment 35

[Rob Wu [:robwu]]

Firefox Color 2.1.5 was published yesterday. This has been fixed.

If your themes are broken after updating to Firefox 70.0b, check whether you're using the latest version of Firefox Color. The "Check for updates" menu item at about:addons can be used to check for updates.

Comment 36

[Alex]

Thank you very much, Rob. That fixed the issue! No more blinding white light at night.

Comment 37

[Miruna Curtean]

Verified the fix using the latest Nightly (70.0a1/20190901214807) under Windows 10 Pro 64-bit and macOS High Sierra 10.13.6 using the Firefox Color and the Gradient extensions.
The themes display in the toolbar as expected using the preset themes, custom colors or custom backgrounds.

Comment 38

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I believe my input is no longer needed here, sorry for the late reply. What I mentioned in bug 1472740 comment 24 is still valid, let me know if you need more info.