754472 - click-to-play: implement multiple plugin doorhanger ui

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

Attachment: click-to-play ui

My understanding is we want the ui to look like the attached mockup.

Comment 2

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

Reopening, because we will need to first implement the differentiating by plugin-type (as seen in the overlays on the attached mockup) before we can do the doorhanger style.

Comment 3

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

Attachment: patch

I'm at the point implementing this where I'd appreciate some feedback. So far I've only done the ui on linux (in part because bug 771284 and related bugs haven't landed). This requires the patch from bug 749257.

Comment 4

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

Attachment: test page

This is a page you can test the ui on (it requires having built with the patch from bug 749257).

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I tested the patch. I seem following things:

-If the number of plugins require playing is only 1, it is good for a doorhanger does not show a center button.

-Should removing close button from xul popup-notification be separated from this bug?

Comment 6

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

Attachment: patch v2

Saneyuki - thanks for the feedback. I'm not the designer, so I'll defer to what Stephen says regarding the close button and having only one plugin type.

Implemented for all three platforms now.

Comment 7

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

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

Jared, this isn't quite ready for a formal review since a lot of the ui elements aren't finalized yet, but if you could have a look at the code changes, that'd be great.

Comment 8

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

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

Adding a request for feedback from Margaret since she worked on the initial doorhanger implementation (IIUC). I might not be able to get to this until next week.

Comment 9

[:Margaret Leibovic]

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

Sorry for the delayed response, I've been traveling. I actually won't be back at work until Thursday, so I will likely be slow to respond until then.

Instead of extending the PopupNotifications for this one case, I think that it would be better to just override the binding, like we do for other unique doorhangers that require more UI. You should look at what we did for "geolocation-notification" and "addon-progress-notification". This should let you avoid changes to PopupNotifications and the generic bindings, and you should be able to put the logic for these center buttons in with your new binding.

Comment 10

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

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

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

Thanks for the feedback Margaret. I'll cancel my feedback request for this version of the patch.

Comment 11

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

Attachment: patch v3

Ok - overrode the binding instead of modifying it.
In terms of ui, the spacing should be right now.

Comment 12

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

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

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

If you add a new toolkit/themes/winstripe/mozapps/plugins/pluginGeneric-64-aero.png, you would also need to add a corresponding non-aero image.

When hitting "Activate All Plugins", I got this error:

Timestamp: 9/18/2012 2:23:34 PM
Error: Error: Permission denied to access property 'toString'

After hitting "Activate All Plugins" the plugins loaded, even though the error message was shown in the Error Console.

This is a partial review since I wasn't able to go over all this changes in this pass. There also appear to be some RTL issues with this patch. Please test it using the Force RTL add-on (https://addons.mozilla.org/en-US/firefox/addon/force-rtl/).

::: browser/base/content/browser-plugins.js
@@ +27,5 @@
>          pluginsPage = "";
>        }
>      }
>  
> +    tagMimetype = pluginElement.QueryInterface(Components.interfaces.nsIObjectLoadingContent).actualType;

This change isn't necessary. Can you revert this back to two lines with .actualType being on a separate line?

@@ +44,5 @@
> +  }
> +
> +  return { mimetype: tagMimetype,
> +           pluginsPage: pluginsPage,
> +           pluginName: pluginName ? pluginName : "Unknown" };

We'll need something localizable here, won't we?

@@ +64,5 @@
>        return "Adobe Flash";
>  
>      // Clean up the plugin name by stripping off any trailing version numbers
>      // or "plugin". EG, "Foo Bar Plugin 1.23_02" --> "Foo Bar"
> +    let newName = aName.replace(/[\s\d\.\-\_\(\)]+$/, "").replace(/\bplug-?in\b/i, "").replace(/\s+$/, "");

Why did the first two regex's get reordered? Can you use String.trim() instead of the last regex?

@@ +166,5 @@
> +          overlayText.textContent = messageString;
> +          if (event.type == "PluginVulnerableUpdatable" ||
> +              event.type == "PluginVulnerableNoUpdate") {
> +            let vulnerabilityString = gNavigatorBundle.getString(event.type);
> +            let vulnerabilityText = doc.getAnonymousElementByAttribute(plugin, "class", "msg msgVulnerabilityStatus");

Can you add an anonid attribute to this element so that the getAnonymousElementByAttribute method isn't checking based on the order of class names? Maybe anonid="vulnerabilityStatus"

@@ +219,5 @@
>        objLoadingContent.playPlugin();
>  
>      let cwu = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                              .getInterface(Ci.nsIDOMWindowUtils);
> +    let pluginNeedsActivation = gPluginHandler._pluginNeedsActivation([aPlugin]);

This is slightly confusing. The function looks like it is going to return if any instances of |aPlugin| need activation, but it actually returns if anything outside of |aPlugin| needs activation. This function should be renamed.

@@ +420,5 @@
> +        label: gNavigatorBundle.getString("activateSinglePlugin"),
> +        plugins: pluginsList[pluginName],
> +        callback: function() {
> +          for (let objLoadingContent of this.plugins) {
> +            objLoadingContent.playPlugin();

Tested on CNN.com and tried to activate Flash by just choosing the "Activate" button on the doorhanger. I got this error:

Timestamp: 9/18/2012 2:23:30 PM
Error: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIObjectLoadingContent.playPlugin]
Source File: chrome://browser/content/browser.js
Line: 3398

::: toolkit/themes/pinstripe/global/notification.css
@@ +229,5 @@
> +  background-repeat: no-repeat;
> +  height: 16px;
> +  width: 16px;
> +  margin-top: -4px;
> +  margin-right: 6px;

-moz-margin-end

Comment 14

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

The patch for bug 772897 depends on the patch in this bug.

Comment 15

[:Margaret Leibovic]

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

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

I like this approach much more. I have a few comments, but I think a browser peer should also have a look before this gets a final r+, since I'm not totally confident in our XBL best practices.

::: browser/base/content/urlbarBindings.xml
@@ +1483,5 @@
> +    </content>
> +    <resources>
> +      <stylesheet src="chrome://global/skin/notification.css"/>
> +    </resources>
> +    <implementation>  

Nit: whitespace.

@@ +1495,5 @@
> +        const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +        let popupnotification = this;
> +        let item = null;
> +        this.notification.options.centerActions.forEach(function(action) {
> +          item = document.createElementNS(XUL_NS, "popupnotification-centeritem");

I don't know that we like making a new element like this. I did something with a binding similar to this for about:permissions [1], and for that I just created a regular XUL element, then attached the binding by setting a class on the element. It seems to me like you could just make an hbox, then attach its contents in a binding to .center-item-box, or something like that.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/aboutPermissions.xml, http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/aboutPermissions.js#618, http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/aboutPermissions.css

@@ +1503,5 @@
> +          action.notification = popupnotification.notification;
> +          popupnotification.appendChild(item);
> +        });
> +        if (item != null) {
> +          item.setAttribute("padbottom", "true");

Instead of setting an attribute like this, you can do this all in CSS with :last-of-type.

Comment 16

[Alex Keybl [:akeybl]]

Once this gets r+ and  ui-review+ lands, we'll be pushing out test blocklists targeting FF18. Assigning Paul as the qacontact given the fact that he'll be helping us verify. We'll also be uplifting to FF17 on Aurora around the same time.

Comment 17

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

Attachment: patch v4

Thanks for the reviews, Jared and Margaret.
Regarding creating the child elements the aboutPermissions way - I couldn't get that to work. Given that the way I'm doing it is the same as in popupNotifications.jsm, maybe we can fix both in a follow-up.
Also, I would use :last-of-type, but the patch for bug 793338 uses padbottom as well, and in ways last-of-type wouldn't work.

Dietrich - I'd appreciate a review for XBL best practices and whatnot.

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 663604 [details] [diff] [review]
patch v4

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

switching review to Neil, who'd be better judge of new bindings than i.

Comment 19

[Neil Deakin]

Comment on attachment 663604 [details] [diff] [review]
patch v4

   _showClickToPlayNotification: function PH_showClickToPlayNotification(aBrowser) {
...
+    let cwu = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
+                           .getInterface(Ci.nsIDOMWindowUtils);
+    let centerActions = gPluginHandler._makeCenterActions(aBrowser);
     let secondaryActions = [{

cwu is unused.


+  <binding id="center-item">
...
+        <xul:button class="center-item-button"
+                    oncommand="document.getBindingParent(this).runCallback();"
+                    xbl:inherits="oncommand=command,label=buttonlabel"/>
+      </xul:hbox>

oncommand=command doesn't make sense. Both attributes have a different meaning.


+  _makeCenterActions: function PH_makeCenterActions(aBrowser) {
+    let contentWindow = aBrowser.contentWindow;
+    let cwu = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
+                           .getInterface(Ci.nsIDOMWindowUtils);
+    let pluginsList = [];

Should really be let pluginsList = {} since you don't look to be using it as an array.


+  // fake having clicked on the "Activate" button for the Test plugin
+  centerAction.callback();
+  notification.options.centerActions.splice(notification.options.centerActions.indexOf(centerAction), 1);

Isn't this the same as: notification.childNodes[n].runCallback() ? It would be better to call the same code that will be executed than to have cloned code.

I didn't review the theme changes.

Comment 20

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

Attachment: patch v5

Neil - I fixed the things you mentioned. If you could have another look, that'd be great - thanks.

Comment 21

[Neil Deakin]

Comment on attachment 664689 [details] [diff] [review]
patch v5

>+  var centerAction = notification.options.centerActions[0];
>+
>+  ok(centerAction.message == "Second Test", "Test 21c, found center action for the Second Test plugin");
>+  // fake having clicked on the "Activate" button for the Test plugin
>+  centerAction.callback();
>+  notification.options.centerActions.splice(notification.options.centerActions.indexOf(centerAction), 1);

Did you want to change this instance as well?

>diff --git a/toolkit/content/PopupNotifications.jsm b/toolkit/content/PopupNotifications.jsm
>--- a/toolkit/content/PopupNotifications.jsm
>+++ b/toolkit/content/PopupNotifications.jsm
>@@ -466,16 +466,21 @@ PopupNotifications.prototype = {
>       if (bo.height == 0 && bo.width == 0)
>         anchorElement = selectedTab; // hidden
>     } else {
>       anchorElement = selectedTab; // null
>     }
> 
>     this._currentAnchorElement = anchorElement;
> 
>+    let doc = this.window.document;
>+    let arrow = doc.getAnonymousElementByAttribute(this.panel, "anonid", "arrow");
>+    // On OS X and Linux we need a different panel arrow color for
>+    // click-to-play plugins, so copy the popupid and use css.
>+    arrow.setAttribute("popupid", this.panel.firstChild.getAttribute("popupid"));

This code shouldn't be calling getAnonymousElementByAttribute as it relies on the internal structure of the panel. You look like you're using it only to style the arrow, but you could do that with a descendant/child selector.

All the changes you made to the toolkit/*css files look like they don't belong there. They should be in browser/themes somewhere, as they apply to a notification that is specific to the browser implementation.

Did you mean to add a pluginBlocked-64.png for linux as well?

>diff --git a/toolkit/themes/pinstripe/global/popup.css b/toolkit/themes/pinstripe/global/popup.css
>--- a/toolkit/themes/pinstripe/global/popup.css
>+++ b/toolkit/themes/pinstripe/global/popup.css
>@@ -80,16 +80,20 @@ panel[type="arrow"][side="right"] {
>+.panel-arrow[side="top"][popupid="click-to-play-plugins"] {
>+  list-style-image: url("chrome://global/skin/arrow/panelarrow-light-vertical.png");
>+}
>+

You also need to support this for side="bottom" when the panel opens upwards, for instance when the window is near the bottom of the screen.

Comment 22

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

Attachment: patch v6

Thanks for the review, again. I'm pretty sure I addressed everything you mentioned (of course, that's what I said last time...)
With respect to pluginBlocked-64.png for linux, I'm waiting on shorlander for that. In the meantime, it doesn't actually matter because from what I understand, linux will just pick up the windows one if it doesn't have one of its own.

Comment 23

[Neil Deakin]

The code and test changes look ok. I'd want someone else to look at the style changes, especially those that relate to popup-notification-menubutton as I don't understand what the ramifications of those changes are.

Comment 24

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

Comment on attachment 665124 [details] [diff] [review]
patch v6

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

I'm seeing a mismatch of the icon on Windows. Is there plans to get a blue icon added to the panel for cases where there isn't a vulnerable/out-of-date plugin? See http://screencast.com/t/68HQMswW for a screenshot on Windows.

Also, there appears to be a number of CSS properties that probably won't work as intended in RTL.

I only looked at the CSS files.

::: browser/themes/gnomestripe/browser.css
@@ +2848,5 @@
> +  -moz-padding-end: 6px;
> +}
> +
> +.click-to-play-plugins-notification-separator {
> +  border-left: 1px solid hsla(211,79%,6%,.1);

Should this be -moz-border-start?

@@ +2892,5 @@
> +  margin-bottom: 4px;
> +}
> +
> +.center-item-button {
> +  min-width: 0;

What does this achieve?

::: browser/themes/pinstripe/browser.css
@@ +3533,5 @@
> +  padding-top: 16px;
> +  -moz-padding-end: 12px;
> +  -moz-padding-start: 20px;
> +  border-bottom-left-radius: 5px; /* oh dear... */
> +  border-top-left-radius: 5px;    /* what do?   */

I'm not sure what these comments are for, but this looks like it is missing RTL support.

If you need RTL, you can do:
> .click-to-play-plugins-notification-icon-box:-moz-locale-dir(ltr) {
> }
> .click-to-play-plugins-notification-icon-box:-moz-locale-dir(rtl) {
> }

Comment 25

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

(In reply to Jared Wein [:jaws] from comment #24)
> I'm seeing a mismatch of the icon on Windows. Is there plans to get a blue
> icon added to the panel for cases where there isn't a vulnerable/out-of-date
> plugin? See http://screencast.com/t/68HQMswW for a screenshot on Windows.

I'm punting on that, for now. At the moment, unless users turn on plugins.click_to_play, they'll only ever see this ui if we've blocklisted a plugin, in which case we show the correct icon. When click-to-play (non-blocklist style) becomes a full feature of its own, it'll need a non-security icon.

> > +.click-to-play-plugins-notification-separator {
> > +  border-left: 1px solid hsla(211,79%,6%,.1);
> 
> Should this be -moz-border-start?

This is actually for a zero-width element, but I suppose that's best.

> > +.center-item-button {
> > +  min-width: 0;
> 
> What does this achieve?

Buttons seem to have a min-width set, so without this, the center item buttons are too wide.

> ::: browser/themes/pinstripe/browser.css
> @@ +3533,5 @@
> > +  padding-top: 16px;
> > +  -moz-padding-end: 12px;
> > +  -moz-padding-start: 20px;
> > +  border-bottom-left-radius: 5px; /* oh dear... */
> > +  border-top-left-radius: 5px;    /* what do?   */
> 
> I'm not sure what these comments are for, but this looks like it is missing
> RTL support.
> 
> If you need RTL, you can do:
> > .click-to-play-plugins-notification-icon-box:-moz-locale-dir(ltr) {
> > }
> > .click-to-play-plugins-notification-icon-box:-moz-locale-dir(rtl) {
> > }

Thanks - that's exactly why I left those comments in there and then promptly forgot about them :)

Comment 26

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

Attachment: patch v7

Comment 27

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

Comment on attachment 666127 [details] [diff] [review]
patch v7

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

I'm fine with the CSS changes, but I'd also like to get Margaret's opinion on this. Margaret, can you prioritize this review since it is targeted for uplift in to Fx17?

Comment 28

[:Margaret Leibovic]

Comment on attachment 666127 [details] [diff] [review]
patch v7

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

I have a few concerns about the button style rules, but it's mostly just a concern about making our styles more intuitive to read. I tested this out on OSX and tried to look for gotchas, but it seems good to me. Some of the padding values seem a little odd, but I trust that shorlander has verified that the pixels are where they should be :)

Jared, can you test out the focus styles on Windows to make sure those look okay?

::: browser/base/content/urlbarBindings.xml
@@ +1426,5 @@
> +                   xbl:inherits="src=itemicon"/>
> +        <xul:description class="center-item-label"
> +                         xbl:inherits="xbl:text=itemtext"/>
> +        <xul:spacer flex="1"/>
> +        <xul:button class="popup-notification-menubutton center-item-button"

Drive-by on the XBL: This class name doesn't really make sense because this isn't a menubutton. If we want to apply the same styles, I think it would make more sense to make common popup-notification-button styles, then we can do special things for type="menu-button" where necessary.

I know that we want to land this bug ASAP, so I don't think this is important enough to block the bug, but it could be a good follow-up to file.

::: browser/themes/pinstripe/browser.css
@@ +3534,5 @@
> +  list-style-image: url("chrome://global/skin/arrow/panelarrow-light-vertical.png");
> +}
> +
> +.click-to-play-plugins-notification-content {
> +  margin: -16px;

Why do we need this negative margin? (Same question applies to the same rule in the other CSS files.)

::: toolkit/themes/pinstripe/global/notification.css
@@ -126,5 @@
>  
> -.popup-notification-menubutton:not([type="menu-button"]) {
> -  padding: 2px 9px;
> -}
> -

I guess we don't have any .popup-notification-menubutton buttons that aren't type="menu-button" now that we always have a "Not now" secondary action. This was probably added before that happened.

Comment 29

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

Thanks for the review, Margaret.
I agree about the button classes/css issues - I'll file a follow-up.
The negative margin is because the panel that contains the popup content has a padding of 16, whereas the design calls for no padding there. Rather than change that and every other popup, I opted for this popup to use a negative margin to basically override that. I imagine there's a way to do this with child/descendant selectors and whatnot, but maybe this could be another follow-up. (for reference, that css is here: http://dxr.lanedo.com/mozilla-central/toolkit/themes/pinstripe/global/popup.css.html#l69 )

Comment 30

[:Margaret Leibovic]

(In reply to David Keeler from comment #29)

> The negative margin is because the panel that contains the popup content has
> a padding of 16, whereas the design calls for no padding there. Rather than
> change that and every other popup, I opted for this popup to use a negative
> margin to basically override that. I imagine there's a way to do this with
> child/descendant selectors and whatnot, but maybe this could be another
> follow-up. (for reference, that css is here:
> http://dxr.lanedo.com/mozilla-central/toolkit/themes/pinstripe/global/popup.
> css.html#l69 )

Cool, thanks for the clarification. I just wanted to make sure we weren't covering up some other bizarre issue.

Comment 31

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

I'm not seeing any focus issues on Windows. I think this still needs r+ from Neil though.

Comment 32

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

(In reply to Jared Wein [:jaws] from comment #31)
> I'm not seeing any focus issues on Windows. I think this still needs r+ from
> Neil though.

Neil (In reply to Neil Deakin from comment #23)
> The code and test changes look ok. I'd want someone else to look at the
> style changes, especially those that relate to popup-notification-menubutton
> as I don't understand what the ramifications of those changes are.

I think Neil as good as r+'d it. Plus, I've been told he's away for the next two weeks. I think we've had enough eyes on this and it's ready to land (here's a try run from last week, by the way: https://tbpl.mozilla.org/?tree=Try&rev=715fdc15679c )

Jared - do you feel comfortable landing this?

Comment 33

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

Yeah, I feel comfortable landing it.

Comment 34

[Dietrich Ayala (:dietrich)]

Comment on attachment 666127 [details] [diff] [review]
patch v7

carrying over neil's review from comment #23. the other areas were reviewed by jared and margaret.

Comment 35

[Alex Keybl [:akeybl]]

Comment on attachment 666127 [details] [diff] [review]
patch v7

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

Comment 36

[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/644aac25b7ab

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/644aac25b7ab

Comment 38

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/93aa14ba87dd

Comment 39

[u279076]

On second thought, changing verifyme to [qa-] given in-testsuite+.

Comment 41

[Paul Silaghi, QA [:pauly]]

Nightly 2012-12-10 Win 7 x64
1. Enable CTP
2. Open http://wickedgoodgames.com/java2/asteroids.html
3. Click the CTP doorhanger

Actual results:
Java is not on the list
http://img24.imageshack.us/img24/181/ctpjava.png

Any ideas? Works fine with other pages like http://mozqa.com/data/firefox/plugins/plugin-test.html

Comment 42

[u279076]

Paul, it might be worth filing a new bug for this specific case. Do you see any errors in the Error Console? David, could you advise Paul further on this case?

Comment 43

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

Good catch, Paul. I filed bug 820497.