Last Comment Bug 742753 - Click to Play should permit each element
: Click to Play should permit each element
Status: VERIFIED FIXED
[parity-chrome][parity-opera]
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: 14 Branch
: All All
: -- normal with 4 votes (vote)
: mozilla15
Assigned To: David Keeler [:keeler] (use needinfo?)
:
Mentors:
: 747638 (view as bug list)
Depends on:
Blocks: click-to-play
  Show dependency treegraph
 
Reported: 2012-04-05 09:00 PDT by Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
Modified: 2013-06-25 06:53 PDT (History)
21 users (show)
jaws: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase.html (1.09 KB, text/html)
2012-04-05 09:00 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
no flags Details
patch (19.60 KB, patch)
2012-04-19 14:47 PDT, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Review
patch v2 + test (16.17 KB, patch)
2012-05-11 14:34 PDT, David Keeler [:keeler] (use needinfo?)
jaws: feedback+
Details | Diff | Review
patch v3 + test (17.35 KB, patch)
2012-05-14 10:33 PDT, David Keeler [:keeler] (use needinfo?)
jaws: review+
Details | Diff | Review
patch v4 + test (17.32 KB, patch)
2012-05-14 13:11 PDT, David Keeler [:keeler] (use needinfo?)
dkeeler: review+
Details | Diff | Review
patch v5 + tests v2 (20.91 KB, patch)
2012-05-15 11:14 PDT, David Keeler [:keeler] (use needinfo?)
jaws: review+
Details | Diff | Review

Description Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-05 09:00:53 PDT
Created attachment 612565 [details]
testcase.html

STR:
0. Set plugins.click_to_play is true.
1. Open a page which has some elements using plugins, e.g: testcase.html
2. Click a plugin element(A).
3. Start plugin of element(A).

Result:
Start plugins of other elements too.


I think that playing plugin should a clicked element only.

This behavior is too simple. Firefox plays all plugins regardless of whether user wants to play all plugins or not.
It will be risk if a page has malignant contents played with plugins.
And from the viewpoint of usability, this behavior goes against user's expectation which Firefox plays plugins in clicked element only.

The current Firefox click-to-play model notifies plugins are used in page by doorhanger notification.
So Using doorhanger action is better when user want to plays all plugins in pages.
Comment 1 Ian Melven :imelven 2012-04-05 10:42:28 PDT
The problem with this approach is that, in the case of Flash, often SWF content relies on other SWFs including 'invisible' helper SWFs and will not work correctly. How can the user know what SWF's to click in what order to get a correctly functioning web page ? Click to play doesn't actually 'play' the content as well, it just allows the plugin instance to load normally.
Comment 2 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-05 12:10:48 PDT
(In reply to Ian Melven :imelven from comment #1)
> The problem with this approach is that, in the case of Flash, often SWF
> content relies on other SWFs including 'invisible' helper SWFs and will not
> work correctly. How can the user know what SWF's to click in what order to
> get a correctly functioning web page ? Click to play doesn't actually 'play'
> the content as well, it just allows the plugin instance to load normally.

When it need "helper" SWFs, user may be play *all plugins* by doorhanger or context-menu on placeholder of elements. (The context menu need to implement it...)
Comment 3 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-05 12:11:04 PDT
Other browsers (Chrome, Opera) permit to each element with to click an element.
Also IE9, this behavior is like the Firefox current behavior. But IE9's approach does not provide click an element to play model, so IE9 permits via notification. IE9 approach makes user recognize that all plugins are switched by this notification.
The click an element to play approach makes user recognize that only clicked element is played if user click an element.
Plugin elements are not necessarily placed on near. So user almost always think that they are not related to single configuration for playing.
Comment 4 Ian Melven :imelven 2012-04-06 09:51:24 PDT
(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #2)
> (In reply to Ian Melven :imelven from comment #1)
> > The problem with this approach is that, in the case of Flash, often SWF
> > content relies on other SWFs including 'invisible' helper SWFs and will not
> > work correctly. How can the user know what SWF's to click in what order to
> > get a correctly functioning web page ? Click to play doesn't actually 'play'
> > the content as well, it just allows the plugin instance to load normally.
> 
> When it need "helper" SWFs, user may be play *all plugins* by doorhanger or
> context-menu on placeholder of elements. (The context menu need to implement
> it...)

we could try to activate all hidden swf's when a visible swf is clicked, perhaps ? (and only activate the actual visible plugin instance that was clicked) ? does that sound more like what you are asking for ? also please note jared's work so far is a first pass on the feature :)
Comment 5 alex_mayorga 2012-04-06 11:47:25 PDT
How about "try to activate all hidden swf's [that comply with same origin policy] when a visible swf is clicked (and only activate the actual visible plugin instance that was clicked)"?
Comment 6 Ian Melven :imelven 2012-04-06 12:35:21 PDT
(In reply to alex_mayorga from comment #5)
> How about "try to activate all hidden swf's [that comply with same origin
> policy] when a visible swf is clicked (and only activate the actual visible
> plugin instance that was clicked)"?

Personally, I don't think we should expect users to understand what a hidden plugin is - and helper SWF's are not necessarily same domain with their main SWF. But I do think we should aim for only activating the visible SWF (or other plugin content, I don't want to be too Flash-centric here) that has actually been clicked.
Comment 7 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-06 17:05:02 PDT
(In reply to Ian Melven :imelven from comment #4)
> we could try to activate all hidden swf's when a visible swf is clicked,
> perhaps ? (and only activate the actual visible plugin instance that was
> clicked) ? does that sound more like what you are asking for ? also please
> note jared's work so far is a first pass on the feature :)

I think that "only activate the actual visible plugin instance that was clicked".
At click-to--play, Firefox should only activate the plugin that user permits (by click) expressly. If it need to activate all plugins, user will permits to activate all plugins by doorhanger command.
Comment 8 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-04-06 17:30:18 PDT
(In reply to alex_mayorga from comment #5)
> How about "try to activate all hidden swf's [that comply with same origin
> policy] when a visible swf is clicked (and only activate the actual visible
> plugin instance that was clicked)"?

I think that it is not good. Its reason is written in Ian's comment #5.
And also, from the viewpoint of security, the same origin can't guarantee the safety of plugin contents provided from its domain. If a site is defaced by cracker, same origin policy is meaningless.
Comment 9 Lozzy 2012-04-11 00:17:57 PDT
I agree with :saneyuki for a number of reasons;

- We shouldn't be going into this half heartedly, this is a great opportunity to increase security and (let's be honest) help curtail the use of plugins generally. It will be much less effective if everything is enabled with the first enabled element.
- As a security concious user, I am expecting that the item I enable is the only one enabled.
- Follow Opera and Chrome on this one, parity will help the cause tremendously, both in therms of users and webdevs. The only outcome I see to not following them is lower security.

If this necessitates webdevs to change their strategy, so be it. They will have to do so to work on Chrome & Opera anyway. I don't see it as a sensible strategy to dilute this feature.
Comment 10 Lozzy 2012-04-11 00:30:03 PDT
Additional reasons I have come to appreciate Noscript's model of click to play each element (or enable all if desired);

- You can control how many plugins run at once, helping avoid situations where a number of plugins will slow down browsing
- Various annoyances are often delivered through plugins (auto playing sound/video), and having control over those is great
Comment 11 David Keeler [:keeler] (use needinfo?) 2012-04-19 14:47:30 PDT
Created attachment 616755 [details] [diff] [review]
patch

This patch implements the behavior that if the user clicks on one plugin, only that plugin and other invisible plugins are played.
Comment 12 Jared Wein [:jaws] (please needinfo? me) 2012-04-19 15:00:10 PDT
Comment on attachment 616755 [details] [diff] [review]
patch

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

::: browser/base/content/browser.js
@@ +7288,5 @@
>      let pluginsPermission = Services.perms.testPermission(browser.currentURI, "plugins");
>      let overlay = doc.getAnonymousElementByAttribute(aPlugin, "class", "mainBox");
>  
> +    if (browser._clickToPlayPluginsActivated ||
> +        (browser._clickToPlaySinglePluginActivated && aIsInvisible)) {

I don't think this is what we want. According to this, if a user activates a Flash plugin on the page, then 10 seconds later an invisible Java plugin is dynamically added to the page, the Java plugin will get activated.
Comment 13 David Keeler [:keeler] (use needinfo?) 2012-04-19 15:22:59 PDT
(In reply to Jared Wein [:jaws] from comment #12)

> I don't think this is what we want. According to this, if a user activates a
> Flash plugin on the page, then 10 seconds later an invisible Java plugin is
> dynamically added to the page, the Java plugin will get activated.

That's handled in bug 746374.
Comment 14 Daniel Cater 2012-04-21 08:51:28 PDT
*** Bug 747638 has been marked as a duplicate of this bug. ***
Comment 15 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-05 14:04:08 PDT
Comment on attachment 616755 [details] [diff] [review]
patch

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

::: browser/base/content/browser.js
@@ +7218,5 @@
> +      let overlay = aContentWindow.document.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> +      let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +      if (!objLoadingContent.activated && plugin != aActivatedPlugin) {
> +        if (this.isTooSmall(plugin, overlay)) objLoadingContent.playPlugin();
> +        else haveUnplayedPlugins = true;

You should change the line of this if-else statement block and add bracket if possible.


A browser should only the plugin that user clicks. A browser *must not* play invisible (hidden) plugins automatically if user clicks a plugin element.
A cracker embeds evil contents that attacks vulnerabilities of plugins. So browser should only play the plugins that user permited obviously. If a web page need to play plugins with invisible (hidden) ones, user permits to play them via doorhanger notification. Therefore a browser has to not play invisible (hidden) plugins automatically. Helper plugin objects can be permited with the function of bug 746374.
Comment 16 David Keeler [:keeler] (use needinfo?) 2012-05-11 14:34:11 PDT
Created attachment 623304 [details] [diff] [review]
patch v2 + test

Invisible plugins will not also played when a visible one is clicked.
Comment 17 Jared Wein [:jaws] (please needinfo? me) 2012-05-11 17:09:19 PDT
Comment on attachment 623304 [details] [diff] [review]
patch v2 + test

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

This looks good so far, but I want to look at it a little longer.
Comment 18 Jared Wein [:jaws] (please needinfo? me) 2012-05-12 18:25:36 PDT
Comment on attachment 623304 [details] [diff] [review]
patch v2 + test

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

Looks good. I'd like to see it again before you check it in.

::: browser/base/content/browser.js
@@ +7223,5 @@
>        notification.remove();
>    },
>  
> +  activateSinglePlugin: function PH_activateSinglePlugin(aContentWindow, aPlugin) {
> +    let browser = gBrowser.getBrowserForDocument(aContentWindow.document);

Can you move this line to be closer to where |browser| is used?

@@ +7232,5 @@
> +    let cwu = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                            .getInterface(Ci.nsIDOMWindowUtils);
> +    let plugins = cwu.plugins;
> +    let haveUnplayedPlugins = false;
> +    for (let plugin of plugins) {

Can you to use Array.some here?

@@ +7235,5 @@
> +    let haveUnplayedPlugins = false;
> +    for (let plugin of plugins) {
> +      let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +      if (plugin != aPlugin && !objLoadingContent.activated) {
> +        haveUnplayedPlugins = true;

Add a |break;| here.

::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
@@ +22,5 @@
>  
>  <!ENTITY missingPlugin                                       "A plugin is needed to display this content.">
>  <!-- LOCALIZATION NOTE (tapToPlayPlugin): Mobile (used for touch interfaces) only has one type of plugin possible. -->
>  <!ENTITY tapToPlayPlugin                                     "Tap here to activate plugin.">
> +<!ENTITY clickToPlayPlugins                                  "Click here to activate plugin.">

Going from plural to singular, we should change the entity name here so localizers can update their text.
Comment 19 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-12 23:49:55 PDT
Comment on attachment 623304 [details] [diff] [review]
patch v2 + test

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

I seem this looks good. Thank you, David!

::: browser/base/content/browser.js
@@ +7315,5 @@
>  
>      let overlay = doc.getAnonymousElementByAttribute(aPlugin, "class", "mainBox");
>      overlay.addEventListener("click", function(aEvent) {
>        if (aEvent.button == 0 && aEvent.isTrusted)
> +        gPluginHandler.activateSinglePlugin(aEvent.target.ownerDocument.defaultView.top, aPlugin);

In some edge cases, this |aPlugin| will be undefined.
If this |aPlugin| is undefined, |overlay| will be undefined too. But in such cases, this part will not be called by changing Bug 752228. (Please look its bug. It describes one of their edge case)

If we can, it is better that gPluginHandler.activateSinglePlugin does not use 2nd parameter |aPlugin|. (You need not accept this point)
Comment 20 Jared Wein [:jaws] (please needinfo? me) 2012-05-13 02:46:47 PDT
what is a scenario where aPlugin is null? the overlay is null when the element is display:none but I'm not familiar with a case where aPlugin can be null.
Comment 21 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-13 05:11:24 PDT
(In reply to Jared Wein [:jaws] from comment #20)
> what is a scenario where aPlugin is null? the overlay is null when the
> element is display:none but I'm not familiar with a case where aPlugin can
> be null.

In the case of Bug 752228, I confirm with Venkman that aPlugin seems like undefined.
I'm sorry if I could not come across to you via the stack trace which I pasted to https://bugzilla.mozilla.org/show_bug.cgi?id=752228#c2
Comment 22 David Keeler [:keeler] (use needinfo?) 2012-05-14 10:33:07 PDT
Created attachment 623719 [details] [diff] [review]
patch v3 + test

Did you literally mean 'Array.some' or '[the array in question].some'?
Also, would a 'break;' be appropriate in that case?
Anyway, I made the changes I thought you meant.
Comment 23 Jared Wein [:jaws] (please needinfo? me) 2012-05-14 11:11:15 PDT
Comment on attachment 623719 [details] [diff] [review]
patch v3 + test

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

Looks good, sorry for the confusing review feedback in the other pass.

r+ with the nit below fixed.

::: browser/base/content/browser.js
@@ +7248,5 @@
> +
> +    let cwu = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                            .getInterface(Ci.nsIDOMWindowUtils);
> +    let plugins = cwu.plugins;
> +    let haveUnplayedPlugins = cwu.plugins.some(function(plugin) {

The |plugins| variable here is unused so we can remove it.
Comment 24 David Keeler [:keeler] (use needinfo?) 2012-05-14 13:11:21 PDT
Created attachment 623791 [details] [diff] [review]
patch v4 + test

Nit fixed. Carrying over r+.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=0d8d7aa1f43d
Comment 25 David Keeler [:keeler] (use needinfo?) 2012-05-15 11:14:27 PDT
Created attachment 624114 [details] [diff] [review]
patch v5 + tests v2

Broke a test; fixed it. Added a new test for something I wasn't testing before (dynamically loading more plugins after one has been clicked).
Comment 26 Jared Wein [:jaws] (please needinfo? me) 2012-05-15 11:35:38 PDT
Comment on attachment 624114 [details] [diff] [review]
patch v5 + tests v2

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

::: browser/base/content/test/browser_bug743421.js
@@ +51,5 @@
>    var plugin = gTestBrowser.contentDocument.getElementsByTagName("embed")[0];
>    var objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
>    ok(!objLoadingContent.activated, "Test 1b, Plugin should not be activated");
>  
> +  popupNotification.mainAction.callback();

Why does this need to be changed?
Comment 27 Jared Wein [:jaws] (please needinfo? me) 2012-05-15 11:37:25 PDT
Comment on attachment 624114 [details] [diff] [review]
patch v5 + tests v2

> --- a/browser/base/content/test/browser_bug743421.js
> +++ b/browser/base/content/test/browser_bug743421.js
> @@ -47,17 +47,17 @@ function test1a() {
>  
>  function test1b() {
>    var popupNotification = PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser);
>    ok(popupNotification, "Test 1b, Should have a click-to-play notification");
>    var plugin = gTestBrowser.contentDocument.getElementsByTagName("embed")[0];
>    var objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
>    ok(!objLoadingContent.activated, "Test 1b, Plugin should not be activated");
>  
> -  EventUtils.synthesizeMouse(plugin, 100, 100, { });
> +  popupNotification.mainAction.callback();
>    setTimeout(test1c, 500);
>  }
>  
>  function test1c() {
>    var popupNotification = PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser);
>    ok(!popupNotification, "Test 1c, Should not have a click-to-play notification");
>    var plugin = gTestBrowser.contentWindow.addPlugin();

This is the context for my previous comment.
Comment 28 David Keeler [:keeler] (use needinfo?) 2012-05-15 11:40:23 PDT
Previously, popupNotification.mainAction.callback() was the same thing as clicking a plugin (i.e. it played every plugin). Now (due to the changes we're making in this bug), clicking one plugin does not play every plugin. This test assumes that an event happens that causes every plugin to play, so instead of clicking one plugin, we need to activate them all through the notification.
Comment 29 Jared Wein [:jaws] (please needinfo? me) 2012-05-15 14:09:44 PDT
Comment on attachment 624114 [details] [diff] [review]
patch v5 + tests v2

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

Oh ok, makes sense.
Comment 30 David Keeler [:keeler] (use needinfo?) 2012-05-16 09:26:00 PDT
Try run for most recent patch: https://tbpl.mozilla.org/?tree=Try&rev=31f065390c1d
Comment 31 Jared Wein [:jaws] (please needinfo? me) 2012-05-16 11:31:10 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e428a3d95bcd
Comment 32 Ryan VanderMeulen [:RyanVM] 2012-05-16 19:57:07 PDT
https://hg.mozilla.org/mozilla-central/rev/e428a3d95bcd
Comment 33 neil@parkwaycc.co.uk 2012-05-18 04:41:09 PDT
So, I was having trouble making the doorhanger go away. Turns out that the site I was testing on uses object/embed fallback, which looks like two plugins from the point of view of the window utils, but you can't actually activate both...
Comment 34 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-05-18 06:47:55 PDT
(In reply to neil@parkwaycc.co.uk from comment #33)
> So, I was having trouble making the doorhanger go away. Turns out that the
> site I was testing on uses object/embed fallback, which looks like two
> plugins from the point of view of the window utils, but you can't actually
> activate both...

I seems your situation should be thought on other bug.
I recommend filing a new bug which describes about your situation, and adding Bug 738698 & this to "Blocks" on it. (If you can, please attach the testcase to reproduce your bug)
Comment 35 Nicolas Barbulesco 2012-11-29 08:30:09 PST
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #0)

> I think that playing plugin should a clicked element only.

> And from the viewpoint of usability, this behavior goes against user's
> expectation which Firefox plays plugins in clicked element only.

Definitely.

(In reply to Lozzy from comment #9)

> - As a security concious user, I am expecting that the item I enable is the
> only one enabled.

Definitely.

Many pages, like Dailymotion video pages, have, in addition of the central video, an auto-refreshing ad that happens to auto-play and to make noise, without caring about the central video already playing !

If a plug-in element, in order to work correctly, needs that some "helper" - or should we say "spy" ? - plug-in elements run too :

1) Well, this is a design problem in the page.
2) However, let's give the user the *additional* possibility of enabling all plug-ins in the page. Or enabling all plug-ins of each type (Flash, for instance) in the page. With a contextual menu item, an address bar notification or whatever.
Comment 36 Paul Silaghi, QA [:pauly] 2012-12-03 07:12:14 PST
http://img255.imageshack.us/img255/9088/ctp.png
nightly 20.0a1 (2012-12-02)
2 questions:
1. why is the 3rd plugin so dark?
2. what's with the "unknown" plugin on the top?
Comment 37 Georg Fritzsche [:gfritzsche] 2012-12-03 07:30:42 PST
(In reply to Paul Silaghi [QA] from comment #36)
> 1. why is the 3rd plugin so dark?
> 2. what's with the "unknown" plugin on the top?

Could you file bugs for those issues? (2) is a regression and not occuring in the current beta/18, (1) might just be a style issue for the binding.
Comment 38 Georg Fritzsche [:gfritzsche] 2012-12-03 07:38:56 PST
(In reply to Georg Fritzsche [:gfritzsche] from comment #37)
> is a regression and not occuring in the current beta/18

Actually it's happening in beta/18 but not in release/17.0.1.
Comment 39 Paul Silaghi, QA [:pauly] 2012-12-04 05:09:34 PST
(In reply to Georg Fritzsche [:gfritzsche] from comment #37)
> (In reply to Paul Silaghi [QA] from comment #36)
> > 1. why is the 3rd plugin so dark?
> > 2. what's with the "unknown" plugin on the top?
> 
> Could you file bugs for those issues? (2) is a regression and not occuring
> in the current beta/18, (1) might just be a style issue for the binding.

bug 818008, bug 818009 filed
Comment 40 Paul Silaghi, QA [:pauly] 2012-12-04 07:26:10 PST
Verified fixed this on nightly 20.0a1 (2012-12-02) considering comment 39.

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