Last Comment Bug 711552 - Create click to play UI for desktop
: Create click to play UI for desktop
Status: RESOLVED FIXED
[sg:want P1] [advisory-tracking-]
: addon-compat, privacy
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- enhancement with 8 votes (vote)
: mozilla14
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
Depends on: 549697 730318 737675 742619 743102 743421 743429 744745 744964 746859 752228
Blocks: cuts-addons 711618 click-to-play 743312
  Show dependency treegraph
 
Reported: 2011-12-16 11:23 PST by David Keeler [:keeler] (use needinfo?)
Modified: 2012-10-13 04:56 PDT (History)
89 users (show)
jaws: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch v1 (12.66 KB, patch)
2011-12-16 11:23 PST, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Splinter Review
patch v2 (12.82 KB, patch)
2011-12-19 11:49 PST, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Splinter Review
patch v3 (22.23 KB, patch)
2012-01-02 17:37 PST, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Splinter Review
WIP patch for bug v3.1 (12.75 KB, patch)
2012-02-23 21:09 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
WIP patch for bug v3.2 (12.64 KB, patch)
2012-03-06 18:10 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug (apply on top of patch for bug 730318) (15.11 KB, patch)
2012-03-20 13:13 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug v2 (apply on top of patch for bug 730318) (17.31 KB, patch)
2012-03-20 18:20 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug v3 (apply on top of patch for bug 730318) (17.40 KB, patch)
2012-03-21 15:44 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
Details | Diff | Splinter Review
Patch for bug (part 2): Add style changes to gnomestripe and pinstripe (2.08 KB, patch)
2012-03-27 14:03 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
Details | Diff | Splinter Review
Patch for bug v4 (19.24 KB, patch)
2012-03-28 15:26 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug v5 (18.83 KB, patch)
2012-03-29 18:23 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug v6 (18.92 KB, patch)
2012-04-02 13:51 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
Details | Diff | Splinter Review
Tests for patch (7.28 KB, patch)
2012-04-02 15:52 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
Details | Diff | Splinter Review
Tests for patch (now with more tests!) (13.81 KB, patch)
2012-04-04 12:17 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review

Description David Keeler [:keeler] (use needinfo?) 2011-12-16 11:23:20 PST
Created attachment 582332 [details] [diff] [review]
patch v1

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

Basically bug 708464 for desktop.
Patch based on Blair's and Margaret's work in bug 549697.
Comment 1 David Keeler [:keeler] (use needinfo?) 2011-12-19 11:49:15 PST
Created attachment 582904 [details] [diff] [review]
patch v2
Comment 2 Ben Bucksch (:BenB) 2011-12-20 14:35:12 PST
Can we do this without notification box? This will trigger far too often and be annoying. Wear-down etc.pp.

I prefer something like FlashBlock: the area of the plugin in the page is covered, with a single "Play" button in the middle. (And yes, I don't care much about plugins that have no UI.)
Comment 3 Ben Bucksch (:BenB) 2011-12-20 14:37:42 PST
xref bug 711618
Comment 4 Dão Gottwald [:dao] 2011-12-20 14:53:11 PST
(In reply to Ben Bucksch (:BenB) from comment #2)
> (And yes, I don't care much about plugins that have no UI.)

There's a good chance that other user would dislike soundcloud.com & Co. breaking.

The alternative to the notification bar would be to always enable invisible plugins.
Comment 5 Ian Melven :imelven 2011-12-20 15:03:32 PST
(In reply to Dão Gottwald [:dao] from comment #4)
>
> The alternative to the notification bar would be to always enable invisible
> plugins.

There's a couple of sides to this IMO - see bug 549697 comment 24 which talks about Flash content breaking if there are invisible 'helper SWFs' that can't be click to play-ed. From a security standpoint, always allowing invisible plugins to load takes away a lot of click to play being a mitigation against 'driveby plugin exploits' (which we know are common). Then again, working on bug 690161 and having an acceptable UX for blocking vulnerable versions of plugins from loading could potentially be a better way of addressing that problem.
Comment 6 :Margaret Leibovic 2011-12-20 15:15:45 PST
(In reply to Ian Melven :imelven from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #4)
> >
> > The alternative to the notification bar would be to always enable invisible
> > plugins.
> 
> There's a couple of sides to this IMO - see bug 549697 comment 24 which
> talks about Flash content breaking if there are invisible 'helper SWFs' that
> can't be click to play-ed. From a security standpoint, always allowing
> invisible plugins to load takes away a lot of click to play being a
> mitigation against 'driveby plugin exploits' (which we know are common).

For mobile (bug 708464), we decided we would just play all plugins on the page if you clicked on one of them - this avoids the hidden "helper SWF" problem. We also decided we would only show a notification if there were no visible plugins on the page, which was our solution to Dão's concern in comment 4.
Comment 7 Ben Bucksch (:BenB) 2011-12-20 15:47:12 PST
> The alternative to the notification bar would be to always enable invisible plugins.

That's a catastrophe security-wise. Then you can just as well drop the whole feature.

Comment 6: pretty much what I wanted to propose

Note that if the user [x] remembers, the enable should still be specific for the plugins that existed at the time of the remembering, and never enable all plugins on the page. It is common that big, normal websites get infected and deliver exploit code unwillingly, and any solution must protect against that.

Another alternative would be to just make invisible plugins visible and put the Play button there. More or less like the notification bar, just inside the page.
Comment 8 David Keeler [:keeler] (use needinfo?) 2012-01-02 17:37:05 PST
Created attachment 585341 [details] [diff] [review]
patch v3

Here's the latest:
* left or right clicking a plugin brings up a menu with the option to play it
* if a visible plugin is played, all invisible plugins are also played
* if there are no visible plugins, a hanger allows the user to play them (otherwise there is no hanger)
* any permission related issues are implemented in bug 711618

Issue: the 'Tap here to activate plugin' message should really be 'Click here...' - is there an easy to differentiate between mobile and desktop in this case? (right now this is in toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd)
Comment 9 Tom Lowenthal 2012-01-03 09:29:01 PST
(In reply to David Keeler from comment #8)
> * if a visible plugin is played, all invisible plugins are also played
> * if there are no visible plugins, a hanger allows the user to play them
> (otherwise there is no hanger)

Is there differentiation between plugin types? That is:
- if I click and play a flash element, does that also enable java applets on the page;
- if there are visible flash elements and invisible java elements, do I get a doorhanger for the java?
Comment 10 David Keeler [:keeler] (use needinfo?) 2012-01-03 15:11:48 PST
(In reply to Tom Lowenthal [:StrangeCharm] from comment #9)
> Is there differentiation between plugin types? 

Not at the moment, but it's certainly doable. One thing I'm concerned about is that such differentiation might result in more notifications and complexity than users want to deal with (thus resulting in them going back to always enabling plugins, which is not what we want).

> That is:
> - if I click and play a flash element, does that also enable java applets on
> the page;

Only if the java applets are "invisible" (essentially, too small to be seen). Enabling one visible plugin does not affect any other visible ones.

> - if there are visible flash elements and invisible java elements, do I get
> a doorhanger for the java?

Not as currently implemented.
Comment 11 Ben Bucksch (:BenB) 2012-01-04 05:06:47 PST
(In reply to David Keeler from comment #8)
> * left or right clicking a plugin brings up a menu with the option to play it
> * if a visible plugin is played, all invisible plugins are also played
> * if there are no visible plugins, a hanger allows the user to play them
> (otherwise there is no hanger)

That sounds perfect. *Exactly* like that. Thanks!
Comment 12 Benjamin Smedberg [:bsmedberg] 2012-01-04 07:59:28 PST
As a security mitigation, what will/should happen in this scenario: a vulnerability is discovered in Java/Acrobat/whatever (or in specific older versions of these plugins), so we want to make only those plugins "click to play" without affecting Flash. Will this UI ignore the plugins that run automatically and the decision to display a hanger is based only on the semi-blocked plugins?

Will the click-to-play UI show *which* plugins are being activated, or just that some plugin is going to be enabled? In the case of multiple kinds hidden plugins, will the UI show all the various types that will be enabled?

Are we exposing to the DOM that a particular plugin element (<object> or <embed> is user-disabled?) This seems important for websites that rely primarily on plugins (e.g. Pandora) so that they can show alternate UI (plugins are disabled, please click to play) instead of timing out and showing a generic "please install Flash" or "Song initialization timed out, please hit refresh" UI.
Comment 13 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-01-11 14:58:27 PST
need to schedule a secteam review
Comment 14 :Margaret Leibovic 2012-02-03 08:28:06 PST
David, I wanted to let you know that we recently changed our implementation of the click to play plugins UI on mobile because we were running into problems when pages were loaded from the cache. The patch for this is in bug 719875, but I also filed bug 723617 about changing the platform side of things for a more robust solution.
Comment 15 Markus Fischer 2012-02-13 03:30:23 PST
(In reply to Ben Bucksch (:BenB) from comment #7)
> Note that if the user [x] remembers, the enable should still be specific for
> the plugins that existed at the time of the remembering, and never enable
> all plugins on the page.

Who will be the "originator" in case of a driveby attack scenery?

Example: you(t)ube gets hacked, an exploit through flash in a driveby scenario. Naturally the user has allowed flash on yt. The driveby injections is done via a <script src="remote://"> tag, which loads more code from a remote page, loads malicious flash swf, etc.

Would the user see, being on you(t)ube.com, that a plugin is being activated via a third party? Or would it be just granted allowance due user previous preference on you(t)ube?


And of course this is an idealized scenario: once they were able to break into the the server, they could of course server the malicious code right from their on page, too. Probably no way to mitigate this ...
Comment 16 David Keeler [:keeler] (use needinfo?) 2012-02-13 10:25:07 PST
(In reply to Markus Fischer from comment #15)

> Would the user see, being on you(t)ube.com, that a plugin is being activated
> via a third party? Or would it be just granted allowance due user previous
> preference on you(t)ube?

It would appear that youtube is trying to load some plugin. However, as the plugin's uri is something the user has never seen before, it would not be automatically played.

> And of course this is an idealized scenario: once they were able to break
> into the the server, they could of course server the malicious code right
> from their on page, too. Probably no way to mitigate this ...

Right. And, if they replaced something the user had seen and enabled before (e.g. the flash object that plays the videos on youtube), it would get played automatically.
Comment 17 Jason 2012-02-13 11:08:17 PST
I have "Click to Play" right now using Firefox via the "NoScript" extension. If NoScript settings are set to Globally Allow Scripts and the Embeddings are applied to whitelisted items, then NoScript doesn't interfere with anything on a page except for plugins, which it displays with a NoScript graphic. When a Plugin is encountered on a webpage, NoScript displays its graphic over the area. To play the plugin, I simply click the NoScript-gracphic and the plugin is allowed to start playing.
Comment 18 Willy_ Foo_Foo 2012-02-14 04:06:33 PST
hi guys
i have enable plugins.click_to_play (on win/linux)
it seems it blocks the plugin fine but when i click on the tap to play nothing happens.
Have to manually switch off plugins.click_to_play to work with plugins.
So any ideas what going on & when will this be fixed?
Comment 19 Jared Wein [:jaws] (please needinfo? me) 2012-02-23 21:09:03 PST
Created attachment 600301 [details] [diff] [review]
WIP patch for bug v3.1

This is based off of David's patch.

I have removed the context menu for the click to play plugins and plugins are now activated upon a single left-click on the plugin.

Still left to implement: Add the ability to remember settings for a site if plugins have been activated on a site for >= 3 times.
Comment 20 Jared Wein [:jaws] (please needinfo? me) 2012-02-24 07:32:26 PST
I'd be happy to take the work for this bug :)
Comment 21 :Margaret Leibovic 2012-02-24 07:56:08 PST
Jared, I mentioned this above in comment 14, but on mobile we had problems storing the plugins to play in an array. This was because we would re-set the array when navigating to a new page (like I see you're doing here in an unload listener), but we don't receive the "PluginClickToPlay" event when pages are loaded from the cache, so the array doesn't get re-populated in that case. You can look at the patch in bug 719875 for the solution we ended up using, but I think the real solution would be bug 723617.
Comment 22 Jared Wein [:jaws] (please needinfo? me) 2012-02-24 08:07:37 PST
(In reply to Margaret Leibovic [:margaret] from comment #21)
> Jared, I mentioned this above in comment 14, but on mobile we had problems
> storing the plugins to play in an array. This was because we would re-set
> the array when navigating to a new page (like I see you're doing here in an
> unload listener), but we don't receive the "PluginClickToPlay" event when
> pages are loaded from the cache, so the array doesn't get re-populated in
> that case. You can look at the patch in bug 719875 for the solution we ended
> up using, but I think the real solution would be bug 723617.

Thanks Margaret. I will include testing and fixing of loading from the cache in my next version.

Margaret: Is this also why the mobile version of click-to-play plugins traverses the DOM and looks for embeds? Is it considered OK that we are setting an attribute on the embed element to know that it was played? That part seemed confusing to me, unless web developers are asking for status on click-to-play from their scripts.
Comment 23 :Margaret Leibovic 2012-02-24 08:13:08 PST
(In reply to Jared Wein [:jaws] from comment #22)

> Margaret: Is this also why the mobile version of click-to-play plugins
> traverses the DOM and looks for embeds? Is it considered OK that we are
> setting an attribute on the embed element to know that it was played? That
> part seemed confusing to me, unless web developers are asking for status on
> click-to-play from their scripts.

Yes, this is the only reason we traverse the DOM. It's also the only reason that we set an attribute on the embed element. To be honest, I hadn't thought about web developers looking at that attribute, but I guess that could be useful. From a security standpoint, setting the attribute themselves wouldn't do anything, other than make the plugin object never get activated, so at least that's not something to worry about.
Comment 24 Jared Wein [:jaws] (please needinfo? me) 2012-02-24 08:17:51 PST
Ok, thanks for the explanation :)

I don't know of another way to keep track of these elements, but I recommend that we rename that attribute to mozPlayed to reduce breakage of scripts but also because "played" isn't an adopted standard as I understand it.

I'll file a separate bug for this refactoring and work on that too.
Comment 25 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-24 08:34:04 PST
Wait, our chrome code sets an attribute on the content DOM element here? That's generally not what we want, we don't want our chrome code injecting observable things into the DOM tree. Doing so causes problems with serialization/deserialization, cloning and using the cloned node, and can also generally confuse code on the page itself.

If our UI code really needs to store the played state on a plugin in a content DOM then we should store that state through internal interfaces in ways where the state does not get copied when a node is cloned etc. We already have nsIObjectLoadingContent, can we move that state there?
Comment 26 Jared Wein [:jaws] (please needinfo? me) 2012-02-24 08:40:09 PST
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #25)
> Wait, our chrome code sets an attribute on the content DOM element here?
> That's generally not what we want, we don't want our chrome code injecting
> observable things into the DOM tree. Doing so causes problems with
> serialization/deserialization, cloning and using the cloned node, and can
> also generally confuse code on the page itself.
> 
> If our UI code really needs to store the played state on a plugin in a
> content DOM then we should store that state through internal interfaces in
> ways where the state does not get copied when a node is cloned etc. We
> already have nsIObjectLoadingContent, can we move that state there?

This will be fixed in bug 730318.
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-24 08:46:53 PST
Excellent, thanks!
Comment 28 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-02-24 15:42:58 PST
Comment on attachment 600301 [details] [diff] [review]
WIP patch for bug v3.1

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

Seems to be working. I have quite a few comments but most are minor or ideas for follow-ups. :)

I think we should use "cursor: pointer" when hovering the click-to-play container to make the clickable area clear.

::: browser/base/content/browser.js
@@ +7098,5 @@
>  
> +      case "PluginClickToPlay":
> +        let clickToPlayBox = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> +        self.appendClickToPlayPlugin(plugin, self.isTooSmall(plugin, clickToPlayBox));
> +        clickToPlayBox.addEventListener("click", self.clickHandlerForClickToPlayPlugins.bind(self), true);

Note: These click event listeners are not removed after use.

@@ +7123,5 @@
> +    this.enablePlugins(browser);
> +  },
> +
> +  enablePlugins: function(browser) {
> +    // XXX not sure if we should enable plugins for the top document or not?

IMO, it would be nice to only enable plugins for documents of the same origin but that could probably be done in a follow-up if others agree.

@@ +7213,5 @@
> +                 callback: function() gPluginHandler.enablePlugins(browser)
> +               }],
> +    };
> +
> +    let notificationBar = notificationBox.getNotificationWithValue("ask-to-enable-plugin");

You could just use notification.barID here.

@@ +7217,5 @@
> +    let notificationBar = notificationBox.getNotificationWithValue("ask-to-enable-plugin");
> +    if (!browser.haveVisiblePlugins && !notificationBar) {
> +      notificationBox.appendNotification(notification.message,
> +                                         notification.barID,
> +                                         notification.iconURL,

Is there a reason we're using a notification box rather than a doorhanger?  I believe new tab-specific prompts should use doorhangers unless there is some reason they can't.

@@ +7221,5 @@
> +                                         notification.iconURL,
> +                                         notificationBox.PRIORITY_WARNING_MEDIUM,
> +                                         notification.buttons);
> +    } else if (!invisible && notificationBar) {
> +      notificationBox.removeNotification(notificationBar, true);

Add a comment about this case.  My understanding is that we're removing a notification bar that's showing if we later find a visible plugin.

Won't this cause a flickering notification bar in this case while the page is loading?  Otherwise, if the page is done loading it's a bit unusual for the notification bar to just disappear since there's no way to bring it back.  It would be nice to wait until we're more sure that we need a notification bar (perhaps with a timeout?) before showing one. For the page load case, it would be nice to wait until DOMContentLoaded (+ some max. time for huge pages) before showing a notification.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +116,5 @@
>  carbonFailurePluginsMessage.restartButton.label=Restart in 32-bit mode
>  carbonFailurePluginsMessage.restartButton.accesskey=R
> +askToEnablePluginsMessage.title=This page requests to use plugins.
> +askToEnablePluginsMessage.enable.label=Enable plugins
> +askToEnablePluginsMessage.enable.accesskey=E

Nit: Not a fan of the "askToEnablePluginsMessage" identifier.  "Ask" is usually implied in a notification message.  

Also, we should probably get UX feedback on the string if we haven't already. "Enable plugins" may be confused with enabling a plugin globally.  Perhaps "Enable plugins on this page"?

Note that doorhangers use a question for copy rather than a statement.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2112,1 @@
>      return NS_OK;

Could you add a comment about what this change is for?

::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
@@ +21,5 @@
>  <!ENTITY pluginWizard.finalPage.restart.label                "&brandShortName; needs to be restarted for the plugin(s) to work.">
>  
>  <!ENTITY missingPlugin                                       "A plugin is needed to display this content.">
> +<!ENTITY tapToPlayPlugin                                     "Tap here to activate plugin.">
> +<!ENTITY clickToPlayPlugin2                                  "Click here to activate plugin.">

Shouldn't plugin be plural here (id + string) to align with the implementation.
Comment 29 Jared Wein [:jaws] (please needinfo? me) 2012-02-28 14:30:16 PST
Comment on attachment 600301 [details] [diff] [review]
WIP patch for bug v3.1

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

::: browser/base/content/browser.js
@@ +7191,5 @@
> +    if (!browser.clickToPlayPlugins) {
> +      browser.haveVisiblePlugins = false;
> +      browser.clickToPlayPlugins = [];
> +      // Clear the click-to-play array upon leaving this page.
> +      plugin.ownerDocument.defaultView.top.addEventListener("unload",

We shouldn't use the unload event, since it will disable the bfcache. We should use pagehide here instead, but bug 723617 will have to be fixed first.
Comment 30 Ian Melven :imelven 2012-02-28 17:14:52 PST
(In reply to Matthew N. [:MattN] from comment #28)
> Comment on attachment 600301 [details] [diff] [review]
> WIP patch for bug v3.1
> 
> Review of attachment 600301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +7123,5 @@
> > +    this.enablePlugins(browser);
> > +  },
> > +
> > +  enablePlugins: function(browser) {
> > +    // XXX not sure if we should enable plugins for the top document or not?
> 
> IMO, it would be nice to only enable plugins for documents of the same
> origin but that could probably be done in a follow-up if others agree.

This will break, for example, plugins displaying advertising content from a 3rd party domain.
Comment 31 Cam 2012-02-28 17:32:09 PST
(In reply to Ian Melven :imelven from comment #30)
> (In reply to Matthew N. [:MattN] from comment #28)
> > Comment on attachment 600301 [details] [diff] [review]
> > WIP patch for bug v3.1
> > 
> > Review of attachment 600301 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +7123,5 @@
> > > +    this.enablePlugins(browser);
> > > +  },
> > > +
> > > +  enablePlugins: function(browser) {
> > > +    // XXX not sure if we should enable plugins for the top document or not?
> > 
> > IMO, it would be nice to only enable plugins for documents of the same
> > origin but that could probably be done in a follow-up if others agree.
> 
> This will break, for example, plugins displaying advertising content from a
> 3rd party domain.

Which honestly would be very welcome, at least as an option.
Comment 32 Jared Wein [:jaws] (please needinfo? me) 2012-03-06 18:10:33 PST
Created attachment 603544 [details] [diff] [review]
WIP patch for bug v3.2

This patch gets us closer to the implementation and is dependent on the patch in bug 730318.

Note that with this patch, the doorhanger will always show, there is no delay in showing the doorhanger, and there is no way to remember the settings for a site. Plugin elements created after activation of the plugins will not be auto-activated.
Comment 33 Jared Wein [:jaws] (please needinfo? me) 2012-03-20 13:13:45 PDT
Created attachment 607677 [details] [diff] [review]
Patch for bug (apply on top of patch for bug 730318)

This patch enables the click-to-play preference so we can get some testing on this in Nightly (and possibly Aurora).

If the other parts of click-to-play plugins aren't landed in time for the merge to Aurora, then we'll change the pref to false for Fx14 during the Aurora cycle.
Comment 34 Dão Gottwald [:dao] 2012-03-20 13:25:06 PDT
Comment on attachment 607677 [details] [diff] [review]
Patch for bug (apply on top of patch for bug 730318)

>+  gBrowser.addEventListener("load",               function(aEvent) {
>+    let browser = gBrowser.getBrowserForDocument(aEvent.target);
>+    gPluginHandler.showPluginClickToPlayDoorhanger(browser);
>+  }, true);

There are progress listeners that should let you call showPluginClickToPlayDoorhanger at the right time. You shouldn't need a load event listener for this.

Also, prefix custom properties you put on browsers with an underscore.

>+@media (-moz-touch-enabled) {
>+  :-moz-handler-clicktoplay .msgClickToPlay {
>+    display: none;
>+  }
>+  :-moz-handler-clicktoplay .msgTapToPlay {
>+    display: block;
>+  }
>+}

This works only on windows.
Comment 35 Jared Wein [:jaws] (please needinfo? me) 2012-03-20 18:20:47 PDT
Created attachment 607815 [details] [diff] [review]
Patch for bug v2 (apply on top of patch for bug 730318)

I've fixed the issues that Dao pointed out and moved the -moz-touch-enabled code to the themes so that we can apply the -moz-touch-enabled correctly.

I've also pushed a patch to inbound for bug 737675 which added support for -moz-touch-enabled on Android and Gonk.
Comment 36 Dão Gottwald [:dao] 2012-03-21 00:44:36 PDT
Comment on attachment 607815 [details] [diff] [review]
Patch for bug v2 (apply on top of patch for bug 730318)

>   startDocumentLoad: function XWB_startDocumentLoad(aRequest) {
>     // clear out feed data
>     gBrowser.selectedBrowser.feeds = null;
> 
>     // clear out search-engine data
>     gBrowser.selectedBrowser.engines = null;
> 
>+    // initialize the click-to-play state
>+    gBrowser.selectedBrowser._loadEventHandled = false;
>+    gBrowser.selectedBrowser._clickToPlayDoorhangerShown = false;

You don't want this for background tabs?

>+  activatePlugins: function(aContentWindow) {
>+    let cwu = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                            .getInterface(Ci.nsIDOMWindowUtils);
>+    let plugins = cwu.plugins;
>+    for (let i in plugins) {
>+      let plugin = plugins[i];

for (let plugin of plugins) {

>+    let isAnyPluginVisible = false;
>+    for (let plugin of plugins) {
>+      let overlay = plugin.ownerDocument.getAnonymousElementByAttribute(plugin, "class", "mainBox");
>+      if (overlay && !gPluginHandler.isTooSmall(plugin, overlay))
>+        isAnyPluginVisible = true;
>+    }
>+    if (plugins && plugins.length && !isAnyPluginVisible) {

if plugins were null or plugins.length were 0, isAnyPluginVisible wouldn't be true

>-<!ENTITY clickToPlayPlugin                                   "Tap here to activate plugin.">
>+<!ENTITY tapToPlayPlugin                                     "Tap here to activate plugin."> <!-- Mobile only has one type of plugin possible. -->

This looks like it should be a proper LOCALIZATION NOTE.
Comment 37 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-03-21 01:22:39 PDT
>+      if (/^about:/.test(aWebProgress.DOMWindow.document.documentURI)) {
>+        aBrowser.addEventListener("click", BrowserOnClick, false);
>+        aBrowser.addEventListener("pagehide", function () {
>+          aBrowser.removeEventListener("click", BrowserOnClick, false);
>+          aBrowser.removeEventListener("pagehide", arguments.callee, true);
>+        }, true);
>+
>+        // We also want to make changes to page UI for unprivileged about pages.
>+        BrowserOnAboutPageLoad(aWebProgress.DOMWindow.document);
>+      }

I think this part should use function name instead of arguments.callee.
e.g:

aBrowser.addEventListener("click", BrowserOnClick, false);
aBrowser.addEventListener("pagehide", function onPagehide() {
  aBrowser.removeEventListener("click", BrowserOnClick, false);
  aBrowser.removeEventListener("pagehide", onPagehide, true);
}, true);
Comment 38 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-03-21 01:58:29 PDT
At patch v2, I have something on my chest.

>+      let options = { timeout: Date.now() + 30000 };
>+      PopupNotifications.show(aBrowser, "click-to-play-plugins",
>+                              messageString, "addons-notification-icon",
>+                              action, null, options);
>+    }

The patch v2 uses "addons-notification-icon" icon as popup notification icon.
However, this icon is used for add-ons installations. The role of click-to-play-plugins is distinct from it. Using same icon may causes some problem of expandability by add-ons.
So Should it make the new icon element for click-to-play?
Comment 39 :Felipe Gomes (needinfo me!) 2012-03-21 01:58:40 PDT
Comment on attachment 607815 [details] [diff] [review]
Patch for bug v2 (apply on top of patch for bug 730318)

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

I'm a little bit uncertain about the timing when the doorhanger might kick in. Is it known if by the time STATE_STOP happens, the style resolution has happened and you will know what are the plugins' dimensions?

::: browser/base/content/browser.js
@@ +5120,5 @@
>      // document URI is not yet the about:-uri of the error page.
>  
> +    if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
> +      gPluginHandler.showPluginClickToPlayDoorhanger(aBrowser);
> +

we cannot have heavy operations in progress listeners as they can slow down networking. Since this call will iterate through plugins, it is potentially unbounded, so we should fire it from a setTimeout(.., 0)


what is the intended behavior if the STATE_STOP happens for a background tab? From what I can tell this will display the doorhanger, which doesn't seem right as the doorhangers are associated with the current tab

@@ +7189,5 @@
> +    overlay.addEventListener("click", function(e) {
> +      if (e.button == 0)
> +        gPluginHandler.activatePlugins(contentWindow);
> +    }, true);
> +

can you reuse addLinkClickCallback here? at least you'll need to check for e.isTrusted but you should just use the same function that the plugin reload uses.

your function can also avoid the closure by using event.target.defaultView.top instead of contentWindow

@@ +7206,5 @@
> +    let isAnyPluginVisible = false;
> +    for (let plugin of plugins) {
> +      let overlay = plugin.ownerDocument.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> +      if (overlay && !gPluginHandler.isTooSmall(plugin, overlay))
> +        isAnyPluginVisible = true;

break the loop (or the function) when the first visible plugin is found

::: toolkit/mozapps/plugins/content/pluginProblem.xml
@@ +53,1 @@
>          <stylesheet src="chrome://mozapps/skin/plugins/pluginProblem.css"/>

Is this change in the stylesheet ordering necessary?
Comment 40 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-21 13:03:52 PDT
(In reply to Felipe Gomes (:felipe) from comment #39)
> can you reuse addLinkClickCallback here? at least you'll need to check for
> e.isTrusted but you should just use the same function that the plugin reload
> uses.

I suspect the isTrusted checks are leftovers from pre-bug 289940 code. They shouldn't be needed unless the relevant addEventListener calls are passing aWantsUntrusted=true (worth testing, though - in a followup if desired).
Comment 41 Jared Wein [:jaws] (please needinfo? me) 2012-03-21 15:44:41 PDT
Created attachment 608121 [details] [diff] [review]
Patch for bug v3 (apply on top of patch for bug 730318)

(In reply to Dão Gottwald [:dao] from comment #36)
> >+    if (plugins && plugins.length && !isAnyPluginVisible) {
> 
> if plugins were null or plugins.length were 0, isAnyPluginVisible wouldn't
> be true

Yeah, this is by design. We should only show the doorhanger if there are more than 0 plugins on a page and none of them are visible.

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #37)
> I think this part should use function name instead of arguments.callee.

My patch only changes the indentation levels of this code, so I don't want to increase the scope of this bug by cleaning up other code nearby. We should just file a separate bug to clean up usages of arguments.callee.

(In reply to Felipe Gomes (:felipe) from comment #39)
> I'm a little bit uncertain about the timing when the doorhanger might kick
> in. Is it known if by the time STATE_STOP happens, the style resolution has
> happened and you will know what are the plugins' dimensions?

STATE_STOP is as similar to the load event as we can get. I haven't seen any issues from this in my testing, but if we notice some down the road then I think we can address them at that time.

> what is the intended behavior if the STATE_STOP happens for a background
> tab? From what I can tell this will display the doorhanger, which doesn't
> seem right as the doorhangers are associated with the current tab

The doorhanger code handles this properly and doesn't show doorhangers for wrong browser.
 
> ::: toolkit/mozapps/plugins/content/pluginProblem.xml
> @@ +53,1 @@
> >          <stylesheet src="chrome://mozapps/skin/plugins/pluginProblem.css"/>
> 
> Is this change in the stylesheet ordering necessary?

The skin stylesheet should follow the content stylesheet. I checked mxr and I can't find any other instance of a skin stylesheet preceding a content stylesheet.

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #40)
> I suspect the isTrusted checks are leftovers from pre-bug 289940 code. They
> shouldn't be needed unless the relevant addEventListener calls are passing
> aWantsUntrusted=true (worth testing, though - in a followup if desired).

I added the e.isTrusted check for consistency and extra precaution for now. I think we should file a follow-up bug to investigate the necessity of the e.isTrusted checks and remove them after we know it is safe to do so.
Comment 42 :Felipe Gomes (needinfo me!) 2012-03-22 23:36:01 PDT
Comment on attachment 608121 [details] [diff] [review]
Patch for bug v3 (apply on top of patch for bug 730318)

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

::: browser/base/content/browser.js
@@ +5169,5 @@
>  
>      if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
> +        Components.isSuccessCode(aStatus)) {
> +      setTimeout(function() { gPluginHandler.showPluginClickToPlayDoorhanger(aBrowser) }, 0);
> +

have the function body in an own line to make it easier to read
Comment 43 Jared Wein [:jaws] (please needinfo? me) 2012-03-23 11:39:53 PDT
Moving sec-review-needed to bug 738698 as OK'd by curtisk.

10:28 AM <jaws> k, i just think that i should file a new bug that is a meta bug for the whole feature and do the sec-review on that bug
10:28 AM <curtisk> jaws: that would be fine
10:28 AM <jaws> ok thanks
10:45 AM <jaws> ok, then i'll remove the sec-review-needed flag from 711552
10:45 AM <curtisk> wfm
Comment 44 Jared Wein [:jaws] (please needinfo? me) 2012-03-27 13:59:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/77319b44907b
Comment 45 Jared Wein [:jaws] (please needinfo? me) 2012-03-27 14:03:56 PDT
Created attachment 609876 [details] [diff] [review]
Patch for bug (part 2): Add style changes to gnomestripe and pinstripe

I forgot to make the necessary style changes for gnomestripe and pinstripe.
Comment 46 Jared Wein [:jaws] (please needinfo? me) 2012-03-27 15:28:25 PDT
Part 1 of this patch was backed out due to test failures. I'm investigating now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f74dc1b7f52
Comment 47 Jared Wein [:jaws] (please needinfo? me) 2012-03-28 15:26:10 PDT
Created attachment 610322 [details] [diff] [review]
Patch for bug v4

This patch encompasses the work from the two previous r+'d patches, but refactors some of the code to make it less complicated.

Showing the notification is no longer delayed until the load event, and we no longer loop over all plugin objects for each click-to-play event. This patch also properly handles back-forward navigation.

On pages where plugins are considered visible by our metric, but not user facing, such as Pandora, we will show the notification in a dismissed state so users can still enable plugins. We should improve our metric for determining if a plugin is "visible", but that shouldn't be included in this bug.

Pushed to try server: https://tbpl.mozilla.org/?tree=Try&rev=93971b34ffd8
Comment 48 Jared Wein [:jaws] (please needinfo? me) 2012-03-28 17:26:12 PDT
Comment on attachment 610322 [details] [diff] [review]
Patch for bug v4

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

::: browser/base/content/browser.js
@@ +7250,5 @@
> +      return;
> +    }
> +
> +    let overlay = doc.getAnonymousElementByAttribute(aPlugin, "class", "mainBox");
> +    let contentWindow = browser.contentWindow;

|contentWindow| is unused here. I'll remove it.
Comment 49 Mozilla RelEng Bot 2012-03-28 19:32:16 PDT
Try run for 93971b34ffd8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=93971b34ffd8
Results (out of 224 total builds):
    exception: 3
    success: 196
    warnings: 23
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-93971b34ffd8
Comment 50 Dão Gottwald [:dao] 2012-03-29 00:48:54 PDT
Comment on attachment 610322 [details] [diff] [review]
Patch for bug v4

> function pageShowEventHandlers(event) {
>   // Filter out events that are not about the document load we are interested in
>   if (event.originalTarget == content.document) {
>     charsetLoadListener(event);
>     XULBrowserWindow.asyncUpdateUI();
>+
>+    // |event.persisted| is true when the page is loaded from the BF cache.
>+    if (event.persisted)
>+      gPluginHandler.reshowClickToPlayNotificationOnPageShow(event);
>   }
> }

>+  reshowClickToPlayNotificationOnPageShow: function(aEvent) {
>+    if (!Services.prefs.getBoolPref("plugins.click_to_play"))
>+      return;
>+
>+    let browser = gBrowser.getBrowserForDocument(aEvent.target.defaultView.top.document);

This is just gBrowser.selectedBrowser.

"function (aEvent) {" for anonymous functions. Better yet, give this function a name.
Comment 51 :Felipe Gomes (needinfo me!) 2012-03-29 01:05:49 PDT
Comment on attachment 610322 [details] [diff] [review]
Patch for bug v4

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

As we talked, with this new approach the first plugin will determine if the doorhanger is shown or not. Ultimately, the problem is that there is no perfect way to determine if the doorhanger needs to be displayed, given the nature of web page loading, styling, dynamic changes... So we will need to talk with UX to see what are the acceptable options.

I'm inclined to think that never showing the doorhanger, but always adding the doorhanger icon is the way to go, specially when considering the goals of the feature (security for outdated plugins for example).

I had already added some comments to the patch so I'm posting them as they may still be relevant afterwards.

::: browser/base/content/browser.js
@@ +258,5 @@
>      XULBrowserWindow.asyncUpdateUI();
> +
> +    // |event.persisted| is true when the page is loaded from the BF cache.
> +    if (event.persisted)
> +      gPluginHandler.reshowClickToPlayNotificationOnPageShow(event);

Brief comment here to explain that this is needed because the click-to-play events are not re-fired on BF cache navigation

@@ +7173,5 @@
>            overlay.style.visibility = "hidden";
>      }
>    },
>  
> +  activatePlugins: function(aContentWindow) {

Now that visible plugins will add the dismissed doorhanger, activatePlugins needs to remove the click-to-play icon from the URL bar

@@ +7183,5 @@
> +    for (let plugin of plugins) {
> +      let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +      if (!objLoadingContent.activated)
> +        objLoadingContent.playPlugin();
> +      plugin.removeEventListener("click", plugin.clickHandler, true);

As Olli pointed out on IRC, no need to worry about this as the listener should go away by itself when the overlay is removed due to the plugins being activated

@@ +7245,5 @@
> +  _handleClickToPlayEvent: function(aPlugin) {
> +    let browser = gBrowser.getBrowserForDocument(aPlugin.ownerDocument.defaultView.top.document);
> +    let doc = aPlugin.ownerDocument;
> +    if (browser._clickToPlayPluginsActivated) {
> +      gPluginHandler.activatePlugins(doc.defaultView.top);

a bit unecessary to iterate over all plugins

@@ +7254,5 @@
> +    let contentWindow = browser.contentWindow;
> +    overlay.addEventListener("click", function(aEvent) {
> +      if (aEvent.button == 0 && aEvent.isTrusted)
> +        gPluginHandler.activatePlugins(aEvent.target.ownerDocument.defaultView.top);
> +    }, true);

For a follow-up: what's the accessibility status of the feature? If the overlay is tabbable you should also listen for an Enter keypress

@@ +7264,5 @@
> +    if (!browser._clickToPlayDoorhangerShown)
> +      gPluginHandler._showClickToPlayNotification(browser, browser._clickToPlayOverlayShown);
> +  },
> +
> +  reshowClickToPlayNotificationOnPageShow: function(aEvent) {

This name is too specific, no need to include "OnPageShow". The comment added on the caller makes the purpose clear

@@ +7277,5 @@
> +      let overlay = aPlugin.ownerDocument.getAnonymousElementByAttribute(aPlugin, "class", "mainBox");
> +      return overlay && !gPluginHandler.isTooSmall(aPlugin, overlay);
> +    }
> +    let plugins = cwu.plugins;
> +    let isAnyPluginVisible = plugins.some(isPluginVisible)

missing semicolon at the end
Comment 52 Jared Wein [:jaws] (please needinfo? me) 2012-03-29 18:23:51 PDT
Created attachment 610781 [details] [diff] [review]
Patch for bug v5

I've addressed the various feedback.

The other plugin overlays have a link that is added to them which accepts the "keydown" event and checks for the Return key. We should add something like that, but we'll need to figure out how we want to do implement it since a link might not make as much sense here since there will be no document navigation occurring.
Comment 53 Dão Gottwald [:dao] 2012-03-30 00:56:13 PDT
Comment on attachment 610781 [details] [diff] [review]
Patch for bug v5

>+  reshowClickToPlayNotification: function PH_reshowClickToPlayNotification(aEvent) {
>+    if (!Services.prefs.getBoolPref("plugins.click_to_play"))
>+      return;
>+
>+    let contentWindow = gBrowser.selectedBrowser.contentWindow;
>+    let cwu = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                            .getInterface(Ci.nsIDOMWindowUtils);
>+    if (cwu.plugins.length)
>+      gPluginHandler._showClickToPlayNotification(browser);
>+  },

aEvent is unused
Comment 54 :Felipe Gomes (needinfo me!) 2012-03-31 18:40:57 PDT
Comment on attachment 610781 [details] [diff] [review]
Patch for bug v5

We did an in person review of this patch, there were only some nits to be fixed and Jared is gonna check something on youtube before posting the next version
Comment 55 Jared Wein [:jaws] (please needinfo? me) 2012-04-02 13:51:55 PDT
Created attachment 611580 [details] [diff] [review]
Patch for bug v6

Fixed some nits. I couldn't reproduce the notification-disappearing issue I was seeing on YouTube last week.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1cc825e6fb87
Comment 56 Jared Wein [:jaws] (please needinfo? me) 2012-04-02 15:52:36 PDT
Created attachment 611629 [details] [diff] [review]
Tests for patch
Comment 57 :Felipe Gomes (needinfo me!) 2012-04-02 17:56:46 PDT
Comment on attachment 611629 [details] [diff] [review]
Tests for patch

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

Thanks for writing these tests, they look good! It'd be great to include a few more things:

- a test for the case when the plugin is too small such that the overlay can't be shown, and then activate it via the doorhanger

- It should also be possible to test the reshowDoorhanger function by using gTestBrowser.goBack()

- And lastly test that the PluginClickToPlay event is fired 3 times when plugin_test2.html loads (asking because I don't see any tests about this in the patch that implemented that event)


r+ for the ones implemented in this patch with the following 2 nits:

::: browser/base/content/test/browser_pluginnotification.js
@@ +213,5 @@
> +  var doc = gTestBrowser.contentDocument;
> +  var rect = doc.getAnonymousElementByAttribute(plugin1, "class", "mainBox").getBoundingClientRect();
> +  ok(rect.width == 200, "Test 9a, Plugin overlay rect should have 200px width before being clicked");
> +  ok(rect.height == 200, "Test 9a, Plugin overlay rect should have 200px height before being clicked");
> +

Why not also check the plugin.activated state in these tests?

@@ +239,5 @@
> +  var plugin3Rect = doc.getAnonymousElementByAttribute(plugin3, "class", "mainBox").getBoundingClientRect();
> +  ok(plugin3Rect.width == 0, "Test 9b, Plugin3 should have click-to-play overlay with zero width");
> +  ok(plugin3Rect.height == 0, "Test 9b, Plugin3 should have click-to-play overlay with zero height");
> +
> +  Services.prefs.clearUserPref("plugins.click_to_play");

use registerCleanupFunction at the beginning of test() to reset this pref
Comment 58 Mozilla RelEng Bot 2012-04-02 20:31:41 PDT
Try run for 1cc825e6fb87 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1cc825e6fb87
Results (out of 287 total builds):
    success: 260
    warnings: 23
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-1cc825e6fb87
Comment 59 Jared Wein [:jaws] (please needinfo? me) 2012-04-04 12:17:18 PDT
Created attachment 612291 [details] [diff] [review]
Tests for patch (now with more tests!)

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=f9cb29edc917
Comment 62 alex_mayorga 2012-04-06 08:46:52 PDT
Thanks for this!

I've traded Flashblock for plugins.click_to_play;true on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:14.0) Gecko/20120406 Firefox/14.0a1 ID:20120406031222 and like it for the most part.

My only complaints are:
- bug 742753
- an "Always activate plugins" option in the door hanger or
- a whitelist

Also, it might be Oracle's fault, but the behavior of clicking "Do I have Java?" at http://java.com/en/ with this feature on is less than ideal.

Should follow up bugs be filled?

Test URLs:
http://grooveshark.com
http://www.adobe.com/software/flash/about/
http://javatester.org/version.html
http://java.com/en/download/installed.jsp
Comment 63 Ian Melven :imelven 2012-04-06 09:47:56 PDT
(In reply to alex_mayorga from comment #62)
> Thanks for this!
> 
> I've traded Flashblock for plugins.click_to_play;true on Mozilla/5.0
> (Windows NT 6.1; Win64; x64; rv:14.0) Gecko/20120406 Firefox/14.0a1
> ID:20120406031222 and like it for the most part.
> 
> My only complaints are:
> - bug 742753
> - an "Always activate plugins" option in the door hanger or
> - a whitelist
> 
> Also, it might be Oracle's fault, but the behavior of clicking "Do I have
> Java?" at http://java.com/en/ with this feature on is less than ideal.
> 
> Should follow up bugs be filled?
> 
> Test URLs:
> http://grooveshark.com
> http://www.adobe.com/software/flash/about/
> http://javatester.org/version.html
> http://java.com/en/download/installed.jsp

Alex, thanks for the feedback !

please see https://wiki.mozilla.org/Opt-in_activation_for_plugins which described the overall plan/current proposal for click to play - a whitelist is definitely a strong possibility. 

the feature proposal is currently being discussed on mozilla.dev.security and we asked the Mozilla UX team for feedback yesterday, also. 

> Also, it might be Oracle's fault, but the behavior of clicking "Do I have
> Java?" at http://java.com/en/ with this feature on is less than ideal.

is this the same as the problem with silverlight described in bug 743102 ? from comment 4 in that bug it sounds like it might be. 

Please note that bug 738698 is the meta bug for 'click to play' :)
Comment 64 Willy_ Foo_Foo 2012-04-06 10:53:17 PDT
Thanks for this! too

i also have traded Flashblock for plugins.click_to_play;true

But by when it will be stable enough to test?
will it have the option to control(enable on demand) for all types of plugins?
& what about say a page has two flash player windows but want to play only the second one
will it be possible ? (flashblock allows this)
Comment 65 Dave Garrett 2012-04-06 14:50:16 PDT
A whitelist may be a required feature if at least to cope with any sites that simply don't work correctly with click-to-play. For example, on http://nyan.cat/ the sound doesn't work with click-to-play, either this implementation or Flashblock. Whitelisting it is required to get it working and there are other sites where this may be the case too.
Comment 66 Ian Melven :imelven 2012-04-06 14:58:55 PDT
(In reply to Dave Garrett from comment #65)
> A whitelist may be a required feature if at least to cope with any sites
> that simply don't work correctly with click-to-play. For example, on
> http://nyan.cat/ the sound doesn't work with click-to-play, either this
> implementation or Flashblock. Whitelisting it is required to get it working
> and there are other sites where this may be the case too.

i see whitelisting as more meaning 'this plugin is always allowed to play by default (without a click)' across all sites. i think we definitely want to have an easy way for a user to choose 'always allow this plugin to play by default on a specific site as well.

This is covered as a use case (imo) with "User is tired of always clicking to play a given plugin (i.e. YouTube, or their favorite Java game site)" in https://wiki.mozilla.org/Opt-in_activation_for_plugins although the exact UX around this use case is still being discussed on mozilla.dev.security

Please note that bug 738698 is the meta bug for 'click to play' and this bug is marked fixed - mozilla.dev.security is probably a better place to discuss click to play overall. Thanks !
Comment 67 Biju 2012-04-18 00:57:07 PDT
I wish we have better UI when click-to-play is enabled.
Some thing like a play button [ > ] on a semi-opaque background to replace the plug-in display area.

Do we have a bug on that?
Comment 68 David Keeler [:keeler] (use needinfo?) 2012-04-18 09:46:28 PDT
(In reply to Biju from comment #67)
> I wish we have better UI when click-to-play is enabled.
> Some thing like a play button [ > ] on a semi-opaque background to replace
> the plug-in display area.
> 
> Do we have a bug on that?

I'm not sure what you mean here. Do you want to just replace what is there now with a play button? Or are you proposing a mechanism that loads the plugin, "pauses" it, and throws the transparent overlay on top?
The latter is probably not desirable because it does away with the security and resource-management gains we get from this feature.
Comment 69 Willy_ Foo_Foo 2012-04-18 10:44:13 PDT
(In reply to David Keeler from comment #68)

> I'm not sure what you mean here. Do you want to just replace what is there
> now with a play button?
Make it beautiful(yet light) so has it looks cool/pretty/appealing to end users 
>Or are you proposing a mechanism that loads the
>plugin, "pauses" it, and throws the transparent overlay on top?
> The latter is probably not desirable because it does away with the security
> and resource-management gains we get from this feature.
Touche
Comment 70 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-04-18 12:53:28 PDT
Probably better to file a new bug on that and to leave this bug as it is.
Comment 71 Dave Garrett 2012-04-18 19:52:55 PDT
I too have been thinking the click-to-play icon could be made more clear. I've filed bug 746859 to ask that it be changed from just the standard plugin icon to one a bit larger with a play icon. It'd be a simple change that would let users quickly see what's going on without having to read the text.
Comment 72 Ian Melven :imelven 2012-04-18 21:55:33 PDT
(In reply to Dave Garrett from comment #71)
> I too have been thinking the click-to-play icon could be made more clear.
> I've filed bug 746859 to ask that it be changed from just the standard
> plugin icon to one a bit larger with a play icon. It'd be a simple change
> that would let users quickly see what's going on without having to read the
> text.

Thanks for your feedback, Dave, I moved the dependency on bug 746859 which you filed over to bug 738698 which is the overall tracking bug for click to play, as this bug is already marked RESOLVED FIXED.
Comment 73 Dão Gottwald [:dao] 2012-04-19 00:13:26 PDT
(In reply to Ian Melven :imelven from comment #72)
> as this bug is already marked RESOLVED FIXED.

This doesn't really matter.
Comment 74 Yunier J. 2012-04-19 09:41:39 PDT
This a feature to many people like, and I want ask to you if this will come activated on Firefox 14?
Too I have a idea, why not create a global configuration on about:permissions like passwords, cookies, shared location and others?
Comment 75 Jared Wein [:jaws] (please needinfo? me) 2012-04-19 10:57:46 PDT
(In reply to Yunier José Sosa Vázquez from comment #74)
> This a feature to many people like, and I want ask to you if this will come
> activated on Firefox 14?

This feature will be disabled by default in Firefox 14

> Too I have a idea, why not create a global configuration on
> about:permissions like passwords, cookies, shared location and others?

A global configuration was just added to about:permissions in bug 711618 :)
Comment 76 Yunier J. 2012-04-19 11:10:07 PDT
(In reply to Jared Wein [:jaws] from comment #75)
> (In reply to Yunier José Sosa Vázquez from comment #74)
> > This a feature to many people like, and I want ask to you if this will come
> > activated on Firefox 14?
> 
> This feature will be disabled by default in Firefox 14
> 
> > Too I have a idea, why not create a global configuration on
> > about:permissions like passwords, cookies, shared location and others?
> 
> A global configuration was just added to about:permissions in bug 711618 :)

Great! But I wished this would have been enabled by default in Firefox 14. I'll see that many users know this to activate plugins.click_to_play == true ;-)
Comment 77 Ian Melven :imelven 2012-04-19 11:19:51 PDT
(In reply to Yunier José Sosa Vázquez from comment #76)
> (In reply to Jared Wein [:jaws] from comment #75)
> > (In reply to Yunier José Sosa Vázquez from comment #74)
> > > This a feature to many people like, and I want ask to you if this will come
> > > activated on Firefox 14?
> > 
> > This feature will be disabled by default in Firefox 14
> > 
> > > Too I have a idea, why not create a global configuration on
> > > about:permissions like passwords, cookies, shared location and others?
> > 
> > A global configuration was just added to about:permissions in bug 711618 :)
> 
> Great! But I wished this would have been enabled by default in Firefox 14.
> I'll see that many users know this to activate plugins.click_to_play == true
> ;-)

Thanks Yunier ! bug 738698 is the tracking bug for click to play. Also, please see https://wiki.mozilla.org/Opt-in_activation_for_plugins for some other ideas we are exploring or planning to implement - there's a lot to discuss and decide before click to play is turned on by default :)
Comment 78 Yunier J. 2012-04-19 13:00:13 PDT
Sure Ian, I help in all whatever it takes. I commented here https://bugzilla.mozilla.org/show_bug.cgi?id=738698#c12

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