1406454 - (themingapi-android) [meta] Get themes working on Android

Status: NEW

(GeckoView :: Extensions, enhancement, P3)

Description

[Andy McKay]

In Firefox 58 Nightly, installing a static or dynamic theme does not work.

If we intend to replace lightweight themes on Android, we should make it work, or drop support.

Comment 1

[Nevin Chen(Not active on Bugzilla)]

I can reproduce this.
I need to re-open the app to make it work.
Please ni Wesly if you need support.

Comment 2

[Jan Henning [:JanH]]

Lightweight theme installation is working fine for me on Nightly.

This bug is about implementing some sort of support for the successor to lightweight themes, so there's no regression window to be found.

Comment 3

[Tim Nguyen :ntim]

Andy, does any WE theme work if you toggle `extensions.webextensions.themes.enabled` to `true` on Android ?

Comment 4

[Wesly Huang (EPM)]

Joe, Andreas, should this be put in the roadmap discussion in Austin?

Comment 5

[Andy McKay]

After flipping the pref, I was able to install both static and dynamic themes. However neither of them seem to do anything, the Android UI remains white and photon-y sadly.

Comment 6

[Tim Nguyen :ntim]

Thanks Andy! Confirms what I'm seeing as well.

I just did a bit of debugging, and I figured out why this wasn't working: seems like the URL class doesn't accept moz-extension:// URIs.

Installing a WebExtension theme seems to be hitting this codepath: https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/BitmapUtils.java#95

Comment 7

[Andreas Bovens [:abovens]]

Sure, we can discuss it there.

It would also be interesting to see how this clashes with / relates to support for <meta name="theme-color" content="#000"> (which we don't support at present).

Comment 8

[Tim Nguyen :ntim]

Can someone make a decision regarding what to do here ? Can Fennec implement basic lightweight theme parity or should it drop theme support altogether ?

I've quickly looked into the issues preventing basic WE themes from working (since WE themes re-use the lightweight theme infrastructure):

- Fixing headerURL to support moz-extension:// URIs on Android
- Making headerURL optional (use a transparent bitmap if only textcolor/accentcolor is specified)


Here's a summary of the work to be done to drop existing theme support:
- Add a warning message that the user's existing lightweight themes will be removed
- Add some code on the Add-ons Manager to remove the user's existing themes at a certain release
- Remove/hide the Fennec Add-ons Manager theme section
- Remove all the code related to it (that includes LightweightThemeConsumer and related Java code)
- Disable "Add to Firefox" button for themes on Android


Right now, Android webextension theme support is in a broken state, we allow users to install them, but they don't work at all even though they are shown in the Add-ons Manager. This isn't a big issue right now, but it will be a big issue when AMO migrates existing lightweight themes over to WebExtension static themes (which is imminent as far as I know).

Comment 9

[Jan Henning [:JanH]]

(In reply to Tim Nguyen :ntim from comment #8)
> Can Fennec implement basic lightweight theme parity or should it drop theme support altogether?
I'm still no fan of the redesigned URL bar, so I'd vote strongly against removing support, but then I'm not the one to decide this.

> - Fixing headerURL to support moz-extension:// URIs on Android
> - Making headerURL optional (use a transparent bitmap if only
> textcolor/accentcolor is specified)

Getting that to work shouldn't be too hard (famous last words), although I think we only support accent colours, no text colours, but that's just one more thing to ignore I guess - do you have some sample themes (one with a bitmap, one without) I could use for testing?

> Here's a summary of the work to be done to drop existing theme support:
If this is meant as an either/or else with the above, this looks like more work than just getting moz-extension:// URIs to work from the Android app, plus it's of course unfriendly for everybody currently using a theme.


> (which is imminent as far as I know)

What timescale does imminent mean? Even if the fix for the above could be produced soon enough to be uplifted to Beta (assuming it is safe to do so), this still means that we wouldn't be ready until the end of October.

Comment 10

[Tim Nguyen :ntim]

(In reply to Jan Henning [:JanH] from comment #9)
> (In reply to Tim Nguyen :ntim from comment #8)
> > Can Fennec implement basic lightweight theme parity or should it drop theme support altogether?
> I'm still no fan of the redesigned URL bar, so I'd vote strongly against
> removing support, but then I'm not the one to decide this.
> 
> > - Fixing headerURL to support moz-extension:// URIs on Android
> > - Making headerURL optional (use a transparent bitmap if only
> > textcolor/accentcolor is specified)
> 
> Getting that to work shouldn't be too hard (famous last words), although I
> think we only support accent colours, no text colours, but that's just one
> more thing to ignore I guess - do you have some sample themes (one with a
> bitmap, one without) I could use for testing?

https://addons.mozilla.org/en-US/firefox/search/?type=statictheme

> > Here's a summary of the work to be done to drop existing theme support:
> If this is meant as an either/or else with the above, this looks like more
> work than just getting moz-extension:// URIs to work from the Android app,
> plus it's of course unfriendly for everybody currently using a theme.
> 
> 
> > (which is imminent as far as I know)
> 
> What timescale does imminent mean? Even if the fix for the above could be
> produced soon enough to be uplifted to Beta (assuming it is safe to do so),
> this still means that we wouldn't be ready until the end of October.

I'll :ddurst answer that.

Comment 11

[Andrew Williamson [:eviljeff]]

ni: jorge for info on the static theme launch on AMO and migration dates (not sure if there are official dates yet)

Comment 12

[Andreas Bovens [:abovens]]

Barbara and I are discussing this.

Comment 13

[Andreas Bovens [:abovens]]

Can we drop theme support, and instead, ship with a dark theme option?

Comment 14

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

The official launch on AMO is imminent (this week or next). The migration will most likely happen in Q4, but we don't have precise dates for it.

Comment 15

[Jan Henning [:JanH]]

(In reply to Tim Nguyen :ntim from comment #6)
> Thanks Andy! Confirms what I'm seeing as well.
> 
> I just did a bit of debugging, and I figured out why this wasn't working:
> seems like the URL class doesn't accept moz-extension:// URIs.
> 
> Installing a WebExtension theme seems to be hitting this codepath:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/
> main/java/org/mozilla/gecko/gfx/BitmapUtils.java#95

I looked a bit around how things work, and what happens for Lightweight Themes is that the theme initially contains an http(s)-URL for the image, which the Android front-end can load directly without problems. Meanwhile, Gecko has started persisting the image to disk. Once that has been finished, the LWT gets updated a second time, this time with the local file URI.

While the original idea behind persisting those images to disk might have been to avoid unnecessary network access and to make LWTs work offline as well, within the context of Webextension static themes on Android this mechanism has another benefit:
In the short-term, getting moz-extension URIs to work from the Android front-end is probably too involved anyway, but even longer term a solution might probably mean calling out to Gecko, unless we want to reimplement the whole logic for how to get from moz-extension URL to the actual location on disk in Android code as well. For themes however must be able to access the theme's image files as soon as the Android app starts up and don't want to wait until Gecko is ready, which'll likely take a few seconds more.
Continuing to persist the currently active theme's images into the profile folder is therefore a good workaround to allow access to those images from Android as early as possible.

In fact, we're doing this for Webextension static themes even now [1], but
- because for Webextensions ext-theme.js is supposed to be in full control (compare bug 1344926 comment 26), the LightweightThemeManager never broadcasts "lightweight-theme-styling-update" with the rewritten theme image URI after persisting the webextension's asset files
- even if I experimentally add the success callback, it never gets called - if I didn't make a mistake, presumably because the current conditions for the success callback to run assume that the file has been persisted from the network [2], and not a locally stored moz-extension

Since as per bug 1344926 comment 26 the LigthweightThemeManager.jsm is eventually supposed to be going away, I'd fix this by adding the persisting code for Webextensions to Android's LightweightThemeConsumer, since the consumer will be used even with Webextensions. So if we detect that the theme to be enabled refers to moz-extension URIs, we'd intercept that particular "lightweight-theme-styling-update", persist the referenced files and then notify the frontend using the rewritten file URIs.
A little more work might also be needed to make sure that for the transition phase with mixed installs of both LWTs and Webextension themes things work properly, i.e. that enabling one kind of theme properly disables any formerly active themes from the other category.

While not quite as easy as I originally imagined this to be, I'd think that the amount of work for this still doesn't compare too unfavourably to untangling our current Android theming support into separate Private Browsing and generic Dark Mode themes (not that that wouldn't be nice, either, but still).

[1] https://dxr.mozilla.org/mozilla-central/rev/5165e750ffabb930deb846410ab62dcc6d1f9e52/toolkit/mozapps/extensions/LightweightThemeManager.jsm#110
[2] https://dxr.mozilla.org/mozilla-central/rev/5165e750ffabb930deb846410ab62dcc6d1f9e52/toolkit/mozapps/extensions/LightweightThemeManager.jsm#1003

Comment 16

[Tim Nguyen :ntim]

Jan, thanks for this useful audit! Would you be interested on working in implementing those changes ? I can offer help regarding the WebExtensions stuff if needed.

Note that static themes just launched on AMO: https://blog.mozilla.org/addons/2018/09/20/future-themes-here/

Comment 17

[Jan Henning [:JanH]]

I'll try to find some time for it.