Tap-to-play plugin click listener sometimes doesn't get triggered

RESOLVED FIXED in Firefox 14

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 14
ARM
Android
Points:
---

Firefox Tracking Flags

(blocking-fennec1.0 +)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
The most problematic test page I've found is: http://people.mozilla.org/~mwargers/tests/plugins/flash/flashembed_dynamicrepeating.html

I added logging around our Gesture:SingleTap handler, trying to figure out what's different between cases where the click listener does/doesn't get fired, and I'm having a tough time tracking down what could be going wrong, but I'll keep looking into it.
(Assignee)

Comment 1

6 years ago
I think we should consider this as a blocker, since it seemed to affect multiple sites mentioned in the comments in bug 741130.
blocking-fennec1.0: --- → ?
(Assignee)

Comment 2

6 years ago
Hm, is this a dupe of bug 740005 (one of the many dupes of bug 732016)?
blocking-fennec1.0: ? → +
(Assignee)

Updated

6 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 3

6 years ago
(In reply to Margaret Leibovic [:margaret] from comment #2)
> Hm, is this a dupe of bug 740005 (one of the many dupes of bug 732016)?

No, it's not.

I found that the problem here is once again that sometimes we aren't actually adding the event listener to the overlays. I made a reduced testcase that makes this clear:

http://people.mozilla.org/~mleibovic/test/dynamic_embed.html

This works on desktop, so I'm wondering what could be different. Maybe we're somehow slower to create the overlay on mobile, so that sometimes it doesn't exist yet when the PluginClickToPlay event handler code runs. That could explain the fact that Martijn's testcase sometimes worked for me (this intermittent-ness is what led me to think it was a tap problem).

In any case, this is pretty tricky. I'm considering exploring adding the click listener to the plugin itself again, since that would fix bug 741130 as well, but maybe there's a more elegant solution?
(Assignee)

Comment 4

6 years ago
(In reply to Margaret Leibovic [:margaret] from comment #3)

> I'm considering exploring adding the
> click listener to the plugin itself again, since that would fix bug 741130
> as well, but maybe there's a more elegant solution?

Jared convinced me that it's a bad idea to add our own event listeners to content. We discussed use a click handler in pluginProblem.xml to send an event to browser.js, but creating a custom event doesn't seem to be working, and Dolske suggested we stay away from adding more to the XBL anyway.

Maybe we can fire an event from platform code when the overlay is created. Does anyone know who a good person would be to talk to about that?
(Assignee)

Comment 5

6 years ago
Created attachment 615961 [details] [diff] [review]
patch

Sigh, desktop already solved this problem:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7126

Doing the same thing for fennec fixed this testcase. This doesn't fix the display: none; bug, but this is a hell of a lot simpler than trying to mess with XBL, and it fixes this dynamically added embed issue.
Attachment #615961 - Flags: review?(mark.finkle)
Comment on attachment 615961 [details] [diff] [review]
patch

Yay! for XBL tricks
Attachment #615961 - Flags: review?(mark.finkle) → review+
You're OK to land this. It's very low risk.
(Assignee)

Comment 8

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/aec492e095b8
Target Milestone: --- → Firefox 14

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/aec492e095b8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(In reply to Mark Finkle (:mfinkle) from comment #6)

> Yay! for XBL tricks

OTOH, if we had been sharing more code between desktop and mobile we might have avoided this in the first place! :) We should really dig ourselves out of that hole someday...
You need to log in before you can comment on or make changes to this bug.