The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 15

Status

()

Firefox for Android
Plugins
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

(Depends on: 1 bug, {late-l10n})

Trunk
Firefox 17
ARM
Android
late-l10n
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15 fixed, firefox16 fixed, fennec15+)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
I'm splitting this off from bug 725286, since adding this "Learn More..." link involves more than a simple string swap.
(Assignee)

Comment 1

5 years ago
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 :(
Attachment #646228 - Flags: review?(mbrubeck)
Attachment #646228 - Flags: feedback?(l10n)
(Assignee)

Comment 2

5 years ago
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

5 years ago
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.
(Assignee)

Comment 5

5 years ago
(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

5 years ago
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.
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 777897
(Assignee)

Comment 8

5 years ago
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.
Attachment #646228 - Attachment is obsolete: true
Attachment #646228 - Flags: review?(mbrubeck)
Attachment #646322 - Flags: review?(mbrubeck)
Attachment #646322 - Flags: feedback?(l10n)

Comment 9

5 years ago
Comment on attachment 646322 [details] [diff] [review]
patch v2

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

Thanks.
Attachment #646322 - Flags: feedback?(l10n) → feedback+
(Assignee)

Comment 10

5 years ago
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.
Attachment #646322 - Attachment is obsolete: true
Attachment #646322 - Flags: review?(mbrubeck)
Attachment #646352 - Flags: review?(mbrubeck)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 12

5 years ago
(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
(Assignee)

Comment 13

5 years ago
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?
(Assignee)

Updated

5 years ago
Blocks: 725286
No longer blocks: 744060
No longer depends on: 725286
https://hg.mozilla.org/mozilla-central/rev/1ad9ae46d82b
Status: NEW → RESOLVED
Last Resolved: 5 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+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/61afffa83517
https://hg.mozilla.org/releases/mozilla-beta/rev/ea1b3c3822cf
status-firefox15: --- → fixed
status-firefox16: --- → fixed
You need to log in before you can comment on or make changes to this bug.