Last Comment Bug 787619 - Unable to use click-to-play when the plugin content is inside an <a> tag
: Unable to use click-to-play when the plugin content is inside an <a> tag
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: mozilla19
Assigned To: Georg Fritzsche [:gfritzsche]
:
Mentors:
: 790241 798003 (view as bug list)
Depends on:
Blocks: click-to-play
  Show dependency treegraph
 
Reported: 2012-08-31 22:33 PDT by Henrik Gemal
Modified: 2013-01-29 04:28 PST (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
verified
verified


Attachments
Reduced test-case (638 bytes, text/html)
2012-10-16 03:54 PDT, Georg Fritzsche [:gfritzsche]
no flags Details
Prevent C2P click events from triggering other handlers (1.34 KB, patch)
2012-10-17 02:15 PDT, Georg Fritzsche [:gfritzsche]
jaws: review+
dkeeler: feedback+
Details | Diff | Review
Test (3.24 KB, patch)
2012-10-17 07:11 PDT, Georg Fritzsche [:gfritzsche]
no flags Details | Diff | Review
Test for clicks being prevented from triggering other handlers (2.90 KB, patch)
2012-10-18 07:28 PDT, Georg Fritzsche [:gfritzsche]
jaws: review+
Details | Diff | Review
Prevent C2P click events from triggering other handlers (1.35 KB, patch)
2012-10-20 03:51 PDT, Georg Fritzsche [:gfritzsche]
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review
Test for clicks being prevented from triggering other handlers (3.09 KB, patch)
2012-10-20 03:51 PDT, Georg Fritzsche [:gfritzsche]
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Henrik Gemal 2012-08-31 22:33:43 PDT
http://www.top-windows-tutorials.com/encryption-tutorial.html

I click on the play icon and the "click-to-play" icon is shown. Now I click on the icon and then the "click-to-play" icon is just shown again.

using latest nightly
Comment 1 Benjamin Smedberg [:bsmedberg] 2012-09-05 08:26:31 PDT
Is this a regression? Do you have any addons installed/does this show up in safe mode?
Comment 2 David Keeler [:keeler] (use needinfo?) 2012-09-07 17:13:03 PDT
Looks like the plugin content is inside an <a> tag, so when you click it, it navigates (and unloads the plugin?). I bet johns would know if this is our bug or if it's something they need to change about their site. In the meantime, the popup notification in the urlbar looks like it works.
Comment 3 John Schoenick [:johns] 2012-09-07 17:29:01 PDT
Yeah, the anchor tag is intercepting onclick (via the 'flowplayer' js) and preventing the clicks from getting to the plugin frame, just re-creating the object ever time you click.

However, when the plugin is active, it is not obstructed from click events by the anchor tag, so it's possible we should be intercepting click events more aggressively in CTP frames to ensure they receive clicks whenever their respective plugin would.
Comment 4 Loic 2012-09-11 09:32:10 PDT
*** Bug 790241 has been marked as a duplicate of this bug. ***
Comment 5 Georg Fritzsche [:gfritzsche] 2012-10-16 03:54:49 PDT
Created attachment 671781 [details]
Reduced test-case
Comment 6 Georg Fritzsche [:gfritzsche] 2012-10-16 04:00:05 PDT
The clicks are actually passed through fine, but the flowplayer is reloading the plugin element on every click until flash is actually loaded.

I don't see how we could properly work around this without special-casing for exactly such cases.

David, do you think we should look into a work-around?
Comment 7 David Keeler [:keeler] (use needinfo?) 2012-10-16 10:02:42 PDT
Well, this seems to be another evangelism bug - the best case would be for flowplayer to modify their implementation. I don't think it's a good use of our time to figure out a way to make this one case work.
Comment 8 John Schoenick [:johns] 2012-10-16 10:27:32 PDT
How does this not occur when the plugin *is* loaded? Does flowplayer remove its onclick handling, or do the way the events bubble to the plugin change? FWIW, this does work in chrome's CTP implementation.
Comment 9 Georg Fritzsche [:gfritzsche] 2012-10-17 02:09:29 PDT
(In reply to John Schoenick [:johns] from comment #8)
> How does this not occur when the plugin *is* loaded? Does flowplayer remove
> its onclick handling, or do the way the events bubble to the plugin change?

Sorry, i completely missed your point before :( 
Both the flowplayer and the C2P handlers are triggered and the C2P code apparently doesn't stop the event from propagating.
Comment 10 Georg Fritzsche [:gfritzsche] 2012-10-17 02:15:30 PDT
Created attachment 672218 [details] [diff] [review]
Prevent C2P click events from triggering other handlers

This fixes the issue for me on both the reduced and the original test-case.
Comment 11 Georg Fritzsche [:gfritzsche] 2012-10-17 02:23:39 PDT
Comment on attachment 672218 [details] [diff] [review]
Prevent C2P click events from triggering other handlers

David, as you are familiar with the C2P frontend - do you see any problems with this?
Comment 12 Georg Fritzsche [:gfritzsche] 2012-10-17 07:11:23 PDT
Created attachment 672290 [details] [diff] [review]
Test
Comment 13 David Keeler [:keeler] (use needinfo?) 2012-10-17 13:40:36 PDT
Comment on attachment 672218 [details] [diff] [review]
Prevent C2P click events from triggering other handlers

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

Awesome - I'm glad this is something we can actually fix.
Comment 14 David Keeler [:keeler] (use needinfo?) 2012-10-17 13:43:31 PDT
Comment on attachment 672290 [details] [diff] [review]
Test

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

::: browser/base/content/test/test_bug787619.html
@@ +23,5 @@
> +                                      getInterface(Ci.nsIDOMWindowUtils);
> +
> +  var wrapperClickCount = 0;
> +
> +  function waitForCondition(condition, nextTest, errorMsg) {

This should already be in head.js - is there a reason you can't use that one?
Comment 15 Georg Fritzsche [:gfritzsche] 2012-10-18 07:28:18 PDT
Created attachment 672783 [details] [diff] [review]
Test for clicks being prevented from triggering other handlers

Adressing Davids point.
Comment 16 Jared Wein [:jaws] (please needinfo? me) 2012-10-18 16:05:34 PDT
Comment on attachment 672783 [details] [diff] [review]
Test for clicks being prevented from triggering other handlers

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

r=me with the following issues addressed.

::: browser/base/content/test/Makefile.in
@@ +39,5 @@
>  		gZipOfflineChild.html^headers^ \
>  		gZipOfflineChild.cacheManifest \
>  		gZipOfflineChild.cacheManifest^headers^ \
> +		test_bug787619.html \
> +		head.js \

I'd prefer if head.js was at the top of this list so it is easier to see that it is included already.

::: browser/base/content/test/test_bug787619.html
@@ +19,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  const Ci = Components.interfaces;
> +  const utils = window.QueryInterface(Ci.nsIInterfaceRequestor).
> +                                      getInterface(Ci.nsIDOMWindowUtils);

nit: Please name this variable either DOMWindowUtils or dwu.

@@ +26,5 @@
> +
> +  function test1() {
> +    let plugin = document.getElementById('plugin');
> +    ok(plugin, 'got plugin element');
> +    var objLC = plugin.QueryInterface(Ci.nsIObjectLoadingContent);

s/var/let/ here and other places in this test.

@@ +29,5 @@
> +    ok(plugin, 'got plugin element');
> +    var objLC = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +    ok(!objLC.activated, 'plugin should not be activated');
> +
> +    synthesizeMouseAtCenter(plugin, {}, window);

Is |window| necessary here? I think that third argument is only necessary if the event should be targeted at a separate window.

@@ +31,5 @@
> +    ok(!objLC.activated, 'plugin should not be activated');
> +
> +    synthesizeMouseAtCenter(plugin, {}, window);
> +    let condition = function() { return objLC.activated };
> +    waitForCondition(condition, test2, 'waited too long for plugin to activate');

waitForCondition(function () objLC.activated, test2, 'waited too long for plugin to activate');
Comment 17 Georg Fritzsche [:gfritzsche] 2012-10-20 03:51:14 PDT
Created attachment 673543 [details] [diff] [review]
Prevent C2P click events from triggering other handlers
Comment 18 Georg Fritzsche [:gfritzsche] 2012-10-20 03:51:57 PDT
Created attachment 673544 [details] [diff] [review]
Test for clicks being prevented from triggering other handlers

Adressed review comments.
Comment 19 Georg Fritzsche [:gfritzsche] 2012-10-20 03:52:17 PDT
https://tbpl.mozilla.org/?tree=Try&rev=5183b6d93280
Comment 22 Georg Fritzsche [:gfritzsche] 2012-10-23 10:07:32 PDT
Comment on attachment 673543 [details] [diff] [review]
Prevent C2P click events from triggering other handlers

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Click-to-play for plugins.
User impact if declined: sites using flowplayers delayed loading won't work when Flash needs C2P activation.
Testing completed (on m-c, etc.): No negative reports after landing on m-c, but possible duplicate fixes (bug 798003).
Risk to taking this patch (and alternatives if risky): low risk as it only prevents C2P activation clicks from triggering other event handlers.
String or UUID changes made by this patch: none.
Comment 23 Georg Fritzsche [:gfritzsche] 2012-10-23 10:09:05 PDT
Comment on attachment 673544 [details] [diff] [review]
Test for clicks being prevented from triggering other handlers

[Approval Request Comment]
Same as the previous comment - this is the accompanying test.
Comment 24 Georg Fritzsche [:gfritzsche] 2012-10-23 10:10:38 PDT
Comment on attachment 673543 [details] [diff] [review]
Prevent C2P click events from triggering other handlers

[Approval Request Comment]
See comment 22.
Comment 25 Georg Fritzsche [:gfritzsche] 2012-10-23 10:11:19 PDT
Comment on attachment 673544 [details] [diff] [review]
Test for clicks being prevented from triggering other handlers

[Approval Request Comment]
See comment 22.
Comment 26 Georg Fritzsche [:gfritzsche] 2012-10-24 08:31:03 PDT
*** Bug 798003 has been marked as a duplicate of this bug. ***
Comment 27 Georg Fritzsche [:gfritzsche] 2012-10-24 08:32:13 PDT
Both patches apply cleanly on Beta and Aurora.
Comment 29 Paul Silaghi, QA [:pauly] 2012-12-19 05:40:22 PST
Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0b4
Comment 30 Paul Silaghi, QA [:pauly] 2013-01-29 04:28:41 PST
Verified fixed on FF 19b3 on Win 7 and Mac OS X 10.8.2

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