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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

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.

Comment 1

[:Margaret Leibovic]

I think we should consider this as a blocker, since it seemed to affect multiple sites mentioned in the comments in bug 741130.

Comment 2

[:Margaret Leibovic]

Hm, is this a dupe of bug 740005 (one of the many dupes of bug 732016)?

Comment 3

[:Margaret Leibovic]

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

Comment 4

[:Margaret Leibovic]

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

Comment 5

[:Margaret Leibovic]

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

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 615961 [details] [diff] [review]
patch

Yay! for XBL tricks

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

You're OK to land this. It's very low risk.

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aec492e095b8

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/aec492e095b8

Comment 10

[Justin Dolske [:Dolske]]

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