Closed Bug 787619 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: Plug-ins, defect)

17 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox17 + fixed
firefox18 + verified
firefox19 --- verified

People

(Reporter: bugzilla, Assigned: gfritzsche)

References

Details

Attachments

(3 files, 3 obsolete files)

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
Is this a regression? Do you have any addons installed/does this show up in safe mode?
Assignee: nobody → georg.fritzsche
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.
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.
Duplicate of this bug: 790241
Summary: unable to use click-to-play → Unable to use click-to-play when the plugin content is inside an <a> tag
Attached file Reduced test-case
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?
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
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.
(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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
This fixes the issue for me on both the reduced and the original test-case.
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?
Attachment #672218 - Flags: feedback?(dkeeler)
Attached patch Test (obsolete) — Splinter Review
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.
Attachment #672218 - Flags: feedback?(dkeeler) → feedback+
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?
Adressing Davids point.
Attachment #672290 - Attachment is obsolete: true
Attachment #672783 - Flags: review?(jaws)
Attachment #672218 - Flags: review?(jaws)
Attachment #672218 - Flags: review?(jaws) → review+
OS: Windows 7 → All
Hardware: x86 → All
Version: Trunk → 17 Branch
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');
Attachment #672783 - Flags: review?(jaws) → review+
Adressed review comments.
Attachment #672783 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3d23e8d2f87a
https://hg.mozilla.org/mozilla-central/rev/404557eb1786
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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.
Attachment #673543 - Flags: approval-mozilla-aurora?
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.
Attachment #673544 - Flags: approval-mozilla-aurora?
Comment on attachment 673543 [details] [diff] [review]
Prevent C2P click events from triggering other handlers

[Approval Request Comment]
See comment 22.
Attachment #673543 - Flags: approval-mozilla-beta?
Comment on attachment 673544 [details] [diff] [review]
Test for clicks being prevented from triggering other handlers

[Approval Request Comment]
See comment 22.
Attachment #673544 - Flags: approval-mozilla-beta?
Attachment #673543 - Flags: approval-mozilla-beta?
Attachment #673543 - Flags: approval-mozilla-beta+
Attachment #673543 - Flags: approval-mozilla-aurora?
Attachment #673543 - Flags: approval-mozilla-aurora+
Attachment #673544 - Flags: approval-mozilla-beta?
Attachment #673544 - Flags: approval-mozilla-beta+
Attachment #673544 - Flags: approval-mozilla-aurora?
Attachment #673544 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 798003
Both patches apply cleanly on Beta and Aurora.
Keywords: checkin-needed
Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0b4
Verified fixed on FF 19b3 on Win 7 and Mac OS X 10.8.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.