1754247 - Add marketplace image icons inside spotlight modal for list of supported locales

Status: VERIFIED FIXED

(Firefox :: Messaging System, enhancement, P2)

Description

[Punam Dahiya [:pdahiya]]

Firefox focus promo uses two download images 'Get it on Google Play' and 'Download on App Store' with respective CTA. For Fx100 Firefox Focus promo needs to rollout globally and these two images can change by locale.

Scope of this bug is to update spotlight tab modal to add two image elements that displays localized markeplace icon images for list of pre-defined locales

https://www.figma.com/file/DuYnaE2DxETEbBFUOrNzyM/Privacy-Segmentation?node-id=707%3A2753

Comment 1

[Punam Dahiya [:pdahiya]]

Firefox focus promotion designs on about:privatebrowsing needs market icons that needs to be localized for locales where supported

https://developer.apple.com/app-store/marketing/guidelines/
https://play.google.com/intl/en_us/badges/

See https://bugzilla.mozilla.org/show_bug.cgi?id=1754227#c2 for 'Get it on Google Play' and 'Download on App Store' market icon placement. Click on these icons will take user to respective download location similar to click of these icons on https://www.mozilla.org/en-US/firefox/browsers/mobile/focus/ ,
https://www.mozilla.org/it/firefox/browsers/mobile/focus/

Size of the images (See attached) saved from mozilla.org link above varies from 4 kb to ~10 kb. One option is to have these assets in tree for supported locales and have code pick respective asset by pointing to respective image by locale e.g. ../badges/appstore/en.svg

NI @gijs for feedback on how to best address localized image support and if we have done image localization anywhere in Firefox desktop. Thanks

Comment 2

[Punam Dahiya [:pdahiya]]

Attachment: download-app-store.svg

Comment 3

[Punam Dahiya [:pdahiya]]

Attachment: google-play.png

Comment 4

[:Gijs (he/him)]

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

NI @gijs for feedback on how to best address localized image support and if we have done image localization anywhere in Firefox desktop.

If we have, I'm not aware of it. Passing to flod in case I'm missing something.

My suggestion would be to use SVG content, and rather than loading it as an image, embed the SVG content in the HTML markup. Then you can use data-l10n-id attributes to localize the "get it on" or "available on" text, the same way you would for HTML content, AIUI.

Note that if the templates are going to contain markup and come from remote sources (nimbus / messaging services / snippets / whatever) we'll need to be very careful around sanitization, which may complicate this approach. We don't have that problem for images normally because they load as images and can't load script, but that wouldn't be true for inline SVG.

Comment 5

[Francesco Lodolo [:flod]]

I don't think we have ever done image localization in products. Even outside, the only use case would be images for app stores, and that is a huge pain (adapting the layout to different content, size constraints, etc.).

For example, I don't know if the SVG approach suggested would work safely with different scripts and different text direction.

In this specific case, note that badges are provided directly by the store's owners, and I'm not sure you can use a different asset
https://play.google.com/intl/en_us/badges/
https://developer.apple.com/app-store/marketing/guidelines/

Comment 6

[Meg Viar [:mviar]]

I have an implementation of this in a draft for D140739, adding the App Store SVGs and Play Store PNGs from bedrock. If we don't have a localized version of the button for the user, it falls back to en-US (which is what bedrock does).

I can pull that work out and move it into a patch here if that makes discussion easier.

Comment 7

[Meg Viar [:mviar]]

Attachment: Bug 1754247 - Add marketplace image icons inside spotlight modal for list of supported locales

Comment 8

[Meg Viar [:mviar]]

Hi @gijs - after coming across Bug 1721627, I'm curious if you have advice on implementing fallbacks for chrome resources. In the attached patch, I'm attempting to localize the app marketplace badges in MobileDownloads.jsx. If the badge for a user's locale is missing, it defaults to the "en-US" badge. The mochitest I've added to test the fallback fails due to a crash triggered by the missing resource. I did notice that test hits are intended to be ignored (line 3392). So, it's possible the issue has to do with how the test itself is configured.

Comment 9

[Ed Lee :Mardak]

Instead of packaging all locale image variants for all Firefox builds, potentially we would only package the appropriate file (localized or default/en-US) to be exposed at the same endpoint that always exists, e.g., chrome://activity-stream/…/android.png. It doesn't seem necessary that we have access to more than one android.png at a time?

Although I don't see any current uses of AB_CD = checks in jar.mn? https://searchfox.org/mozilla-central/search?q=AB_CD+%3D&path=jar.mn

We used to have something pretty hacky long ago… https://github.com/mozilla/activity-stream/commit/9552c572e270d5756effb3d7c06dc3f22c1aa5e0#diff-c426376da315963b335c5f79b7dbefd88f004788c604ec24df2b2a4a0fc65213L32-L64

Comment 10

[Francesco Lodolo [:flod]]

(In reply to Ed Lee :Mardak from comment #9)

Instead of packaging all locale image variants for all Firefox builds, potentially we would only package the appropriate file (localized or default/en-US) to be exposed at the same endpoint that always exists, e.g., chrome://activity-stream/…/android.png. It doesn't seem necessary that we have access to more than one android.png at a time?

Assuming you can make this work for full builds, I'm not sure how it would work for language packs?

Comment 11

[Meg Viar [:mviar]]

The new badges all come from Bedrock. What if we just included the en-US defaults for Android and IOS in MC and requested the other localized versions from Bedrock (so the localized URL for Android badges would be https://www.mozilla.org/media/img/l10n/${userLocale}/firefox/android/btn-google-play-high-res.png). The downside is this would create a dependency on Bedrock and break if the directory structure with those assets changes.

Comment 12

[:Gijs (he/him)]

(In reply to Meg Viar from comment #8)

Hi @gijs - after coming across Bug 1721627,

(FWIW, in bugzilla's Very Special Markdown Flavour, you can just type bug NNNNN and it'll autolink it. This has the advantage over doing it manually that it gets the bug summary and status (fixed/reopened/whatever) as the tooltip so it makes it easier to know what bug you're talking about in situations like this. :-) )

I'm curious if you have advice on implementing fallbacks for chrome resources. In the attached patch, I'm attempting to localize the app marketplace badges in MobileDownloads.jsx. If the badge for a user's locale is missing, it defaults to the "en-US" badge. The mochitest I've added to test the fallback fails due to a crash triggered by the missing resource. I did notice that test hits are intended to be ignored (line 3392). So, it's possible the issue has to do with how the test itself is configured.

The ignoring of test files is only that it ignores files packaged specifically for tests. In your case, you're not packaging the file for a test, you're packaging for release and you're exercising the code that would be hit on release.

For text types, if you use fetch the code assumes (for better or worse) that you know what you're doing and will deal with the resource not being there.

Looking at your patch, you're using a per-locale directory structure and you could do a fetch() on the directory, and if that works, use the resources in it - but it's kind of kludgy. A hardcoded list might be more performant, if slightly more work to maintain going forward.

Looking at the patch, I'm actually also kinda concerned about the footprint of all these assets... looks like we'd add a few hundred kb to the on-disk size, and probably close to 100kb to the installer size because pngs don't compress particularly well - and that is kind of unfortunate.

(In reply to Meg Viar from comment #11)

The new badges all come from Bedrock. What if we just included the en-US defaults for Android and IOS in MC and requested the other localized versions from Bedrock (so the localized URL for Android badges would be https://www.mozilla.org/media/img/l10n/${userLocale}/firefox/android/btn-google-play-high-res.png). The downside is this would create a dependency on Bedrock and break if the directory structure with those assets changes.

This makes sense when you think about this from a web perspective, but there are a number of reasons I believe it's not a good idea here:

0. the dependency reasons you cited
1. it breaks in offline mode / without a network
2. it adds fingerprinting surface to the browser usage (a passive network adversary can see the network requests and can know that you're opening a private browsing window)
3. it adds security risk because we'd be embedding remote resources directly into an in-product surface, so an active network attacker could load resources into the browser's privileged environment that way (and, with the right images, exploit UAF bugs in our png/jpg/webp/svg code without needing an additional sandbox escape). See also e.g. bug 1513445, bug 1607483. We already disallow requests like this for scripts and documents - we haven't restricted images yet but we really should.

(In reply to Francesco Lodolo [:flod] from comment #10)

(In reply to Ed Lee :Mardak from comment #9)

Instead of packaging all locale image variants for all Firefox builds, potentially we would only package the appropriate file (localized or default/en-US) to be exposed at the same endpoint that always exists, e.g., chrome://activity-stream/…/android.png. It doesn't seem necessary that we have access to more than one android.png at a time?

Assuming you can make this work for full builds, I'm not sure how it would work for language packs?

Can we not package image files in langpacks?

Comment 13

[Francesco Lodolo [:flod]]

(In reply to :Gijs (he/him) from comment #12)

Assuming you can make this work for full builds, I'm not sure how it would work for language packs?

Can we not package image files in langpacks?

I don't know, and I'm not sure who's the right person to ask at this point (likely someone in RelEng?).

As far as I can remember, we've never done something like that, and we only package strings as part of the XPI.
Assuming you can get the right image in the package, would you be able to access it correctly?

Comment 14

[Meg Viar [:mviar]]

@venetia provided a list of the top 10 locales for mobile downloads. Would limiting the new badge assets to these 10 locales sufficiently reduce the footprint? This would also make hard coding & maintaining the list of supported locales more manageable.

Comment 15

[Meg Viar [:mviar]]

Attachment: Top 10 locales for mobile downloads

Provided by @venetia on 3/23/22

Comment 16

[Meg Viar [:mviar]]

Based on the top ten locales, I reduced the list 7 languages and hard coded the list to avoid crashes on missing resources. That reduces the size of the new badge assets to 168K to the on-disk size (the full set from Bedrock was 1mb).

Comment 17

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #13)

(In reply to :Gijs (he/him) from comment #12)

Assuming you can make this work for full builds, I'm not sure how it would work for language packs?

Can we not package image files in langpacks?

I don't know, and I'm not sure who's the right person to ask at this point (likely someone in RelEng?).

As far as I can remember, we've never done something like that, and we only package strings as part of the XPI.
Assuming you can get the right image in the package, would you be able to access it correctly?

I assume you could just use a chrome://browser/locale/blah/blah.png URL, much like you would for .properties or `.dtd files...

(In reply to Meg Viar from comment #16)

Based on the top ten locales, I reduced the list 7 languages and hard coded the list to avoid crashes on missing resources. That reduces the size of the new badge assets to 168K to the on-disk size (the full set from Bedrock was 1mb).

If the langpack option isn't realistic then I think this is our best option.

Comment 18

[Meg Viar [:mviar]]

@aki - do you know if we've ever packaged image files in language packs? Or maybe know someone in release engineering who might have some context? We're exploring packaging different versions of app marketplace badges (SVGs and PNGs) for a number of locales.

Comment 19

[Aki Sasaki (not active)]

I do not. I think langpacks are addons and therefore have the same functionality/limitations as other addons, but that's a complete guess.
Since :flod is already here, NIing :theone and :mkaply, who may know the answer or know who else to ask.

Comment 20

[Mike Kaply [:mkaply]]

I did some testing and Gijs seems to be correct. Because we are just looking at chrome paths in the XPI, any file (including images) can be placed at the correct location in the langpack and then loaded via the chrome URL.

This was local testing, dynamically loading the XPI, but it should work the same way in practice.

Comment 21

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

(In reply to Mike Kaply [:mkaply] from comment #20)

I did some testing and Gijs seems to be correct. Because we are just looking at chrome paths in the XPI, any file (including images) can be placed at the correct location in the langpack and then loaded via the chrome URL.

This was local testing, dynamically loading the XPI, but it should work the same way in practice.

I confirm that it seems technically possible to reference other resources packages inside the langpack XPI.

We do also create a resource URI, which should be also the scheme we use to load the Fluent locales from, see https://searchfox.org/mozilla-central/rev/b671b6390e88672543b9b7c82132be655bd98856/toolkit/components/extensions/Extension.jsm#3179-3207

(As a side note I haven't checked yet if there is any validation on the addons-linter side that would fail with other resources packaged into a langpack xpi, but I can't recall any specific check related to that just based on my memory about the kind of checks the addons-linter does in that area).

The part that may become tricky and would be worth double-checking carefully may be "making sure that all those additional resources will not keep the xpi in use" (by Firefox itself, e.g. because the image is still references somewhere in the Firefox UI) while the AOM internals may be trying to be removed the xpi (on Windows trying to remove the xpi file while Firefox is still keeping it in use would throw, on Linux and MacOS it will be removed just fine and the disk space freed once the process keeping it in use is finally closed).

It would be great if we could add specific test coverage for that scenario if we are going to package and use other resources, and in particular images, inside the langpack xpi.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9182f2288fd9
Add marketplace image icons inside spotlight modal for list of supported locales r=Mardak,Gijs

Comment 23

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9182f2288fd9

Comment 24

[Simona Rosu [:srosu], Ecosystem QA]

I‘ve verified this enhancement using the latest Firefox Nightly 100.0a1 DE, es-ES, and SR locales (Build ID: 20220331185657) on Windows 10 x64, macOS 11.6, and Ubuntu 20.04.

• The text of the marketplace buttons is translated to supported locales.
• The text of marketplace buttons has fallback in English on unsupported locales.