Last Comment Bug 765506 - click-to-play: attach click listener to plugin element instead of overlay
: click-to-play: attach click listener to plugin element instead of overlay
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: 14 Branch
: x86_64 Mac OS X
: -- normal with 1 vote (vote)
: ---
Assigned To: David Keeler [:keeler] (use needinfo?)
:
:
Mentors:
Depends on: 741130
Blocks: click-to-play 774937
  Show dependency treegraph
 
Reported: 2012-06-16 11:56 PDT by bugzilla
Modified: 2012-08-13 10:53 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
affected


Attachments
Example (513.38 KB, image/jpeg)
2012-06-16 11:56 PDT, bugzilla
no flags Details
patch (7.85 KB, patch)
2012-07-18 10:22 PDT, David Keeler [:keeler] (use needinfo?)
jaws: review+
Details | Diff | Splinter Review
patch v2 (7.92 KB, patch)
2012-07-18 12:25 PDT, David Keeler [:keeler] (use needinfo?)
dkeeler: review+
Details | Diff | Splinter Review

Description bugzilla 2012-06-16 11:56:09 PDT
Created attachment 633839 [details]
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 2012-06-16 14:09:54 PDT
Can you provide an URL (video streaming) to test please?
Comment 3 David Keeler [:keeler] (use needinfo?) 2012-07-17 15:24:59 PDT
Looking at the page, this is another instance of bug 741130.

*** This bug has been marked as a duplicate of bug 741130 ***
Comment 4 Loic 2012-07-17 15:35:37 PDT
(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 David Keeler [:keeler] (use needinfo?) 2012-07-17 15:43:19 PDT
(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 David Keeler [:keeler] (use needinfo?) 2012-07-18 10:22:55 PDT
Created attachment 643436 [details] [diff] [review]
patch

Jared - this is the desktop version of bug 741130.
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-07-18 11:37:00 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-07-18 11:50:45 PDT
(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) 2012-07-18 12:08:33 PDT
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 David Keeler [:keeler] (use needinfo?) 2012-07-18 12:25:15 PDT
Created attachment 643503 [details] [diff] [review]
patch v2

Thanks for the speedy review - carrying over r+.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=5d5b136a5754
Comment 11 bugzilla 2012-07-20 05:12:27 PDT
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) 2012-07-20 08:45:33 PDT
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) 2012-07-23 11:14:01 PDT
Sorry, removing checkin-needed as per https://bugzilla.mozilla.org/show_bug.cgi?id=741130#c22
Comment 14 David Keeler [:keeler] (use needinfo?) 2012-08-13 10:53:00 PDT
This was fixed by bug 741130.

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