Closed Bug 1643776 Opened 4 years ago Closed 4 years ago

Pre-install a new colorful theme (Alpenglow) to be used a fourth option in multistage about:welcome theme screen

Categories

(Firefox :: Messaging System, enhancement, P1)

Product:
Firefox
Component:
Messaging System
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
81 Branch
Iteration:
81.2 - Aug 10 - Aug 23
Tracking Flags:
Tracking Status
firefox78 --- unaffected
firefox79 --- wontfix
firefox80 --- wontfix
firefox81 --- verified

People

(Reporter: pdahiya, Assigned: dmosedale)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

firefox-alpenglow.zip
5.50 KB, application/zip
Details
Bug 1643776 - Add Alpenglow theme, r?pdahiya
47 bytes, text/x-phabricator-request
Details | Review
Alpenglow selected theme while onboarding
352.77 KB, image/png
Details
Dark Icon for Customize Mode
3.54 KB, image/svg+xml
Details
Light Icon for Customize Mode
3.61 KB, image/svg+xml
Details
Alpenglow Preview - Light
4.25 KB, image/svg+xml
Details
Alpenglow Screenshots for UI Review.zip
1.68 MB, application/zip
Details

Scope of this bug is to pre-install a third theme so that its available to be shown on theme screen for multistage about:welcome 79 v2 experiment users.

Aaron is working on getting this new theme done

Assignee: nobody → pdahiya
Iteration: --- → 79.1 - June 1 - June 14
status-firefox78: --- → unaffected
status-firefox79: --- → affected
Priority: -- → P1

NI Dao to help answer scope of work in terms of complexity to add a new theme packaged in Firefox 79 similar to https://searchfox.org/mozilla-central/source/browser/themes/addons. Please note click of theme tile in above figma prorotype, supports theme viewer and changes theme of 'Choose a look' screen

For 79 onboarding experiment, we would like to show the new theme only during onboarding and need not surface it under about:addon#themes and Menu -> Customize -> Themes.

Thanks!

Flags: needinfo?(dao+bmo)

(In reply to Punam Dahiya [:pdahiya] from comment #2)

NI Dao to help answer scope of work in terms of complexity to add a new theme packaged in Firefox 79 similar to https://searchfox.org/mozilla-central/source/browser/themes/addons. Please note click of theme tile in above figma prorotype, supports theme viewer and changes theme of 'Choose a look' screen

You can add themes in browser/themes/addons, they won't automatically be shown to the user. To expose the theme to the addon manager, call AddonManager.maybeInstallBuiltinAddon: https://searchfox.org/mozilla-central/rev/35b97af64a55d1d30caa4d6e9fabc1a7fbabc509/browser/components/BrowserGlue.jsm#1321-1330
After that your code can select the theme by calling theme.enable() like here: https://searchfox.org/mozilla-central/rev/35b97af64a55d1d30caa4d6e9fabc1a7fbabc509/browser/components/customizableui/CustomizeMode.jsm#1528

For 79 onboarding experiment, we would like to show the new theme only during onboarding and need not surface it under about:addon#themes and Menu -> Customize -> Themes.

So you want to package the theme with Firefox, but then not offer a way to use it besides onboarding? That seems a bit weird (as in limiting and wasteful) to me.

(In reply to Punam Dahiya [:pdahiya] from comment #1)

Figma link displaying choose a theme onboarding screen

https://www.figma.com/proto/KqQZwwLhft9cUIcBtMVmbz/Desktop-First-Run?node-id=122%3A973&viewport=-128%2C296%2C0.1449243128299713&scaling=contain

You'll have the somehow handle the case of the OS being in dark mode, in which case "default" isn't really available (it automatically switches to the dark theme).

Flags: needinfo?(dao+bmo)

For 79 onboarding experiment, we would like to show the new theme only during onboarding and need not surface it under about:addon#themes and Menu -> Customize -> Themes.

So you want to package the theme with Firefox, but then not offer a way to use it besides onboarding? That seems a bit weird (as in limiting and wasteful) to me.

+1, for experiment in 79 we have decided to keep scope to existing preinstalled themes (light and dark) and when we are ready to add a new theme in onboarding, have it available everywhere.

You'll have the somehow handle the case of the OS being in dark mode, in which case "default" isn't really available (it automatically switches to the dark theme).

Instead of default, showing two options as light and dark theme should help resolve this case

Iteration: 79.1 - June 1 - June 14 → 79.2 - June 15 - June 28
Iteration: 79.2 - June 15 - June 28 → ---
Priority: P1 → P2
Summary: Pre-install a new theme to be used a third option in multistage about:welcome theme screen → Pre-install a new colorful theme to be used a fourth option in multistage about:welcome theme screen

Adding link to colorful theme https://addons.mozilla.org/en-US/firefox/addon/firefox-alpenglow/, that should be pre-installed in firefox.

Scope of this bug is to pre-install new theme in firefox - https://searchfox.org/mozilla-central/source/browser/components/BrowserGlue.jsm#1275

and display it as fourth option in 'Choose a look' screen in onboarding flow.

Onboarding flow can retrieve this pre-installed theme using AddonManager.getAddonByID method

https://searchfox.org/mozilla-central/source/browser/components/newtab/aboutwelcome/AboutWelcomeParent.jsm#197

Priority: P2 → P1

NI shorlander to help share colorful theme (https://addons.mozilla.org/en-US/firefox/addon/firefox-alpenglow/,) assets similar to light theme below. Thanks!

https://searchfox.org/mozilla-central/source/browser/themes/addons/light

Flags: needinfo?(shorlander)

Would there be any engineering support available from the add-on team to get this new theme landed in Firefox?

Flags: needinfo?(atsay)

Redirecting Amy's NI to Stuart.

FYI, I'm not sure if this falls under the WebExtensions team's purview.

Flags: needinfo?(atsay) → needinfo?(scolville)

(In reply to Meridel from comment #7)

Would there be any engineering support available from the add-on team to get this new theme landed in Firefox?

Do you have any specific questions that need answering? That should point the way to who is best to help with this.

Flags: needinfo?(scolville) → needinfo?(mwalkington)

Thanks Stuart, NI Luca to help answer below questions, please feel free to redirect if someone else can help answer

a) Whats the scope of pre-installing a colorful theme https://addons.mozilla.org/en-US/firefox/addon/firefox-alpenglow/ in firefox assuming we don't want to surface this new theme in about:addons and customize -> themes

I see below places where we should be updating
https://searchfox.org/mozilla-central/source/browser/components/BrowserGlue.jsm#1275
https://searchfox.org/mozilla-central/source/browser/themes/addons/light
https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/aboutaddons.js#87
https://searchfox.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/LightweightThemes.jsm#31

Are there other touchpoints? Will be great to get high level guidance on scope here

This theme should surface alongside Default, Light and Dark in 'Choose a look' screen during onboarding and enabled if user selects
https://searchfox.org/mozilla-central/source/browser/components/newtab/aboutwelcome/AboutWelcomeParent.jsm#197

b) Can we retrieve files needed for new theme from AMO side?

Flags: needinfo?(mwalkington) → needinfo?(lgreco)

(In reply to Punam Dahiya [:pdahiya] from comment #10)

Thanks Stuart, NI Luca to help answer below questions, please feel free to redirect if someone else can help answer

a) Whats the scope of pre-installing a colorful theme https://addons.mozilla.org/en-US/firefox/addon/firefox-alpenglow/ in firefox assuming we don't want to surface this new theme in about:addons and customize -> themes

I see below places where we should be updating
https://searchfox.org/mozilla-central/source/browser/components/BrowserGlue.jsm#1275
https://searchfox.org/mozilla-central/source/browser/themes/addons/light
https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/aboutaddons.js#87
https://searchfox.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/LightweightThemes.jsm#31

Are there other touchpoints? Will be great to get high level guidance on scope here

Follows some details about the parts that would need changes and what the purpose of that change is.
Let me know if this helps and if you have more doubts or questions (or issues with this details that are blocking you).

BrowserGlue.jsm

Based on my current understanding (derived by reading the other comments in this issue) I think that in BrowserGlue.jsm this theme should be handled a bit differently, if you call AddonManager.maybeInstallBuiltinAddon for this theme unconditionally, the theme will be listed in the available ones even if it hasn't been selected during the about:welcome onboarding wizard.

I personally think it would be totally fine, if we are ok to have that theme listed by default as all other builtin themes, but if we are not, then the change in BrowserGlue.jsm should only be called if the theme is already installed, with the purpose to update it to a more recent version if necessary when the browser is updated.

browser/themes/addons

In this directory, a new sub-directory should be created for the new builtin theme, and inside that directory the https://addons.mozilla.org/en-US/firefox/addon/firefox-alpenglow/ xpi should be unpacked and after that:

  • a new entry should be added to the DIRS variable in the browser/themes/addons/moz.build file
  • a new moz.build file should be added in the new theme sub-directory, and based on the current content of the xpi it should look like more or less like:
EXTRA_JS_MODULES.themes['theme-dirname'] += [
    'icon.svg',
    'manifest.json',
]

EXTRA_JS_MODULES.themes['theme-dirname']["images"] += [
    'images/*.svg',
]
  • to the theme manifest.json file, we should also add (as done for the other builtin themes):
    • an explicit extension id using the applications.gecko.id manifest property (something like firefox-themename@mozilla.org)
    • an icon (by adding something like "icons": { "32": "icon.svg" }), which is what will be shown in the customize mode theme selector
      (otherwise it would show a default theme icon)
    • a description property

I see that the theme name is currently "Firefox Radiance", that name will be visible in the customize mode theme selector and in about:addons, we should change it if that isn't the name that should be visible there.

toolkit/mozapps/extensions/content/aboutaddons.js

The change in aboutaddons.js is needed to associate a screenshot to the builtin theme.
Besides that, there shouldn't be any additional changes needed on the about:addons page internals.

browser/tools/mozscreenshots/mozscreenshots/extension/configurations/LightweightThemes.jsm

I'm not sure about this part, I guess that listing a builtin theme there may be required to take some screenshot automatically, but that is just a guess, we should double-check that with someone with more direct knowledge about what is the purpose of this jsm.

browser/components/newtab/aboutwelcome/AboutWelcomeParent.jsm

In this jsm module I think that we may need to just special case this particular theme and before trying to enable it we should be sure that it is installed.

AddonManager.maybeInstallBuiltinTheme takes care of also checking if the theme is already installed and only need to be updated, but unfortunately it doesn't return the resulting addon wrapper, not a promise to wait that the addon has been completely installed and so calling AddonManager.getAddonByID right after calling AddonManager.maybeInstallBuiltinAddon is not going to return an addon wrapper as you would expect.

As an alternative approach, AboutWelcomeParent.jsm could check if the addon is already installed using AddonManager.getAddonByID and if it is not it could call AddonManager.installBuiltinAddon which return a promise that resolves to the addon wrapper (on which we can call the enable method and have the theme enabled as for the other builtin themes).

Then I guess that the browser.aboutwelcome.overrideContent pref would also need to be updated to list this additional theme.

This theme should surface alongside Default, Light and Dark in 'Choose a look' screen during onboarding and enabled if user selects
https://searchfox.org/mozilla-central/source/browser/components/newtab/aboutwelcome/AboutWelcomeParent.jsm#197

b) Can we retrieve files needed for new theme from AMO side?

If you want or need it to be a builtin, I don't think so, at least is not something that I think we have ever done before and so it is not unlikely that to achieve it we may have to evaluate what kind of changes should be done to the related internals.

It should be possible to install the theme from a AMO url, e.g. I recall that some times ago activity-stream used to support installing extensions as part of the onboarding wizard (I'm not sure if it is still something that is still supported by activity-stream, but from AboutWelcomeParent it would be definitely possible to install an extension from a given AMO url).

By default installing the theme from an AMO url would show a confirmation doorhanger (a doorhanger with a Add and Cancel buttons, no permissions to accept given that it would be a pure theme add-on without any js code in it), it may be possible to workaround that, but it would likely be an hack without apply changes to make it possible without any crude workaround.

Flags: needinfo?(lgreco)

Thanks Luca for detailed info, this is very helpful, I might have more questions as I work on the patch

BrowserGlue.jsm

Based on my current understanding (derived by reading the other comments in this issue) I think that in BrowserGlue.jsm this theme should be handled a bit differently, if you call AddonManager.maybeInstallBuiltinAddon for this theme unconditionally, the theme will be listed in the available ones even if it hasn't been selected during the about:welcome onboarding wizard.

I can run it by product, If this is easier I think we should make this new theme available everywhere by installing it unconditionally as all other builtin themes

I personally think it would be totally fine, if we are ok to have that theme listed by default as all other builtin themes, but if we are not, then the change in BrowserGlue.jsm should only be called if the theme is already installed, with the purpose to update it to a more recent version if necessary when the browser is updated.

Attached file firefox-alpenglow.zip —
Flags: needinfo?(shorlander)
Assignee: pdahiya → dmose

(In reply to Punam Dahiya [:pdahiya] from comment #1)

Figma link displaying choose a theme onboarding screen

https://www.figma.com/proto/KqQZwwLhft9cUIcBtMVmbz/Desktop-First-Run?node-id=122%3A973&viewport=-128%2C296%2C0.1449243128299713&scaling=contain

This figma link doesn't seem to point to anything related to theme choice; am I missing something?

Flags: needinfo?(pdahiya)

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #14)

(In reply to Punam Dahiya [:pdahiya] from comment #1)

Figma link displaying choose a theme onboarding screen

https://www.figma.com/proto/KqQZwwLhft9cUIcBtMVmbz/Desktop-First-Run?node-id=122%3A973&viewport=-128%2C296%2C0.1449243128299713&scaling=contain

This figma link doesn't seem to point to anything related to theme choice; am I missing something?

yes, the link is outdated. Here's the link with new colorful theme design specs. Thanks!

https://www.figma.com/file/KqQZwwLhft9cUIcBtMVmbz/Desktop-First-Run?node-id=122%3A973

Flags: needinfo?(pdahiya)

I don't quite follow the intent of the last sentence here, Punam, can you unpack the meaning a bit?

(In reply to Punam Dahiya [:pdahiya] from comment #12)

Thanks Luca for detailed info, this is very helpful, I might have more questions as I work on the patch

BrowserGlue.jsm

Based on my current understanding (derived by reading the other comments in this issue) I think that in BrowserGlue.jsm this theme should be handled a bit differently, if you call AddonManager.maybeInstallBuiltinAddon for this theme unconditionally, the theme will be listed in the available ones even if it hasn't been selected during the about:welcome onboarding wizard.

I can run it by product, If this is easier I think we should make this new theme available everywhere by installing it unconditionally as all other builtin themes

Flags: needinfo?(pdahiya)

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #16)

I don't quite follow the intent of the last sentence here, Punam, can you unpack the meaning a bit?

(In reply to Punam Dahiya [:pdahiya] from comment #12)

Thanks Luca for detailed info, this is very helpful, I might have more questions as I work on the patch

BrowserGlue.jsm

Based on my current understanding (derived by reading the other comments in this issue) I think that in BrowserGlue.jsm this theme should be handled a bit differently, if you call AddonManager.maybeInstallBuiltinAddon for this theme unconditionally, the theme will be listed in the available ones even if it hasn't been selected during the about:welcome onboarding wizard.

I can run it by product, If this is easier I think we should make this new theme available everywhere by installing it unconditionally as all other builtin themes

My understanding from Luca feedback is, using AddonManager.maybeInstallBuiltinAddon for this theme unconditionally in BrowserGlue.jsm will result in surfacing this theme as default builtin theme at all places (about:addons, customize -> theme and onboarding) . If it makes implementation easier we should use this approach and share with product managers of these respective modules.

Flags: needinfo?(pdahiya)
Iteration: --- → 81.1 - July 27 - Aug 09
Iteration: 81.1 - July 27 - Aug 09 → 81.2 - Aug 10 - Aug 23

Aaron, here's a needinfo about getting both a dark and a light icon for the about:welcome installer. If you're pressed for time, I imagine one could be good enough.

Flags: needinfo?(abenson)
Attached file Alpenglow Screenshots for UI Review (obsolete) —

Adding screenshots for UI review with new Alpenglow theme. Thanks!

Flags: needinfo?(shorlander)
Blocks: 1659871

As discussed with @dmose filed bug 1659871 for tests and follow up performance improvements. Thanks!

Flags: needinfo?(shorlander)

Thanks @shorlander, updated screenshots with final images

Attachment #9170827 - Attachment is obsolete: true
Pushed by pdahiya@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ffb6196daf7
Add Alpenglow theme r=dmose,rpl,flod
Pushed by pdahiya@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a1fa73b1a63
Add Alpenglow theme r=dmose,rpl,flod

Fixed test browser_all_files_referenced.js, and re-landed patch after adding a trailing slash in the exception path for alpenglow images. Thanks

Flags: needinfo?(dmose)
Flags: needinfo?(abenson)

So the problem here is that the error doesn't seem to reproduce either locally or on try, even when we're explicitly running the test that all files are referenced. So I have a theory about a thing that has some reasonable chance of working, and as far as we can tell, there's no known way to test it without actually landing it. If this blows up again, my apologies to the sheriffs.

Flags: needinfo?(pdahiya)
Attachment #9168254 - Attachment description: Bug 1643776 - Add Alpenglow theme → Bug 1643776 - Add Alpenglow theme, r?pdahiya
Regressions: 1660323
Pushed by pdahiya@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f4bf8bab6cf
Add Alpenglow theme, r=dmose,rpl,flod
Pushed by pdahiya@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/edf8192f88f9
Add Alpenglow theme, r=dmose,rpl,flod
Regressions: 1660557

https://hg.mozilla.org/mozilla-central/rev/edf8192f88f9

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox81: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

status-firefox80: --- → ?

I have verified the following using Firefox Nightly 81.0a1 (Build ID: 20200824094458) on Windows 10 x64, macOS 10.15, and Ubuntu Linux 18.04.

  • The 4th theme option, "Firefox Alpenglow" is correctly displayed on the "Choose a look" slide from the "about:welcome" page.
  • The "Firefox Alpenglow" is a 4th default option in the "Themes" section from the "about:addons" page.
  • The "Firefox Alpenglow" option is available in the "Themes" menu of the "Customize Nightly" page. This can be observed by doing the following:
    1. Open the hamburger menu located in the top right corner of the browser.
    2. Select the "Customize..." option.
    3. Click the "Themes" dropdown menu located at the bottom right side of the page and observe the displayed theme options.
  • The "Firefox Alpenglow" is displayed correctly if the OS theme is set to Light.
  • The "Firefox Alpenglow" is displayed correctly if the OS theme is set to Dark.
Status: RESOLVED → VERIFIED
status-firefox81: fixed → verified

Marking this feature as won't fix for 80. Thanks

status-firefox80: ? → wontfix
Depends on: 1660557
No longer regressions: 1660557
Summary: Pre-install a new colorful theme to be used a fourth option in multistage about:welcome theme screen → Pre-install a new colorful theme (Alpenglow) to be used a fourth option in multistage about:welcome theme screen
Regressions: 1698725
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: