Closed Bug 741130 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox15+ wontfix, firefox16- wontfix, firefox18 verified, blocking-fennec1.0 -)

VERIFIED FIXED
Firefox 17
Tracking Status
firefox15 + wontfix
firefox16 - wontfix
firefox18 --- verified
blocking-fennec1.0 --- -

People

(Reporter: martijn.martijn, Assigned: keeler)

References

()

Details

(Keywords: testcase)

Attachments

(1 file, 2 obsolete files)

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.
I think this is the same as bug 741128.
Whiteboard: dupe of 741128?
Depends on: 741128
I'm seeing this bug on real sites, where flash ads are dynamically made visible.
blocking-fennec1.0: --- → ?
Status: NEW → RESOLVED
blocking-fennec1.0: ? → +
Closed: 12 years ago
Resolution: --- → DUPLICATE
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.
Assignee: nobody → margaret.leibovic
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Whiteboard: dupe of 741128?
(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.
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)..
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.
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.
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
Blocks: 744060
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.
bug 741130 related?
(In reply to Tony Chung [:tchung] from comment #11)
> bug 741130 related?

... that's this bug. I think you made a copy/paste mistake :)
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)
blocking-fennec1.0: + → ?
blocking-fennec1.0: ? → -
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.
(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.
David, would you be interested in taking this bug, since it's also an issue on desktop?
Sure - I can work on it.
Assignee: margaret.leibovic → dkeeler
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?
(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.
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #643526 - Flags: review?(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.
Attachment #643526 - Flags: review?(margaret.leibovic)
Attachment #643526 - Flags: review-
Attachment #643526 - Flags: feedback?(jaws)
Could we iterate through the plugins that are in the content window utils array to remove the event listeners?
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?)
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.
Attachment #643526 - Flags: feedback?(jaws)
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #643526 - Attachment is obsolete: true
Attachment #648021 - Flags: review?(bzbarsky)
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).
Attachment #648021 - Flags: review?(jaws)
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.
Attachment #648021 - Flags: review?(jaws) → review+
Comment on attachment 648021 [details] [diff] [review]
patch v2

r=me
Attachment #648021 - Flags: review?(bzbarsky) → review+
Attached patch patch v2 rebasedSplinter Review
Had to rebase. Carrying over r+  from bz and jaws.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=8bedebfbe986
Attachment #648021 - Attachment is obsolete: true
Attachment #648021 - Flags: review?(khuey)
Attachment #650918 - Flags: review+
Try looks good. Marking checkin-needed.
Keywords: checkin-needed
(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
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/241a91038e8c
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Wontfixing for FF15 since we've now gone to build on our 5th Beta, but please nominate for Aurora uplift if this is ready.
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.
tracking-fennec: --- → ?
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.
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
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.