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

VERIFIED FIXED in Firefox 17

Status

()

Core
Plug-ins
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Henrik Gemal, Assigned: gfritzsche)

Tracking

17 Branch
mozilla19
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ fixed, firefox18+ verified, firefox19 verified)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
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.

Updated

5 years ago
Duplicate of this bug: 790241

Updated

5 years ago
Summary: unable to use click-to-play → Unable to use click-to-play when the plugin content is inside an <a> tag
(Assignee)

Comment 5

5 years ago
Created attachment 671781 [details]
Reduced test-case
(Assignee)

Comment 6

5 years ago
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
Last Resolved: 5 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.
(Assignee)

Comment 9

5 years ago
(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 → ---
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
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)
(Assignee)

Comment 12

5 years ago
Created attachment 672290 [details] [diff] [review]
Test
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?
(Assignee)

Comment 15

5 years ago
Created attachment 672783 [details] [diff] [review]
Test for clicks being prevented from triggering other handlers

Adressing Davids point.
Attachment #672290 - Attachment is obsolete: true
Attachment #672783 - Flags: review?(jaws)
(Assignee)

Updated

5 years ago
Attachment #672218 - Flags: review?(jaws)
Attachment #672218 - Flags: review?(jaws) → review+
OS: Windows 7 → All
Hardware: x86 → All
Version: Trunk → 17 Branch
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
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+

Updated

5 years ago
tracking-firefox17: ? → +
tracking-firefox18: ? → +
(Assignee)

Comment 17

5 years ago
Created attachment 673543 [details] [diff] [review]
Prevent C2P click events from triggering other handlers
Attachment #672218 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
Created attachment 673544 [details] [diff] [review]
Test for clicks being prevented from triggering other handlers

Adressed review comments.
Attachment #672783 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=5183b6d93280
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d23e8d2f87a
https://hg.mozilla.org/integration/mozilla-inbound/rev/404557eb1786
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3d23e8d2f87a
https://hg.mozilla.org/mozilla-central/rev/404557eb1786
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
status-firefox17: --- → affected
status-firefox18: --- → affected
(Assignee)

Comment 22

5 years ago
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?
(Assignee)

Comment 23

5 years ago
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?
(Assignee)

Comment 24

5 years ago
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?
(Assignee)

Comment 25

5 years ago
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+
(Assignee)

Updated

5 years ago
Duplicate of this bug: 798003
(Assignee)

Comment 27

5 years ago
Both patches apply cleanly on Beta and Aurora.
Keywords: checkin-needed
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
status-firefox17: affected → fixed
status-firefox18: affected → fixed
status-firefox19: --- → fixed
Keywords: checkin-needed
Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0b4
Blocks: 738698
status-firefox18: fixed → verified
Verified fixed on FF 19b3 on Win 7 and Mac OS X 10.8.2
Status: RESOLVED → VERIFIED
status-firefox19: fixed → verified
You need to log in before you can comment on or make changes to this bug.