741128 - Plugin placeholder not shown when embeds become visible by dynamically changing height/width

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.

Steps to reproduce:
- Visit testcase, ignore the plugin doorhanger by tapping outside of it
- Tap on the 'make flash embeds visible' button


Expected result:
- Plugin placeholders visible with "Tap here to activate plugin" message

Actual result:
- No plugin placeholders rendered

Comment 1

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

In /mobile/android/chrome/content/browser.js, at the following lines:

> // If the overlay is too small, hide the overlay and act like this
> // is a hidden plugin object
> let overlay = plugin.ownerDocument.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> if (!overlay || PluginHelper.isTooSmall(plugin, overlay)) {
>   if (overlay)
>     overlay.style.visibility = "hidden";
>   if (this.loadEventProcessed && !this.clickToPlayPluginDoorhangerShown)
>     PluginHelper.showDoorHanger(this);
>   return;
> }

the setting of |overlay.style.visibility| and the |return| should be removed.

Comment 2

[:Margaret Leibovic]

Attachment: patch

This patch makes the testcase in this bug work, but I found it doesn't fix bug 741130 as well because in that testcase the overlay in null when the PluginClickToPlay event fires, so the click handler never gets added to it.

Additionally, this patch will mean that really small/unreadable overlays will still be visible, and I'm not sure we want that. Ideally, we want to be able to listen for changes in the size/visibility of the overlays, but I'm not sure how we'd do that.

Comment 3

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

(In reply to Margaret Leibovic [:margaret] from comment #2)
> Created attachment 611910 [details] [diff] [review]
> patch
> 
> This patch makes the testcase in this bug work, but I found it doesn't fix
> bug 741130 as well because in that testcase the overlay in null when the
> PluginClickToPlay event fires, so the click handler never gets added to it.

If it's display:none the binding won't be attached, so I'm not sure what can be done in that case. Tell developers to use a different method to show/hide plugins?
 
> Additionally, this patch will mean that really small/unreadable overlays
> will still be visible

I think in those cases sites should either be using width:0+height:0, visibility:hidden, or display:none. If they have their plugin at non-zero width/height, then I'm not sure what they expect to happen.

> and I'm not sure we want that. Ideally, we want to be
> able to listen for changes in the size/visibility of the overlays, but I'm
> not sure how we'd do that.

That's bug 227495.

Comment 4

[Johnathan Nightingale [:johnath]]

Marking this one a final blocker, based on the decision to mark bug 741130 as a blocker, and this being pretty likely a dup.

Comment 6

[:Margaret Leibovic]

(In reply to Jared Wein [:jaws] from comment #3)
> (In reply to Margaret Leibovic [:margaret] from comment #2)
> > Created attachment 611910 [details] [diff] [review]
> > patch
> > 
> > This patch makes the testcase in this bug work, but I found it doesn't fix
> > bug 741130 as well because in that testcase the overlay in null when the
> > PluginClickToPlay event fires, so the click handler never gets added to it.
> 
> If it's display:none the binding won't be attached, so I'm not sure what can
> be done in that case. Tell developers to use a different method to show/hide
> plugins?

I don't think we have the power to do that right now. At one point I was attaching the listener to the plugin element itself, but the problem with that we don't want the listener after the plugin has been activated, and it's hard to go through and remove listeners after playAllPlugins() is called. We could have a check in the click handler to see if the plugin was already activated. This is less clean than the current approach, but it would solve the problem. What do you think?

> > Additionally, this patch will mean that really small/unreadable overlays
> > will still be visible
> 
> I think in those cases sites should either be using width:0+height:0,
> visibility:hidden, or display:none. If they have their plugin at non-zero
> width/height, then I'm not sure what they expect to happen.

Yeah, this is an edge case, and I don't think we should worry about it too much.

> > and I'm not sure we want that. Ideally, we want to be
> > able to listen for changes in the size/visibility of the overlays, but I'm
> > not sure how we'd do that.
> 
> That's bug 227495.

I don't think that will happen before we want to ship fennec native :)

Comment 7

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

(In reply to Margaret Leibovic [:margaret] from comment #6)
> (In reply to Jared Wein [:jaws] from comment #3)
> > (In reply to Margaret Leibovic [:margaret] from comment #2)
> > > Created attachment 611910 [details] [diff] [review]
> > > patch
> > > 
> > > This patch makes the testcase in this bug work, but I found it doesn't fix
> > > bug 741130 as well because in that testcase the overlay in null when the
> > > PluginClickToPlay event fires, so the click handler never gets added to it.
> > 
> > If it's display:none the binding won't be attached, so I'm not sure what can
> > be done in that case. Tell developers to use a different method to show/hide
> > plugins?
> 
> I don't think we have the power to do that right now. At one point I was
> attaching the listener to the plugin element itself, but the problem with
> that we don't want the listener after the plugin has been activated, and
> it's hard to go through and remove listeners after playAllPlugins() is
> called. We could have a check in the click handler to see if the plugin was
> already activated. This is less clean than the current approach, but it
> would solve the problem. What do you think?

Let's stick with the current patch and reopen bug 741130 for sites that are using display:none for showing and hiding plugin elements.

Since plugins are replaced elements we may not know the dimensions at the time of our event handler, so dynamically visible plugins might not be using any CSS changes.

Comment 8

[:Margaret Leibovic]

(In reply to Jared Wein [:jaws] from comment #7)
> (In reply to Margaret Leibovic [:margaret] from comment #6)
> > (In reply to Jared Wein [:jaws] from comment #3)
> > > (In reply to Margaret Leibovic [:margaret] from comment #2)
> > > > Created attachment 611910 [details] [diff] [review]
> > > > patch
> > > > 
> > > > This patch makes the testcase in this bug work, but I found it doesn't fix
> > > > bug 741130 as well because in that testcase the overlay in null when the
> > > > PluginClickToPlay event fires, so the click handler never gets added to it.
> > > 
> > > If it's display:none the binding won't be attached, so I'm not sure what can
> > > be done in that case. Tell developers to use a different method to show/hide
> > > plugins?
> > 
> > I don't think we have the power to do that right now. At one point I was
> > attaching the listener to the plugin element itself, but the problem with
> > that we don't want the listener after the plugin has been activated, and
> > it's hard to go through and remove listeners after playAllPlugins() is
> > called. We could have a check in the click handler to see if the plugin was
> > already activated. This is less clean than the current approach, but it
> > would solve the problem. What do you think?
> 
> Let's stick with the current patch and reopen bug 741130 for sites that are
> using display:none for showing and hiding plugin elements.

We can do that, but we still have the same problem - how should we fix this? Do you have a better idea than attaching the click listener to the plugin itself?

> Since plugins are replaced elements we may not know the dimensions at the
> time of our event handler, so dynamically visible plugins might not be using
> any CSS changes.

I don't understand what you mean by this comment - are you talking about plugin elements that are dynamically added, rather than hidden/shown? Isn't that a separate issue? (And doesn't that work correctly?)

Comment 9

[:Margaret Leibovic]

Updating the summary to reflect the actual problem with this testcase.

Comment 10

[:Margaret Leibovic]

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

I reopened bug 741130 to address the display: none; to display: block; problem, so we can land this patch to address the changing height/width problem.

Comment 11

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

I think we should continue to put the event handler on the overlay. I think this patch will fix most if not all sites. We can use bug 741130 to track any sites that are using display:none and if we find that it is necessary then at that time we should think about a different solution.

For "replaced elements" I am using the term found in the CSS spec (http://www.w3.org/TR/CSS21/conform.html#replaced-element). I am talking about plugin elements that do not have their dimensions specified by the page they are loaded in. Our event handler may be calculating their width before the content has loaded, and thus might not be the final width and height.

Comment 12

[:Margaret Leibovic]

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

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a13c9668e47f

Comment 14

[Martijn Wargers (dead)]

Verified fixed with a 2012-04-08 nighlty build on the Samsung Galaxy Nexus.