741130 - Tapping Plugin placeholder does not work in this case after embed going from display: none to display: block

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

You need to have the "Tap to Play" setting enabled for plugins for this bug.

See url testcase, steps to reproduce:
- Visit testcase, ignore the plugin doorhanger by tapping outside of it
- Tap on the 'make flash embeds visible' button
- Tap on the 'Tap here to activate plugin' placeholder plugin

Expected result:
- Flash plugins become visible, plugin placeholder disappears

Actual result:
- Nothing happens.

Tested in today's trunk Fennec Native build on the Samsung Galaxy SII.

Comment 1

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

I think this is the same as bug 741128.

Comment 2

[Martijn Wargers (dead)]

I'm seeing this bug on real sites, where flash ads are dynamically made visible.

Comment 4

[:Margaret Leibovic]

Reopening, since this isn't actually a dupe. The problem here is that the "tap to play" overlay is null when the PluginClickToPlay event fires for plugin elements set to display: none;, so the click handler never gets attached.

In bug 741128, I discussed the idea of attaching the click handler to the plugin itself, as opposed to the overlay, but there may be a cleaner solution.

Comment 5

[:Margaret Leibovic]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #2)
> I'm seeing this bug on real sites, where flash ads are dynamically made
> visible.

Martijn, do you know if these sites were using display:none; and display:block; to dynamically show the elements? Also, do you remember which sites were having this problem?

In bug 741128 comment 11, Jared mentioned that the patch in that bug may fix the problems you were seeing in this bug.

Comment 6

[Martijn Wargers (dead)]

I thought I saw code like that at the http://www.volkskrant.nl site, but I'm not sure anymore.
I just noticed that at various (dutch) newspaper sites, I couldn't tap on the plugin placeholder to activate Flash.
I'm noticing it now at: http://www.bndestem.nl/nieuws/algemeen/binnenland/10798541/Politie-houdt-scholieren-aan-om-dreigtweets.ece
on my Samsung Galaxy Nexus

I have a copy of that page here: http://people.mozilla.org/~mwargers/tests/unminimized/Politie-massaal-op-school-na-dreigtweet.ece.htm
I suspect this line of code on that page: jQuery('#'+this+'_holder').css({ "height": height + "px" }).show();
 is causing this bug basically.
Although, the display: none display: block thing is not targetted at the embed directly, in this case (I suspect more of an iframe targeting another iframe targeting the embed)..

Comment 7

[:Margaret Leibovic]

I think bug 741590 fixed the problem in that testcase, since it's now working for me.

If we can't find a real site that's broken because of this display:none; problem, I don't think this bug needs to block anymore, but we can keep it open in case we find a real site at some point.

Comment 8

[Martijn Wargers (dead)]

I can't tap on the top plugin placeholder on this page: http://www.ad.nl/ad/nl/10350/Vertrek-Hero-Brinkman/article/detail/3237814/2012/04/08/Brinkman-Ook-andere-PVV-ers-gaan-zich-los-worstelen.dhtml
Not sure why not, it might not have anything to do with this bug, the code to generate this ad at the top looks like an embed in an object that was generated with .innerHTML, afaict.

Comment 9

[Martijn Wargers (dead)]

I guess the problem mentioned in comment 8 is more or less the issue that dynamic embeds seem to be very problematic to activate by tapping in general.
With this testcase, I have great difficulty tapping on the plugin placeholders to activate them: http://people.mozilla.org/~mwargers/tests/plugins/flash/flashembed_dynamicrepeating.html

Comment 10

[:Margaret Leibovic]

I think this is a problem with the tap events, not the click to play code. I added some logging, and on that test page we are getting PluginClickToPlay events and properly attaching a click listener. I've found that I'm able to activate the plugins by tapping on them, although I agree with you that sometimes my tap isn't registered properly.

Comment 11

[Tony Chung [:tchung]]

bug 741130 related?

Comment 12

[:Margaret Leibovic]

(In reply to Tony Chung [:tchung] from comment #11)
> bug 741130 related?

... that's this bug. I think you made a copy/paste mistake :)

Comment 13

[:Margaret Leibovic]

I filed bug 745016 about the tap issues.

This bug is about click listeners not getting added when plugins are made dynamically visible by changing their style rules from display:none; to display:block;. Given that the problematic real-world testcases in this bug seem to be suffering from  745016, I don't think that this bug needs to block. It seems that most of the real-world dynamic plugin testcases were fixed by bug 741128.

(For the record, if it takes a few tries tapping on a plugin for it to play, that's bug 745016 - plugins suffering from this bug will never be activated)

Comment 14

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

Johnny or Olli, would setting an event listener directly on a plugin object (object, embed, applet) cause any foreseeable issues with web content? Maybe it would break when cloning nodes?

I want to make sure that we would be safe doing so, since that route seems to be the only approach that we could use here.

We can't set event handlers directly in the XBL using <handler> because that script would run with content privileges and we require chrome privileges for activating plugins.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jared Wein [:jaws] from comment #14)
> Johnny or Olli, would setting an event listener directly on a plugin object
> (object, embed, applet) cause any foreseeable issues with web content?
Make sure you add the listener to system event group. Otherwise web page can just 
call stopPropagation() and the listener won't be called

> Maybe
> it would break when cloning nodes?
Not sure what you mean here.

Comment 16

[:Margaret Leibovic]

David, would you be interested in taking this bug, since it's also an issue on desktop?

Comment 17

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

Sure - I can work on it.

Comment 19

[alex_mayorga]

Is this video not playing along with "click to play" http://austin.ynn.com/content/top_stories/285818/international-scouts-cooperate-to-clean-up-camp-mabry because of this bug or is it a different bug?

Comment 20

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

(In reply to alex_mayorga from comment #19)
> Is this video not playing along with "click to play"
> http://austin.ynn.com/content/top_stories/285818/international-scouts-
> cooperate-to-clean-up-camp-mabry because of this bug or is it a different
> bug?

Looks more like bug 752516 - the click-to-play overlay isn't receiving the click because there's something over it.

Comment 21

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

Attachment: patch

Here's a fix, but I can't seem to figure out where the click-to-play tests are for android. Is there some equivalent of the mochitest-browser-chrome tests in browser/base/content/test?

Comment 22

[:Margaret Leibovic]

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

Thanks for the patch.

I'm afraid we're going to run into problems with this patch when there's more than one plugin object on the page. This patch just removes the click listener from the plugin that was actually clicked, so if the user clicks on any other plugin on the page after it's been activated by the call to playAllPlugins, we're still going to be executing this click listener callback function. I feel like we could get around this by removing the listener in PluginHelper.playPlugin, but that would also involve making the callback non-anonymous. Perhaps we can make it a function in PluginHelper. I'd also like to verify that adding listeners to a plugin objects in content won't cause leaks, since we may never end up removing them if we never activate them.

I'd also like to hear what Jared has to say about this patch, since the same problem currently exists on desktop.

(In reply to David Keeler from comment #21)
> Here's a fix, but I can't seem to figure out where the click-to-play tests
> are for android. Is there some equivalent of the mochitest-browser-chrome
> tests in browser/base/content/test?

Unfortunately, we don't have tests for this for android right now, mainly because we don't have a suitable test harness like browser-chrome tests for fennec native.

Comment 23

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

Could we iterate through the plugins that are in the content window utils array to remove the event listeners?

Comment 24

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

Thanks for having a look at this.

(In reply to Margaret Leibovic [:margaret] from comment #22)
> I'm afraid we're going to run into problems with this patch when there's
> more than one plugin object on the page. This patch just removes the click
> listener from the plugin that was actually clicked, so if the user clicks on
> any other plugin on the page after it's been activated by the call to
> playAllPlugins, we're still going to be executing this click listener
> callback function.

That reminds me - is there a reason for activating all plugins when a user taps one in mobile? For desktop, we moved to only activating the one interacted with in bug 742753.

> I'd also like to verify that adding listeners to a plugin objects in content
> won't cause leaks, since we may never end up removing them if we never
> activate them.

I would also like to know the answer to this. A related question: when we added the listener to the overlays, would those not leak if we never activated the plugins they corresponded to? If not, what's the difference? (is this a chrome/content thing?)

Comment 25

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

Upon thinking about this, I don't think this will leak. When the page/tab is navigated or closed, the element will be removed and so will its listeners. So, this all should get properly garbage collected.

In terms of calling a listener again when the plugins have already been activated, I don't think there's any harm here. The plugin won't be restarted because the code won't call objLoadingContent.playPlugin() if it has already been activated. It's extra work, but as long as the listener is idempotent, nothing bad happens.

Comment 26

[:Margaret Leibovic]

(In reply to David Keeler from comment #24)
> Thanks for having a look at this.
> 
> (In reply to Margaret Leibovic [:margaret] from comment #22)
> > I'm afraid we're going to run into problems with this patch when there's
> > more than one plugin object on the page. This patch just removes the click
> > listener from the plugin that was actually clicked, so if the user clicks on
> > any other plugin on the page after it's been activated by the call to
> > playAllPlugins, we're still going to be executing this click listener
> > callback function.
> 
> That reminds me - is there a reason for activating all plugins when a user
> taps one in mobile? For desktop, we moved to only activating the one
> interacted with in bug 742753.

We don't have a way to show a persistent icon in the toolbar like we do on desktop, so if there are any invisible "helper swf" objects that need to be activated for the visible plugin to work, the user wouldn't have a way to do that, so we decided to just play them all.

(In reply to David Keeler from comment #25)
> Upon thinking about this, I don't think this will leak. When the page/tab is
> navigated or closed, the element will be removed and so will its listeners.
> So, this all should get properly garbage collected.

Yeah, I think you're right.

> In terms of calling a listener again when the plugins have already been
> activated, I don't think there's any harm here. The plugin won't be
> restarted because the code won't call objLoadingContent.playPlugin() if it
> has already been activated. It's extra work, but as long as the listener is
> idempotent, nothing bad happens.

The listener calls e.preventDefault(), which we wouldn't want. Maybe have a check at the beginning of the listener to see if this plugin has already been activated, and bail is so? Actually, we're already storing a clickToPlayPluginsActivated flag on the tab, so you can just check that at the beginning of the callback function.

Comment 27

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

Attachment: patch v2

This is a much better approach - this patch enables XBL bindings to be attached to html:object, html:embed, and html:applet nodes when accessing them in JS, which is exactly what we need.

Comment 28

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

Comment on attachment 648021 [details] [diff] [review]
patch v2

Forgot to add: I tested with flashblock/adblock and there didn't seem to be any negative consequences (other than that you have to click twice to activate anything with flashblock, which is to be expected).

Comment 29

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

Comment on attachment 648021 [details] [diff] [review]
patch v2

Review of attachment 648021 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the test changes, Splinter now shows trailing whitespace so maybe you could clean that up before landing.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 648021 [details] [diff] [review]
patch v2

r=me

Comment 31

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

Attachment: patch v2 rebased

Had to rebase. Carrying over r+  from bz and jaws.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=8bedebfbe986

Comment 32

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

Try looks good. Marking checkin-needed.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

(In reply to David Keeler from comment #31)
> Created attachment 650918 [details] [diff] [review]
> patch v2 rebased
> 
> Had to rebase. Carrying over r+  from bz and jaws.
> Try run: https://tbpl.mozilla.org/?tree=Try&rev=8bedebfbe986

<3

https://hg.mozilla.org/integration/mozilla-inbound/rev/241a91038e8c

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/241a91038e8c

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

Wontfixing for FF15 since we've now gone to build on our 5th Beta, but please nominate for Aurora uplift if this is ready.

Comment 36

[Alex Keybl [:akeybl]]

Untracking for FF16 - this has been an issue since 14.0's release, so no need to track. If the fix is deemed low risk, we may consider uplift to Beta 16.

Comment 37

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

My impression from talking to bz is that the fix is not low risk. Also, there are simple work-arounds for the bug, so I don't think we need to uplift this.

Comment 38

[Cristian Nicolae (:xti)]

Flash content is displayed after I tapped on plugin placeholder. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-24)
Device: Galaxy Note
OS: Android 4.0.4