Closed Bug 777805 Opened 9 years ago Closed 9 years ago

Update plugin placeholder string for unsupported platforms, and add "Learn More..." link

Categories

(Firefox for Android Graveyard :: Plugins, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox15 fixed, firefox16 fixed, fennec15+)

RESOLVED FIXED
Firefox 17
Tracking Status
firefox15 --- fixed
firefox16 --- fixed
fennec 15+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

()

Details

(Keywords: late-l10n)

Attachments

(1 file, 2 obsolete files)

I'm splitting this off from bug 725286, since adding this "Learn More..." link involves more than a simple string swap.
Attached patch patch (obsolete) — Splinter Review
I wasn't sure of the most l10n-friendly way of adding this "Learn More..." link, since the first string needs a trailing space for the spacing to work out correctly. I added an extra empty string after the link for RTL localizers to work with, but I'm not sure if that's best. I just wanted to avoid needing to do things like messing with the styles to get a space between the message string and the link.

Also, I'm sorry this adds more strings :(
Attachment #646228 - Flags: review?(mbrubeck)
Attachment #646228 - Flags: feedback?(l10n)
Extra also: I still need a real SUMO url, but that would be easy to land separately because it wouldn't affect string at all (I just copied the way we did the telemetry "Learn More..." link).
Comment on attachment 646228 [details] [diff] [review]
patch

Review of attachment 646228 [details] [diff] [review]:
-----------------------------------------------------------------

f-, I think the localization note for the .post string might want to be different.

Also, I'm not sure that the SUMO part is easy to work like that, we'd want input from either support or webdev on that, I think.

(FWIW, the sumo code ends up in https://github.com/mozilla/kitsune/blob/master/apps/inproduct/views.py, not sure if we have more redirects in the db than the fixture has in https://github.com/mozilla/kitsune/blob/master/apps/inproduct/fixtures/inproduct/redirects.json)

::: mobile/android/chrome/content/browser.js
@@ +2814,5 @@
> +          learnMoreLink.addEventListener("click", function(e) {
> +            let learnMoreUrl = Services.urlFormatter.formatURLPref("app.support.baseURL");
> +            // XXX link to the correct article
> +            learnMoreUrl += "";
> +            BrowserApp.addTab(learnMoreUrl);

If you're going through the app.support.baseURL, you'll need a redirect in the SUMO database to the actual article instead of just the article. Maybe jump on #webdev and ask if that's preferred?

We also have http://mxr.mozilla.org/mozilla-central/source/mobile/xul/app/mobile.js#435, 

pref("app.sync.tutorialURL", "https://support.mozilla.org/kb/sync-firefox-between-desktop-and-mobile");

which just goes to the sumo kb directly.

::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
@@ +28,5 @@
> +<!ENTITY unsupportedPlatform.learnMore                       "Learn More…">
> +<!-- LOCALIZATION NOTE (unsupportedPlatform.pre): Mobile only. Included for RTL locales. This message should be the same
> +	 as unsupportedPlatform.pre. Include a trailing space as needed. -->
> +<!ENTITY unsupportedPlatform.post                            "">
> +

I think it's good to have a post string, but I'm not sure if that'd be for RTL, shouldn't that content align with locale.dir or so?
Attachment #646228 - Flags: feedback?(l10n) → feedback-
Note that we already show a bottom-left help icon for the crashed plugin case:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-plugins.js#606

From the background of bug 665196, it seems a lot of people actually clicked on it, so it appears to be effective.

This might help with keeping the text brief, as well as avoiding string churn.
(In reply to Axel Hecht [:Pike] from comment #3)

> ::: mobile/android/chrome/content/browser.js
> @@ +2814,5 @@
> > +          learnMoreLink.addEventListener("click", function(e) {
> > +            let learnMoreUrl = Services.urlFormatter.formatURLPref("app.support.baseURL");
> > +            // XXX link to the correct article
> > +            learnMoreUrl += "";
> > +            BrowserApp.addTab(learnMoreUrl);
> 
> If you're going through the app.support.baseURL, you'll need a redirect in
> the SUMO database to the actual article instead of just the article. Maybe
> jump on #webdev and ask if that's preferred?

We're already doing this, so if it's a problem, we have two problems:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6188

Maybe Brian knows about why this is okay?

(In reply to Justin Dolske [:Dolske] from comment #4)
> Note that we already show a bottom-left help icon for the crashed plugin
> case:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> plugins.js#606
> 
> From the background of bug 665196, it seems a lot of people actually clicked
> on it, so it appears to be effective.
> 
> This might help with keeping the text brief, as well as avoiding string
> churn.

I like this suggestion. Ian, what do you think?
http://support.mozilla.org/1/mobile/13.0a1/Darwin/de/how-can-i-help-submitting-performance-data redirects to a real url (the topic in the case you referenced), but http://support.mozilla.org/1/mobile/13.0a1/Darwin/de/configure-mobile-privacy-and-security-settings for example does not. So it needs to be actually set up as an extra step on sumo. Just picked an arbitrary example of an existing kb article for the latter.
I followed up in #webdev, and it is indeed the case that we just let the SUMO folks know what URL we're using, and they can redirect it to the appropriate article. I'm cc'ing Ricky, who gave me this information ;). He said that our current approach for the telemetry url is correct.

So it sounds like we can still update the URL path in browser.js whenever we make the article, so this shouldn't block us from landing a patch here to get these new strings in. 

(In reply to Axel Hecht [:Pike] from comment #3)

> f-, I think the localization note for the .post string might want to be
> different.

> ::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
> @@ +28,5 @@
> > +<!ENTITY unsupportedPlatform.learnMore                       "Learn More…">
> > +<!-- LOCALIZATION NOTE (unsupportedPlatform.pre): Mobile only. Included for RTL locales. This message should be the same
> > +	 as unsupportedPlatform.pre. Include a trailing space as needed. -->
> > +<!ENTITY unsupportedPlatform.post                            "">
> > +
> 
> I think it's good to have a post string, but I'm not sure if that'd be for
> RTL, shouldn't that content align with locale.dir or so?

Hm, I always get confused about what's expected in RTL locales. If locale.dir switches, [.pre][link][.post] will become [.post][link][.pre]. I guess it depends if the RTL locales want the link to still follow the message in a LTR way, so that it appears on a lower line if the text has to wrap. In any case, I can provide the extra empty string, and let localizers do what's best for them :)

I can update the localization notes to indicate that these are all part of one message+link combo, and that localizers should structure it as they see fit.
Depends on: 777897
Attached patch patch v2 (obsolete) — Splinter Review
I updated the localization note for unsupportedPlatform.post, and I pointed to bug 777897 in a comment about the URL.

In that bug we can point to the correct article, coordinating with SUMO/webdev folks to make sure it works correctly.
Attachment #646228 - Attachment is obsolete: true
Attachment #646228 - Flags: review?(mbrubeck)
Attachment #646322 - Flags: review?(mbrubeck)
Attachment #646322 - Flags: feedback?(l10n)
Comment on attachment 646322 [details] [diff] [review]
patch v2

Review of attachment 646322 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.
Attachment #646322 - Flags: feedback?(l10n) → feedback+
Attached patch patch v3Splinter Review
When I updated the string to use &brandShortName; I forgot to also include brand.dtd.

I also decided to use "why-cant-firefox-mobile-play-flash-on-my-device" as the article name. I'm open to suggestions to change that, but it's not a really big deal, since we can always redirect it to a different article.
Attachment #646322 - Attachment is obsolete: true
Attachment #646322 - Flags: review?(mbrubeck)
Attachment #646352 - Flags: review?(mbrubeck)
Attachment #646352 - Attachment is patch: true
Comment on attachment 646352 [details] [diff] [review]
patch v3

Review of attachment 646352 [details] [diff] [review]:
-----------------------------------------------------------------

r=mbrubeck with one nit:

::: mobile/android/chrome/content/browser.js
@@ +2813,5 @@
> +        if (learnMoreLink) {
> +          learnMoreLink.addEventListener("click", function(e) {
> +            let learnMoreUrl = Services.urlFormatter.formatURLPref("app.support.baseURL");
> +            learnMoreUrl += "why-cant-firefox-mobile-play-flash-on-my-device";
> +            BrowserApp.addTab(learnMoreUrl);

Instead of adding a "click" listener, can this code set the "href" attribute attribute on the link?  Then things like context menu items will work as expected.  You can add target="_blank" to the link element to make it open in a new tab.

If we can't do that for some reason, you should pass {parentId: BrowserApp.selectedTab.id} to addTab(), so the back button will work correctly.
Attachment #646352 - Flags: review?(mbrubeck) → review+
(In reply to Matt Brubeck (:mbrubeck) from comment #11)

> ::: mobile/android/chrome/content/browser.js
> @@ +2813,5 @@
> > +        if (learnMoreLink) {
> > +          learnMoreLink.addEventListener("click", function(e) {
> > +            let learnMoreUrl = Services.urlFormatter.formatURLPref("app.support.baseURL");
> > +            learnMoreUrl += "why-cant-firefox-mobile-play-flash-on-my-device";
> > +            BrowserApp.addTab(learnMoreUrl);
> 
> Instead of adding a "click" listener, can this code set the "href" attribute
> attribute on the link?  Then things like context menu items will work as
> expected.  You can add target="_blank" to the link element to make it open
> in a new tab.

That is a better idea. I was just copying the way desktop deals with links in these overlays, and I'm not sure why they did that, but this worked fine when I tested it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad9ae46d82b
Target Milestone: --- → Firefox 17
Comment on attachment 646352 [details] [diff] [review]
patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): needed for bug 725286
User impact if declined: users will get a less apologetic/helpful error message on unsupported devices
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk string changes, plus the addition of a help link
String or UUID changes made by this patch: 3 strings for a message/link in the plugin error overlay
Attachment #646352 - Flags: approval-mozilla-beta?
Attachment #646352 - Flags: approval-mozilla-aurora?
Blocks: 725286
No longer blocks: 744060
No longer depends on: 725286
https://hg.mozilla.org/mozilla-central/rev/1ad9ae46d82b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #646352 - Flags: approval-mozilla-beta?
Attachment #646352 - Flags: approval-mozilla-beta+
Attachment #646352 - Flags: approval-mozilla-aurora?
Attachment #646352 - Flags: approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.