765506 - click-to-play: attach click listener to plugin element instead of overlay

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[bugzilla]

Attachment: Example

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20100101 Firefox/14.0
Build ID: 20120612164001

Steps to reproduce:

I have the new click-to-play function enabled.
When I would like to active the flash Plugin for a flash-object of the Site Sockshare.com and Putlocker.com, I can't do that.


Actual results:

When I try to activate the plugin by clicking on the button, nothing happens. The stent remains, and the video does not play itself. Also update a page does not work.


Expected results:

You should make the (wild card) away, and instead should activate the plugin and start running. For example on YouTube.

Comment 1

[Loic]

Can you provide an URL (video streaming) to test please?

Comment 2

[bugzilla]

http://www.sockshare.com/file/7DFB1ACDA1626148#

Comment 3

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

Looking at the page, this is another instance of bug 741130.

Comment 4

[Loic]

(In reply to David Keeler from comment #3)
> Looking at the page, this is another instance of bug 741130.
> 
> *** This bug has been marked as a duplicate of bug 741130 ***

Are you sure? The bug you linked is about Firefox for Android. Is there something I missed?

Comment 5

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

(In reply to Loic from comment #4)
> Are you sure? The bug you linked is about Firefox for Android. Is there
> something I missed?

Sorry, that was misleading - bug 741130 is indeed for Firefox for Android, but the exact same problem occurs in Firefox for desktop, so I was intending to fix both problems at the same time. A better approach might be to have separate bugs, since the fixes are technically separate.

Comment 6

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

Attachment: patch

Jared - this is the desktop version of bug 741130.

Comment 7

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

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

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

::: browser/base/content/browser-plugins.js
@@ +285,2 @@
>            gPluginHandler.activateSinglePlugin(aEvent.target.ownerDocument.defaultView.top, aPlugin);
> +          aPlugin.removeEventListener("click", this);

aPlugin.removeEventListener("click", listener, true);

::: browser/base/content/test/browser_pluginnotification.js
@@ +631,5 @@
> +  var objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +  ok(!objLoadingContent.activated, "Test 19b, plugin should not be activated");
> +
> +  gTestBrowser.contentWindow.displayPlugin();
> +  var condition = function() doc.getAnonymousElementByAttribute(plugin, "class", "mainBox") != null;

If it's going from display:none to display:block, isn't the mainBox always null?

::: browser/base/content/test/plugin_hidden_to_visible.html
@@ +7,5 @@
> +<script>
> +function addPlugin() {
> +  var div = document.createElement("div");
> +  div.id = "container";
> +  div.style.display = "none";

I think the test should be having the object with display:none, not the container, right?

Comment 8

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

(In reply to Jared Wein [:jaws] from comment #7)
> If it's going from display:none to display:block, isn't the mainBox always
> null?
...
> I think the test should be having the object with display:none, not the
> container, right?

From the behavior I'm seeing, the mainBox is only null if the element the object is in is display:none. If the object itself is display:none, the overlay does exist - it's just not visible.

Comment 9

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

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

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

Ok, I'm fine with the changes then, but make sure to update the removeEventListener line.

::: browser/base/content/browser-plugins.js
@@ +276,5 @@
>        return;
>      }
>  
> +    let listener = {
> +      handleEvent: function(aEvent) {

Since there is only one event to handle, can you change this to a plain-old function then just reference the function in the add/removeEventListener calls?

Comment 10

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

Attachment: patch v2

Thanks for the speedy review - carrying over r+.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=5d5b136a5754

Comment 11

[bugzilla]

15 Beta with Firefox, it still does not work.
How can I use the patch that you have made; Sorry I'm not a developer and I understand little of it.

Comment 12

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

The patch passed all the tests on our tryserver, so it should be checked in to mozilla-central within a few days and then appear on Firefox Nightly the day after.

As to the Version attribute of this bug, please leave it at Firefox 14 since that is the first version where this bug occurred. When this bug gets fixed it will have the "Target Milestone" updated to reflect which version that it initially got fixed in.

If the fix is expedited to our Beta or Aurora branch, then the status-firefox14 and status-firefox15 flags will be updated.

Comment 13

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

Sorry, removing checkin-needed as per https://bugzilla.mozilla.org/show_bug.cgi?id=741130#c22

Comment 14

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

This was fixed by bug 741130.