787619 - Unable to use click-to-play when the plugin content is inside an <a> tag

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Henrik Gemal]

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]

Is this a regression? Do you have any addons installed/does this show up in safe mode?

Comment 2

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

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]]

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 5

[Georg Fritzsche [:gfritzsche]]

Attachment: Reduced test-case

Comment 6

[Georg Fritzsche [:gfritzsche]]

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

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

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]]

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]]

(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]]

Attachment: 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]]

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]]

Attachment: Test

Comment 13

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

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

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

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]]

Attachment: Test for clicks being prevented from triggering other handlers

Adressing Davids point.

Comment 16

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

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]]

Attachment: Prevent C2P click events from triggering other handlers

Comment 18

[Georg Fritzsche [:gfritzsche]]

Attachment: Test for clicks being prevented from triggering other handlers

Adressed review comments.

Comment 19

[Georg Fritzsche [:gfritzsche]]

https://tbpl.mozilla.org/?tree=Try&rev=5183b6d93280

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d23e8d2f87a
https://hg.mozilla.org/integration/mozilla-inbound/rev/404557eb1786

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d23e8d2f87a
https://hg.mozilla.org/mozilla-central/rev/404557eb1786

Comment 22

[Georg Fritzsche [:gfritzsche]]

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]]

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]]

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]]

Comment on attachment 673544 [details] [diff] [review]
Test for clicks being prevented from triggering other handlers

[Approval Request Comment]
See comment 22.

Comment 27

[Georg Fritzsche [:gfritzsche]]

Both patches apply cleanly on Beta and Aurora.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/496b5931379e
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab6323dea358

https://hg.mozilla.org/releases/mozilla-beta/rev/6072a4fd599d
https://hg.mozilla.org/releases/mozilla-beta/rev/3c4b9ffec3d4

Comment 29

[Paul Silaghi, QA [:pauly]]

Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0b4

Comment 30

[Paul Silaghi, QA [:pauly]]

Verified fixed on FF 19b3 on Win 7 and Mac OS X 10.8.2