Closed Bug 741128 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- +

People

(Reporter: martijn.martijn, Assigned: Margaret)

References

()

Details

(Keywords: testcase)

Attachments

(1 file)

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
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.
Summary: Plugin placholder not shown when embeds are dynamically made visible → Plugin placeholder not shown when embeds are dynamically made visible
Blocks: 741130
Attached patch patchSplinter Review
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.
Assignee: nobody → margaret.leibovic
Attachment #611910 - Flags: feedback?(jwein)
(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.
Marking this one a final blocker, based on the decision to mark bug 741130 as a blocker, and this being pretty likely a dup.
blocking-fennec1.0: --- → +
Duplicate of this bug: 741130
(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 :)
Attachment #611910 - Flags: feedback?(jwein) → feedback+
(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.
Status: NEW → ASSIGNED
(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?)
Updating the summary to reflect the actual problem with this testcase.
Summary: Plugin placeholder not shown when embeds are dynamically made visible → Plugin placeholder not shown when embeds become visible by dynamically changing height/width
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.
Attachment #611910 - Flags: review?(mark.finkle)
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.
Attachment #611910 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mozilla-central/rev/a13c9668e47f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Verified fixed with a 2012-04-08 nighlty build on the Samsung Galaxy Nexus.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.