793338 - click-to-play: implement multiple plugin doorhanger ui (vulnerability status bits)

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

+++ This bug was initially created as a clone of Bug #754472 +++

bug 754472 handled moving to the new UI overall. This bug handles the parts that are specific to click-to-play via blocklisting (i.e. the vulnerability status of the plugins).

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Dietrich - this applies on top of the patch in bug 754472.

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 663605 [details] [diff] [review]
patch

Get initial review from Jared or Margaret. I can do a final pass if needed, but they should be enough.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 663605 [details] [diff] [review]
patch

Jared - if you could have another look at this, that'd be great.
I left the exclamation marks in as per the UI design, but if you feel strongly about removing them, I could maybe be convinced.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 663605 [details] [diff] [review]
patch

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

I talked with Stephen and I'm OK with the usage of the exclamation points, but it does differ with our Plugin Check tone. For comparison, http://jrswab.files.wordpress.com/2012/09/firefoxpluginupdatepage.png

::: browser/base/content/browser-plugins.js
@@ +437,5 @@
>      for (let pluginName in pluginsList) {
> +      let plugin = pluginsList[pluginName][0];
> +      let warn = false;
> +      let warningtext = "";
> +      let updatelink = Services.urlFormatter.formatURLPref("plugins.update.url");

nit: camelCase these variable names for internal consistency with the rest of this file.

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

Blair had this feedback in bug 772897 comment #4:
If these images are useful to be re-used (shorlander?), they should be moved out of /extensions/ and into something like global/icons

@@ +152,5 @@
> +  background-image: url("chrome://mozapps/skin/extensions/alerticon-info-negative.png");
> +  background-repeat: no-repeat;
> +  width: 16px;
> +  height: 15px;
> +  margin-top: -4px;

Why do we need the negative margin here? Is the image not the correct height or is there something else that can be used here? Also, should the image be height=16px?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2

Fixed the camel case and the negative margins.
With regard to the images (> +  background-image: url("chrome://mozapps/skin/extensions/stripes-info-negative-small.png");), I was just following the style of what was already there - there are a number of such stripe images. If we want to move them all, that sounds like a follow-up bug that we can take care of later.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 664642 [details] [diff] [review]
patch v2

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

::: toolkit/themes/pinstripe/global/notification.css
@@ +261,5 @@
> +  background-image: url("chrome://mozapps/skin/extensions/alerticon-info-negative.png");
> +  background-repeat: no-repeat;
> +  width: 16px;
> +  height: 15px;
> +  padding-bottom: 4px;

Please change this to margin-bottom before checking in.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v3

rebased, etc. patch for landing when the patch in bug 754472 gets r+'d.
(carrying over r+)

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 666270 [details] [diff] [review]
patch v3

[Triage Comment]
Pre-approving for Aurora, contingent upon a green build on m-i or m-c.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Landed on m-i. I'll wait for inbound to turn green before landing on m-a.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7949d28f958f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7949d28f958f

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a4880b03634f

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 666270 [details] [diff] [review]
patch v3

>+.center-item-box[warn="true"] {
>+  background-image: url("chrome://mozapps/skin/extensions/stripes-info-negative-small.png");

It's unclear why you're putting this image into mozapps/skin/extensions/. Please file a bug on moving it away from there. Next time around, ask a toolkit peer for review (https://wiki.mozilla.org/Modules/Toolkit).

By the way, browser.xul and browser.css are shared by a variety of different code. Any ids and classes you add ought to be unambiguous; "center-item-box" is way too generic.

Comment 13

[Paul Silaghi, QA [:pauly]]

"Check for updates" option from the doorhanger near the location bar opens a new FF window, unlike the "Check for updates" option from the CTP screen, which opens a new tab.

Comment 14

[Paul Silaghi, QA [:pauly]]

The new UI looks ok on all OSes except bug 797976 (comment 13). Verified fixed