Last Comment Bug 777805 - Update plugin placeholder string for unsupported platforms, and add "Learn More..." link
: Update plugin placeholder string for unsupported platforms, and add "Learn Mo...
Status: RESOLVED FIXED
: late-l10n
Product: Firefox for Android
Classification: Client Software
Component: Plugins (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 17
Assigned To: :Margaret Leibovic
:
Mentors:
http://people.mozilla.org/~mwargers/t...
Depends on: 777897
Blocks: 725286
  Show dependency treegraph
 
Reported: 2012-07-26 11:15 PDT by :Margaret Leibovic
Modified: 2012-07-27 14:39 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
15+


Attachments
patch (8.21 KB, patch)
2012-07-26 11:30 PDT, :Margaret Leibovic
l10n: feedback-
Details | Diff | Review
patch v2 (8.27 KB, patch)
2012-07-26 13:59 PDT, :Margaret Leibovic
l10n: feedback+
Details | Diff | Review
patch v3 (9.05 KB, patch)
2012-07-26 14:37 PDT, :Margaret Leibovic
mbrubeck: review+
blassey.bugs: approval‑mozilla‑aurora+
blassey.bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description :Margaret Leibovic 2012-07-26 11:15:54 PDT
I'm splitting this off from bug 725286, since adding this "Learn More..." link involves more than a simple string swap.
Comment 1 :Margaret Leibovic 2012-07-26 11:30:08 PDT
Created attachment 646228 [details] [diff] [review]
patch

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 :(
Comment 2 :Margaret Leibovic 2012-07-26 11:31:21 PDT
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 3 Axel Hecht [:Pike] 2012-07-26 11:49:05 PDT
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?
Comment 4 Justin Dolske [:Dolske] 2012-07-26 11:55:16 PDT
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.
Comment 5 :Margaret Leibovic 2012-07-26 11:59:01 PDT
(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?
Comment 6 Axel Hecht [:Pike] 2012-07-26 12:05:40 PDT
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.
Comment 7 :Margaret Leibovic 2012-07-26 13:33:45 PDT
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.
Comment 8 :Margaret Leibovic 2012-07-26 13:59:34 PDT
Created attachment 646322 [details] [diff] [review]
patch v2

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.
Comment 9 Axel Hecht [:Pike] 2012-07-26 14:04:24 PDT
Comment on attachment 646322 [details] [diff] [review]
patch v2

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

Thanks.
Comment 10 :Margaret Leibovic 2012-07-26 14:37:51 PDT
Created attachment 646352 [details] [diff] [review]
patch v3

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.
Comment 11 Matt Brubeck (:mbrubeck) 2012-07-26 14:43:07 PDT
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.
Comment 12 :Margaret Leibovic 2012-07-26 15:03:40 PDT
(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
Comment 13 :Margaret Leibovic 2012-07-26 15:06:05 PDT
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
Comment 14 :Ehsan Akhgari (out sick) 2012-07-27 08:59:29 PDT
https://hg.mozilla.org/mozilla-central/rev/1ad9ae46d82b

Note You need to log in before you can comment on or make changes to this bug.