Closed Bug 1548776 Opened 5 years ago Closed 5 years ago

Multiple Firefox Add-ons themes are not working-themes are not applied

Categories

(Firefox for Android Graveyard :: Add-on Manager, defect)

Product:
Firefox for Android Graveyard
Component:
Add-on Manager
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox-esr68 verified, firefox66 wontfix, firefox67 wontfix, firefox68 wontfix, firefox69 wontfix, firefox70 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 70
Tracking Flags:
Tracking Status
firefox-esr68 --- verified
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- verified

People

(Reporter: mlobontiuroman, Assigned: JanH)

References

(
URL
)

Details

(Keywords: dev-doc-needed, regression)

Attachments

(2 files)

2019_05_03_11_24_01.mp4
1.25 MB, video/mp4
Details
Bug 1548776 - URL-decode all file names in GeckoJarReader. r?snorp
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-esr68+
Details | Review
Attached video 2019_05_03_11_24_01.mp4 —

Devices:

  • Google Pixel 3 XL (Android 9);
  • Samsung Galaxy Tab A6 (Android 7.0);
  • Samsung Galaxy Tab S3 (Android 8.0);
  • Nexus 5 (Android 6.0.1);
  • Xiaomi MiPad M2 (Android 5.1).

Builds:

  • Nightly 68.0a1 (2019-05-01);
  • Beta 67.0b16;
  • Release 66.0.2.

Steps to reproduce:

  1. Go to https://addons.mozilla.org/en-US/firefox/themes/.
  2. Search for "Calm Pastel" themes and add one of them.

Expected result:
The installed theme should apply and be seen.

Actual result:
The installed theme is not working,

Notes:
Not reproducible on Desktop.

Flags: needinfo?(jh+bugzilla)

Triaging this as a P3 for now, since I don't see any issue with other themes in the themes section. I tried on beta and all of the other featured themes I tried did work.

Priority: -- → P3

Hi, i reproduced this issue but with different themes, on device Huawei MediaPad M3 Lite10(Android 7.0.0) on build Firefox Beta 68.0b14, Firefox Release 67.0.3 and on Firefox Nightly 68.0a1 (2019-06-26).
Themes:

  • "Purple Earth"
  • "NightlyLights"
    Both of them are property of wpossum.
    Video attached.

Hi, I reproduce the issue with device Sony Xperia Z3 (Android 5.1.1) on build Firefox RC 68.0 build 3.
Theme:

  • rainbow blur - by Alix P
Summary: "Calm Pastel" theme from Firefox Add-ons doesn't work → Multiple Firefox Add-ons themes are not working-themes are not applied

Moto E5 running Android 8 with Fennec Nightly from GPS and I am seeing this issue. It was working during the week beginning 17th June, but at a point since then, I cannot get a number of themes to display.

I am seeing this issue on some themes, but not on others. For example "cool neon" does work, but "calm pastel" does not as do other themes.

I can remember an issue I had with desktop Nightly some time ago where certain desktop themes would cause the browser to collapse. (See https://bugzilla.mozilla.org/show_bug.cgi?id=1344839). Pardon my technical ignorance, but I think this was down to the codes used by certain colours in the themes. The themes were okay, it was just that Firefox could not figure out how to display them.

Could the same issue be present in Fennec where some colours in some themes (the background or the text colour) are causing Fennec to not display the theme?

If someone could get in contact (email or Slack) to help me to use Mozregression with Fennec, happy to help find a regression that could assist with this.

User has flagged up this issue in the SUMO support forum at https://support.mozilla.org/en-US/questions/1264674

Hi! This is reproducible on ESR 68.1b7 with HTC Desire 820 (Android 6.0.1).
Theme: KINGYO~goldfish

status-firefox-esr68: --- → affected

Hey Shane, do you have some cycles to try and diagnose this? Might be more serious that originally thought.

Flags: needinfo?(mixedpuppy)
Keywords: regression, regressionwindow-wanted
Priority: P3 → --

This is two area's I'm not really familiar with, android and themes. However, I took a look and didn't see anything obvious in the api layer. A little testing doesn't show anything consistent (image size, filename, image type). Chatted with Luca about it, he came up with this after a little testing:

08-28 22:10:44.235 2377 2504 D GeckoJarReader: No Entry for Spooky%20TimeH3.jpg
08-28 22:10:44.235 2377 2504 W GeckoBitmapUtils: decodeUrl: stream not found downloading jar:file:///data/user/0/org.mozilla.fennec_rpl/files/mozilla/23wnjrl8.default/extensions/%7B319a0a05-c764-4c0a-90a7-abb92774ec72%7D.xpi!/Spooky%20TimeH3.jpg

This is down in the java layer, we'd need to find someone more familiar with working on that.

AddonManager is not the right component, but not certain what is.

Lets see of someone who's familiar with this code can give us some input.

Component: Add-on Manager → General
Flags: needinfo?(snorp)
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(agi)

Looking at the theme from comment 10, the problem seems to be that we're trying to open the file using it's URL-encoded file name, but in the ZIP-file the image is actually stored with a literal space in the file name.

Flags: needinfo?(jh+bugzilla)

There really isn't a good component for this, but to ensure we keep an eye on it (since it effects themes at least) putting it back into AddonManager.

Component: General → Add-on Manager

Assuming the URL string the JAR reader receives has been completely URL-encoded,
we need to decode not just the first nested jarUrl, as already happens through
getZipFile(), but all subsequent path components, too.

At least for jar:-URLs received from Gecko, the above assumption certainly seems
to be true.

I just remembered, there is one theme failing that doesn't have a space in the filename: Black Panther Theme by MaDonna. Can you verify with that theme?

Which one exactly? There are multiple "Black Panther" themes by that author, at least one of which does have a space in the file name, too.

Assignee: nobody → jh+bugzilla
Flags: needinfo?(snorp)
Flags: needinfo?(agi)
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/cd343dac2978
URL-decode all file names in GeckoJarReader. r=snorp

(In reply to Jan Henning [:JanH] from comment #15)

Which one exactly? There are multiple "Black Panther" themes by that author, at least one of which does have a space in the file name, too.

I just went through the list in comment 8, downloaded them to examine the manifest/etc. My download does not have a space in the filename, the image is "header.png".

https://addons.mozilla.org/firefox/downloads/file/2759993/black_panther_theme_by_madonna-3.0-an+fx.xpi

Oh right - in that case that particular theme is working fine for me. Maybe the confusion happened while compiling that list, and one of MaDonna's other "Black Panther" themes was actually meant?

(In reply to Jan Henning [:JanH] from comment #18)

Oh right - in that case that particular theme is working fine for me. Maybe the confusion happened while compiling that list, and one of MaDonna's other "Black Panther" themes was actually meant?

IIRC when I tested this on firefox beta on android, it would not work.

Works for me on a current-ish Beta (68.1b7), too.

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

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

status-firefox69: --- → ?

The regression window is most likely the switch from LWTs to webextension static themes.

status-firefox69: ? → wontfix
Keywords: regressionwindow-wanted

Comment on attachment 9089181 [details]
Bug 1548776 - URL-decode all file names in GeckoJarReader. r?snorp

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fennec now ships from ESR68 only
  • User impact if declined: A significant number of browser themes won't work as we cannot properly load the theme image.
  • Fix Landed on Version: 70
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch assumes that the complete JAR-URL has been URL-encoded when it is being handed to the GeckoJarReader. As far as themes are concerned this is true, however there is a possibility that URL-encoding of JAR-URLs hasn't been consistently applied in other locations using JAR-URLs, in which case we might try to URL-decode parts of the URL that haven't actually been URL-encoded previously.

In practice however, outside of themes the GeckoJarReader is only being used for loading resources from the omnijar inside the APK, and the file names in our APKs and the omnijar don't seem to use any special characters where URL-encoding would make an actual difference.

  • String or UUID changes made by this patch: none
Attachment #9089181 - Flags: approval-mozilla-esr68?
Flags: qe-verify+

Comment on attachment 9089181 [details]
Bug 1548776 - URL-decode all file names in GeckoJarReader. r?snorp

let's get this in early 68.2 nightly and beta builds

Attachment #9089181 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
QA Whiteboard: [qa-triaged]

Hi, verified with Google Pixel 3 XL (Android 9) and Samsung Galaxy Note 9 (Android 8.1.0) on Firefox Beta 68.2b1 and Firefox Nightly 70.0a1 (2019-09-03) the issue is still reproducible.
I checked with the build from Comment 21(Firefox Nightly 70.0a1 (2019-08-30)) with themes:

  • "Calm Pastel"
  • "Purple Earth"
  • "NightlyLights"

NOTE:
For theme:

  • "KINGYO~goldfish" the issue is not reproducible with the mentioned devices, neither on Firefox Nightly (2019-09-03) nor from build from Comment 21(Firefox Nightly 70.0a1 (2019-08-30))- the theme is applied

I will mark the ticket as REOPEN.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Only themes that specify a theme image via theme_frame are supported, themes that use e.g. additional_backgrounds (Purple Earth, Nightly Lights) or no theme image at all (Calm Pastel - technically it has an image because I think it was previously required even for desktop, but in any case it's completely transparent) aren't supported on Android.

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED

Hi, based on previous comment for theme "KINGYO~goldfish" and neither does for "Running Foxes by MaDonna" the issue is not reproducible with Google Pixel 3 XL (Android 9) and Samsung Galaxy Note 9 (Android 8.1.0) on Firefox Beta68.2b1, Firefox Nightly 71.0a1 (2019-09-03) and Firefox Nightly 70.0a1 (2019-08-30), this was one of the themes that previously did not work and now works correctly.
I will mark the ticket as VERIFIED.

Status: RESOLVED → VERIFIED
status-firefox70: fixed → verified
status-firefox-esr68: fixed → verified

It seems our MDN docs need updating.

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

That states theme_frame is not supported on android. Information from comment 29 should all be verified an MDN.

Keywords: dev-doc-needed
QA Whiteboard: [qa-triaged]
Flags: qe-verify+

Hi Richard, would you be able to take care of comment 31?

Flags: needinfo?(richard)

I will let you know when it's done Caitlin

Hi Jan, I have made the MDN changes for this:

Would you have a moment to check and confirm? Thanks!

Flags: needinfo?(richard) → needinfo?(jh+bugzilla)

(In reply to Richard Bloor from comment #34)

Hi Jan, I have made the MDN changes for this:

Between 65 and 70, headerURL as an alternative (and deprecated) version of theme_frame is supported, too. Otherwise it looks fine, thank you.

Also, in case we want to document it that explicitly, then Fennec currently only supports the following ways of specifying a colour (i.e. for frame):

Flags: needinfo?(jh+bugzilla)

(In reply to Jan Henning [:JanH] from comment #35)

Between 65 and 70, headerURL as an alternative (and deprecated) version of theme_frame is supported, too.

Can I confirm, Comment 28 seems to indicate that the issue was reproduced on Beta 68.2b1 suggesting that headerURL ceased to be an alternative in 68 or possibly before.

Thanks for the other information, I will update the information about color support in Firefox for Android themes.

Flags: needinfo?(jh+bugzilla)

headerURL is certainly still working in the current official versions based on ESR68, and beyond that it doesn't really matter at the moment.

Flags: needinfo?(jh+bugzilla)

Can I confirm that the browser compatibility table should read like this:

Firefox Firefox for Android
headerURL 55 — 70 65 — 70
notes Use theme_frame instead. Use theme_frame instead. Required if theme_frame isn't provided.
theme_frame 55 68
notes Required if headerURL isn't provided.
Flags: needinfo?(jh+bugzilla)

As far as I can tell, headerURL might already have stopped working in Firefox 70 both on Desktop and on Nightly - while I haven't tested the very last Nightly version before the branch to Firefox 71, in the local builds of Firefox 70 I still had lying around, themes with headerURL have already stopped working properly. Plus in any case for the current Firefox for Android there are no official releases later than 68, so for "Firefox for Android" I'd just note it as 65 - 68, and for desktop I'd double-check when exactly it stopped working.

Flags: needinfo?(jh+bugzilla)

Hi Tim, can you confirm when headerURLwas removed from Firefox?

Flags: needinfo?(ntim.bugs)

(In reply to Richard Bloor from comment #40)

Hi Tim, can you confirm when headerURLwas removed from Firefox?

headerURL was removed in bug 1472740 in Firefox 70. You have to use theme_frame which should now do the same thing as headerURL on Android. Also, theme_frame should have started working at the same time as when WebExtension themes did on Android (not sure what release this was, but I think Jan knows).

Flags: needinfo?(ntim.bugs)

Sorry to be a pain to Tim, so I update the compatibility table correctly, can I confirm that means the last version in which headerURL was supported was 69.

Flags: needinfo?(ntim.bugs)

(In reply to Richard Bloor from comment #42)

Sorry to be a pain to Tim, so I update the compatibility table correctly, can I confirm that means the last version in which headerURL was supported was 69.

Yep, same as Firefox Desktop.

Flags: needinfo?(ntim.bugs)

Note that Firefox for android >= 69 does not exist. It's currently at 68.1.

Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: