920677 - Click-to-play plugins fail to show placeholder after resizing

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[[e-mail]]

+++ This bug was initially created as a clone of Bug #790483 +++

When a plugin tag is created as click-to-play, we'll hide the placeholder UI if the frame is too small, but fail to show it when the frame is later resized. This breaks sites like:

http://news.yahoo.com/blogs/trending-now/long-lost-renoir-masterpiece-found-among-junk-flea-170238665.html

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Frank Wein [:mcsmurf]]

Too bad the suite/browser/test/browser_pluginnotification.js test still seems to fail with that patch. But I'll check what can be done about that. In general CTP on YouTube and other pages seems to work again with this patch.

Comment 3

[Frank Wein [:mcsmurf]]

Neil: Actually the failing test is a real problem; disable for example the Flash plugin in the addons manager and then open this page:
data:text/html,<object type="application/x-shockwave-flash"/>

Normally you should see a plugin overlay with a link to the add ons manager so that you can enable the plugin again. In this case no plugin overlay appears at all. But: This also happens without your patch (not sure when exactly this regressed..). Should I file a new bug for that?

Comment 4

[Frank Wein [:mcsmurf]]

The disabled plugin thing broke between 2013-09-23 and 2013-09-27 (no nightly builds between those two), hg revs:
fine: http://hg.mozilla.org/mozilla-central/rev/f97307cb4c95
http://hg.mozilla.org/comm-central/69f939e4cded

broken: http://hg.mozilla.org/mozilla-central/rev/e4cd2242cc7d
http://hg.mozilla.org/comm-central/ef87acafa4f7

Looks like Bug Bug 920927 to me, maybe SeaMonkey code needs an additional fix for the disabled plugin case?

Comment 5

[Frank Wein [:mcsmurf]]

Actually Bug 920927 landed on the 26th and was backed out again on the 30th (SeaMonkey builds from 1st of October are still affected). So probably rather Bug 790483 then, which this bug here tries to port/fix.

Comment 6

[[e-mail]]

Attachment: Fixed patch

I gave the disabled plugin case a notification so that it shares the same code path with the other plugins. I can't change any strings because this needs to land on the branch but I think those strings will do for now.

Comment 7

[Frank Wein [:mcsmurf]]

Comment on attachment 816317 [details] [diff] [review]
Fixed patch

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

All in all I think the patch is fine to go on branches, I don't want to delay this patch too much. See below on what would be nice to have fixed on checkin if possible.

::: suite/common/bindings/notification.xml
@@ +577,3 @@
>              var overlay = this.getPluginUI(aPlugin, "main");
> +            aPlugin.addEventListener("overflow", this);
> +            aPlugin.addEventListener("underflow", this);

One note regarding the pluginUnavailable method: Maybe we should add a check for the value of aNotification to not add it the missingPlugins list when the notification is of type disabled-plugin:
568             // Save information on the plugin to give to the plugin finder.
569             var pluginInfo = this.getPluginInfo(aPlugin);
570             this.missingPlugins.set(pluginInfo.mimetype, pluginInfo);

May seem a bit hacky, but I think that's fine in this case ;). Also should not really affect performance in any way. The problem is otherwise when there is a missing plugin and a disabled plugin on a page, the plugin finder service would try to download both plugins when actually only one plugin needs to be downloaded.

There's another problem though with the new plugin-disabled notification: It displays an overlay when I think it should not. See http://hg.mozilla.org/comm-central/annotate/4b9e2dcb78d3/suite/browser/test/browser/browser_pluginnotification.js#l478 this test fails with this patch. When a click-to-play plugin is "disabled" via "Never activate plugins for this site", it displays the "plugin disabled" overlay for that page (and a notification bar appears). On the one hand, this description is ok. But: It offers a "Manage Plugins" link, which does not help the user in this case as it opens the addons manager/plugins tab. There the user cannot enable the plugin again though, as it has been disabled because of some plugin permission. Is there an easy way to fix this problem?
Hm, maybe I should work on porting the new Firefox click-to-play UI, the new one actually has a few things that are better than in this one. Though their UI is targeted at making CTP the default as they have no "Never display plugin for this page" UI anymore.

Comment 8

[Frank Wein [:mcsmurf]]

I've noticed there's nsIObjectLoadingContent::PLUGIN_USER_DISABLED (this one is used for plugins that don't get displayed because of user content prefs as I understand it), maybe we should handle this case in PluginBindingAttached and hide the managePlugins link when the user has decided to never activate plugins on this page:
                 var manageLink = this.getPluginUI(plugin, "managePluginsLink");
                 manageLink.style.visibility = "hidden";
or something like that.

With this solution I would be fine.

Comment 9

[[e-mail]]

(In reply to Frank Wein from comment #8)
> I've noticed there's nsIObjectLoadingContent::PLUGIN_USER_DISABLED
Not actually used sorry.

I suppose I could test for the plugin permission and not show the manage button, or maybe it would be easier if I just removed the button and we deal with that in a followup bug.

Comment 10

[Frank Wein [:mcsmurf]]

Oh, too bad, http://hg.mozilla.org/mozilla-central/annotate/423b9c30c73d/content/base/src/nsObjectLoadingContent.cpp#l1994 looked like it does something.
In that case I would vote for removing the button and dealing with this in another bug (possibly also implementing the new FF UI in SeaMonkey and enabling CTP by default).

Comment 11

[[e-mail]]

Attachment: Possible patch

This version checks for the permissions and if the plugin is disabled in the add-ons manager then continues to display the placeholder and prompt as before but if it was disabled in permissions then it leaves the plugin hidden (note that disabling the permissions from the doorhanger hides the plugin).

Comment 12

[Frank Wein [:mcsmurf]]

Comment on attachment 819694 [details] [diff] [review]
Possible patch

Good solution.

Comment 13

[[e-mail]]

Pushed comm-central changeset 41f44f6f0b0c.

Comment 14

[[e-mail]]

Comment on attachment 819694 [details] [diff] [review]
Possible patch

[Approval Request Comment]
Regression caused by (bug #): 790483 was uplifted to Gecko 26
User impact if declined: Click-to-play plugins are invisible
Testing completed (on m-c, etc.): Rode the aurora train
Risk to taking this patch (and alternatives if risky): No alternative
String changes made by this patch: None

Comment 15

[Philip Chee]

Comment on attachment 819694 [details] [diff] [review]
Possible patch

a=me for comm-beta

Comment 16

[[e-mail]]

https://hg.mozilla.org/releases/comm-beta/rev/1f67b8ac530f