Implement UI for plugins made click-to-play by the blocklist

VERIFIED FIXED in Firefox 17

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ verified, firefox18 verified)

Details

Attachments

(2 attachments, 4 obsolete attachments)

When we make a plugin click-to-play for whatever reason via the blocklist, we need to have some UI in the addons manager.
(Assignee)

Updated

5 years ago
Assignee: nobody → dkeeler
Blocks: 760625
Component: Plug-ins → Add-ons Manager
Product: Core → Toolkit
Summary: implement UI for plugins made click-to-play by the blocklist → Implement UI for plugins made click-to-play by the blocklist
(Assignee)

Updated

5 years ago
Blocks: 738698
Created attachment 662306 [details] [diff] [review]
addon manager bits
Attachment #662306 - Flags: ui-review?(shorlander)
Attachment #662306 - Flags: review?(bmcbride)
Created attachment 662307 [details] [diff] [review]
doorhanger bits
Attachment #662307 - Flags: ui-review?(shorlander)
Attachment #662307 - Flags: review?(jaws)
Comment on attachment 662306 [details] [diff] [review]
addon manager bits

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

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2784,5 @@
>          var warningLink = document.getElementById("detail-warning-link");
>          warningLink.value = gStrings.ext.GetStringFromName("details.notification.softblocked.link");
>          warningLink.href = this._addon.blocklistURL;
>          warningLink.hidden = false;
> +      } else if (this._addon.blocklistState == Ci.nsIBlocklistService.STATE_OUTDATED || this._addon.blocklistState == Ci.nsIBlocklistService.STATE_VULNERABLE_UPDATE_AVAILABLE) {

From bug 760625 comment 9:
> STATE_NO_UPDATE is intended to convey something a bit
> stronger than STATE_OUTDATED - that is, the plugin is known to be vulnerable
> and there is no update, rather than it's simply old

Given that, I think the wording for STATE_VULNERABLE_UPDATE_AVAILABLE should be different than STATE_OUTDATED. It should probably also be an error notification like STATE_VULNERABLE_NO_UPDATE, rather than just a warning.

@@ +2801,5 @@
> +          "details.notification.vulnerable",
> +          [this._addon.name], 1
> +        );
> +        var errorLink = document.getElementById("detail-error-link");
> +        errorLink.value = gStrings.ext.GetStringFromName("details.notification.blocked.link");

This needs to use a different string (even if the value is the same in en-US), since the context is slightly different.
Attachment #662306 - Flags: review?(bmcbride) → review-
Comment on attachment 662307 [details] [diff] [review]
doorhanger bits

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

This is completely independent from the Add-ons Manager UI bits, so would be good to have it in a separate bug.

And I see this patch depends on the patch in bug 754472 - took me awhile to find that :) Haven't looked at this patch too closely, since I haven't looked at the other bug.

::: browser/base/content/urlbarBindings.xml
@@ +7,5 @@
>  
>  <!DOCTYPE bindings [
>  <!ENTITY % notificationDTD SYSTEM "chrome://global/locale/notification.dtd">
>  %notificationDTD;
> +<!ENTITY % pluginsDTD SYSTEM "chrome://mozapps/locale/plugins/plugins.dtd">

Can't re-use strings like this (different context, which matters a lot in some locales). You'll need to add new strings to browser/locales/en-US/chrome/browser/browser.properties, and handle it like the other notifications.

::: toolkit/themes/pinstripe/global/notification.css
@@ +224,5 @@
>    padding-bottom: 12px;
>  }
>  
> +.center-item-box[warn="true"] {
> +  background-image: url("chrome://mozapps/skin/extensions/stripes-info-negative-small.png");

If these images are useful to be re-used (shorlander?), they should be moved out of /extensions/ and into something like global/icons
Status: NEW → ASSIGNED
Depends on: 754472
Comment on attachment 662307 [details] [diff] [review]
doorhanger bits

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

Blair already gave good feedback, and I just noticed too small things. Nice work so far, thanks for working on this!

::: browser/base/content/browser-plugins.js
@@ +414,5 @@
>      }
>  
>      let centerActions = [];
>      for (let pluginName in pluginsList) {
> +      let onePlugin = pluginsList[pluginName][0];

Maybe rename variable to 'plugin'?

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +133,4 @@
>  PluginVulnerableUpdatable=This plugin is vulnerable and should be updated.
>  PluginVulnerableNoUpdate=This plugin has security vulnerabilities.
> +vulnerableUpdatablePluginWarning=Outdated Version!
> +vulnerableNoUpdatePluginWarning=Vulnerable Plugin!

I think we can drop the exclamation marks.
Attachment #662307 - Flags: review?(jaws) → review-
Thanks for the reviews, Blair and Jared. I'm waiting on Shorlander for some feedback, and then I'll put up the revised patches (and I'll probably spin off the notification patch into its own bug).
(Assignee)

Updated

5 years ago
Depends on: 793338
(Assignee)

Updated

5 years ago
No longer depends on: 793338
(Assignee)

Updated

5 years ago
No longer depends on: 754472
(Assignee)

Updated

5 years ago
Attachment #662307 - Attachment is obsolete: true
Attachment #662307 - Flags: ui-review?(shorlander)
Created attachment 664122 [details] [diff] [review]
patch v2

Thanks for the review. I've addressed your comments, so I'm asking for another one.
Attachment #662306 - Attachment is obsolete: true
Attachment #662306 - Flags: ui-review?(shorlander)
Attachment #664122 - Flags: review?(bmcbride)
Comment on attachment 664122 [details] [diff] [review]
patch v2

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

r+ with the following string change:

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
@@ +27,5 @@
>  notification.outdated=An important update is available for %1$S.
>  notification.outdated.link=Update Now
> +#LOCALIZATION NOTE (notification.vulnerableUpdatable) %1$S is the add-on name
> +notification.vulnerableUpdatable=%1$S is known to be vulnerable. Please update it.
> +notification.vulnerableUpdatable.link=More Information

This doesn't quite feel right to me - "Please update it" is a call to action, but the link is separate. Taking inspiration from the outdated notification:

notification.vulnerableUpdatable=%1$S is known to be vulnerable and should be updated.
notification.vulnerableUpdatable.link=Update Now

(Same for the details variant of those strings.)
Attachment #664122 - Flags: review?(bmcbride) → review+
Created attachment 664599 [details] [diff] [review]
patch v3

Fixed strings. Carrying over r+.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=48f092dd21d7
Thanks, Blair!
Attachment #664122 - Attachment is obsolete: true
Attachment #664599 - Flags: review+
QA Contact: paul.silaghi

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f341fc58f595
https://hg.mozilla.org/mozilla-central/rev/f341fc58f595
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment on attachment 664599 [details] [diff] [review]
patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): blocklist/click-to-play plugins (bug 760625)
User impact if declined: users won't see the vulnerability status of click-to-play-blocklisted plugins in about:addons
Testing completed (on m-c, etc.): tested on m-c, been on m-c for about a week
Risk to taking this patch (and alternatives if risky): low (this is really just some small UI changes)
String or UUID changes made by this patch: added strings: 

+notification.vulnerableUpdatable=%1$S is known to be vulnerable and should be updated.
+notification.vulnerableUpdatable.link=Update Now

+notification.vulnerableNoUpdate=%1$S is known to be vulnerable. Use with caution.
+notification.vulnerableNoUpdate.link=More Information

+details.notification.vulnerableUpdatable=%1$S is known to be vulnerable and should be updated.
+details.notification.vulnerableUpdatable.link=Update Now

+details.notification.vulnerableNoUpdate=%1$S is known to be vulnerable. Use with caution.
+details.notification.vulnerableNoUpdate.link=More Information
Attachment #664599 - Flags: approval-mozilla-aurora?
(In reply to David Keeler from comment #12)
> User impact if declined: users won't see the vulnerability status of
> click-to-play-blocklisted plugins in about:addons

If we don't take this in FF17, wouldn't the about:addons experience match that of a soft/hard block?
(In reply to Alex Keybl [:akeybl] from comment #13)
> (In reply to David Keeler from comment #12)
> > User impact if declined: users won't see the vulnerability status of
> > click-to-play-blocklisted plugins in about:addons
> 
> If we don't take this in FF17, wouldn't the about:addons experience match
> that of a soft/hard block?

Unfortunately, no. The UI depends on the exact value of the plugin's blocklistState. If it isn't one of STATE_OUTDATED, STATE_SOFTBLOCKED, or STATE_BLOCKED, it'll just look like a normal plugin.
(In reply to David Keeler from comment #14)
> Unfortunately, no. The UI depends on the exact value of the plugin's
> blocklistState. If it isn't one of STATE_OUTDATED, STATE_SOFTBLOCKED, or
> STATE_BLOCKED, it'll just look like a normal plugin.

Can you propose a workaround that would not have l10n string impact, given the fact that this wasn't included in what we originally intended to uplift?
(In reply to Alex Keybl [:akeybl] from comment #15)
> Can you propose a workaround that would not have l10n string impact, given
> the fact that this wasn't included in what we originally intended to uplift?

For Fx17 we could just show the existing outdated warning instead (which would also mean minimal code change, fwiw).
(In reply to Blair McBride (:Unfocused) from comment #16)
> (In reply to Alex Keybl [:akeybl] from comment #15)
> > Can you propose a workaround that would not have l10n string impact, given
> > the fact that this wasn't included in what we originally intended to uplift?
> 
> For Fx17 we could just show the existing outdated warning instead (which
> would also mean minimal code change, fwiw).

Let's go with this if David agrees.
Created attachment 667704 [details] [diff] [review]
aurora workaround

Something like this?
Attachment #667704 - Flags: review?(bmcbride)
Comment on attachment 667704 [details] [diff] [review]
aurora workaround

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

Yep. Would really like the test ported too though.

Nit: Wrap the lines after the || ?
Attachment #667704 - Flags: review?(bmcbride) → feedback+
Created attachment 667741 [details] [diff] [review]
aurora workaround v2

Ported the tests, added line breaks.
Attachment #667704 - Attachment is obsolete: true
Attachment #667741 - Flags: review?(bmcbride)
Attachment #667741 - Flags: review?(bmcbride)
Attachment #667741 - Flags: review+
Attachment #667741 - Flags: approval-mozilla-aurora?
FF 18.0a1 (2012-10-03)
Clicking on "Update now" in Add-ons Manager right next to:
Shockwave Flash 11.1.102.63 => links to https://addons.mozilla.org/en-US/firefox/blocked/p94

Silverlight 4.0.60531.0 => links to https://addons.mozilla.org/en-US/firefox/blocked/p147 (We're sorry, but we can't find what you're looking for)

Wouldn't be better that "Update now" to link to the plugins check page ?
Comment on attachment 664599 [details] [diff] [review]
patch v3

This isn't the patch we're considering for aurora, so removing the flag.
Attachment #664599 - Flags: approval-mozilla-aurora?
(In reply to Paul Silaghi [QA] from comment #21)
> FF 18.0a1 (2012-10-03)
> Clicking on "Update now" in Add-ons Manager right next to:
> Shockwave Flash 11.1.102.63 => links to
> https://addons.mozilla.org/en-US/firefox/blocked/p94
> 
> Silverlight 4.0.60531.0 => links to
> https://addons.mozilla.org/en-US/firefox/blocked/p147 (We're sorry, but we
> can't find what you're looking for)

Looks like there aren't pages up for the new staged blocks.

> Wouldn't be better that "Update now" to link to the plugins check page ?

I'm on the fence about this one. On the one hand, that's where all other "Update now" links go. On the other, using the block url allows us to give really specific information about what this new kind of block is and how the user can update their plugin. Blair, what do you think?
Depends on: 798176
(In reply to David Keeler from comment #23)
> Looks like there aren't pages up for the new staged blocks.

AFAIK, that's normal for staged blocks.


> > Wouldn't be better that "Update now" to link to the plugins check page ?
> 
> I'm on the fence about this one. On the one hand, that's where all other
> "Update now" links go. On the other, using the block url allows us to give
> really specific information about what this new kind of block is and how the
> user can update their plugin. Blair, what do you think?

Yea, I'm on the fence too. The blocked pages do link to the same update link that the Plugin Checker page links to - so IMO its not a problem, more of a question of which messaging is betterer. Filed bug 798176 for a decision on that, but IMO it's something that can be sorted out at any time.
status-firefox17: --- → affected
status-firefox18: --- → fixed
tracking-firefox17: --- → +
Comment on attachment 667741 [details] [diff] [review]
aurora workaround v2

thanks for working around the l10n changes post-string freeze, makes this possible to uplift. please land prior to this coming monday, oct 8th which is merge day.
Attachment #667741 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [aurora workaround v2 needs checkin to aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a30519f6f10
status-firefox17: affected → fixed
Keywords: checkin-needed
Whiteboard: [aurora workaround v2 needs checkin to aurora]
Addons Manager FF 17b5:
"An important update is available for Java 7u7"

Addons Manager FF 18, 19:
"Java 7u7 is known is known to be vulnerable and should be updated"

Why not the same message ?

Comment 28

5 years ago
(In reply to Paul Silaghi [QA] from comment #27)
> Why not the same message ?

AFAIK because the L10n freeze didn't allow us to get the changed string into 17.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #28)
> (In reply to Paul Silaghi [QA] from comment #27)
> > Why not the same message ?
> 
> AFAIK because the L10n freeze didn't allow us to get the changed string into
> 17.

Yes. See comment 15.
Verified fixed on FF 17b5 based on comment 29.
status-firefox17: fixed → verified
Verified fixed on FF 18b3.
Status: RESOLVED → VERIFIED
status-firefox18: fixed → verified
You need to log in before you can comment on or make changes to this bug.