819992 - Click-to-Play doorhanger appearing on every page with Flash

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Rob Campbell [:rc] (:robcee)]

In today's nightly (2012-12-10) the Click-to-play doorhanger is appearing on every page with flash after a page load or reload.

Comment 1

[Loic]

On this YT page e.g., it's ok like previously:
http://www.youtube.com/watch?v=gFmuNApHFec

but not on this streaming site:
http://www.jango.com/music/Rihanna?l=0

Comment 2

[Paul Rouget [:paul]]

Not sure if related, but this appeared today here:
- go to http://imgur.com/
- see the popup, click "never activate plugin for this site"
- reload
- popup appears again

Comment 3

[Alex Keybl [:akeybl]]

We've discussed how to resolve this.

1) Let's do bug 820109 only once per browser session
2) Let's change the icon from the blue plugin to red blocked plugin in the awesomebar
3) Whenever the red blocked plugin icon is visible, let's animate it (fade out, fade in, fade out, fade in) 

Thanks for taking this on Jared!

Comment 4

[Alex Keybl [:akeybl]]

We're targeting this for landing in tomorrow's beta, if at all possible.

Comment 5

[Alex Keybl [:akeybl]]

(In reply to Paul Rouget [:paul] from comment #2)
> Not sure if related, but this appeared today here:
> - go to http://imgur.com/
> - see the popup, click "never activate plugin for this site"
> - reload
> - popup appears again

These steps do not reproduce for me. May be the difference between enabling CTP manually and having a blocklisted Flash version. Please file a separate bug.

Comment 6

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

Attachment: Patch (untested)

I'm in the middle of a clobber build so this patch is untested, but it *should* work as akeybl had described in the above comment.

Requesting review before I can test it so that I can get a quicker turn-around-time on it to land it in time for beta.

Comment 7

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Alex Keybl [:akeybl] from comment #3)
> We've discussed how to resolve this.
> 
> 1) Let's do bug 820109 only once per browser session
> 2) Let's change the icon from the blue plugin to red blocked plugin in the
> awesomebar
> 3) Whenever the red blocked plugin icon is visible, let's animate it (fade
> out, fade in, fade out, fade in) 
> 
> Thanks for taking this on Jared!

Is your saying to resolve fix showing doorhanger popup on many pages which is talked in bug 810082 ?

If it is so, I think fixing it sufficiency that we'll change to no showing popup when in normal CTP which is not blocking outdated plugins.
If we talk about the annoying behavior reported in bug 810082 comment #35, the root problem is that it's very annoying the showing doorhanger popup steals user focusing. We should fix it.

Comment 8

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Comment on attachment 690704 [details] [diff] [review]
Patch (untested)

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

This code will not work as your expect.

::: browser/base/content/browser-plugins.js
@@ +649,3 @@
>      PopupNotifications.show(aBrowser, "click-to-play-plugins",
>                              messageString, "plugins-notification-icon",
>                              mainAction, secondaryActions, options);

you need to like this:
 PopupNotifications.show(aBrowser, "click-to-play-plugins",
                                    messageString, icon,		
                                    mainAction, secondaryActions, options);

Comment 9

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

Attachment: Patch v.2 (untested)

Thanks for catching that!

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 690734 [details] [diff] [review]
Patch v.2 (untested)

>+  _notificationDisplayed: false,

please rename this to _notificationDisplayedOnce

>       notification.dismissed = false;
>       PopupNotifications._update(notification.anchorElement);
>+      !this._notificationDisplayed = true;

remove exclamation mark

>+#blocked-plugins-notification-icon[showing] {
>+  animation: fadeIn 500ms ease 0s 4 alternate both;
>+}
>+
>+@keyframes fadeIn {
>+  from {
>+    opacity: 0;
>+  }
>+
>+  to {
>+    opacity: 1;
>+  }
>+}

Rename this animation such that the name can't clash with random other animations and is linked to where this animation is used (i.e. blocked-plugins-notification-icon).

>--- a/browser/themes/pinstripe/browser.css
>+++ b/browser/themes/pinstripe/browser.css

>+#blocked-plugins-notification-icon {
>+  list-style-image: url(chrome://mozapps/skin/plugins/notifyPluginBlocked.png);

This isn't the red icon on OS X.

Comment 11

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Comment on attachment 690734 [details] [diff] [review]
Patch v.2 (untested)

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

I comment with assumption which this patch is related to handle the feature of blocking vulnerable plugin.

::: browser/base/content/browser-plugins.js
@@ +283,3 @@
>        notification.dismissed = false;
>        PopupNotifications._update(notification.anchorElement);
> +      !this._notificationDisplayed = true;

this._notificationDisplayed should not be configurated in this part. This part checks whether a scripted plugin is visible or not.

@@ +649,1 @@
>      PopupNotifications.show(aBrowser, "click-to-play-plugins",

If we change this._notificationDisplayed to handle blocking VulnerablePlugin, I think we should refer this._notificationDisplayed in this.

Comment 13

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Bug 820348 has been duplicated.
But I seem that the patch which Jared attached is only related to blocking outdated plugins.

Jared, will your next/other patch fix that showing many popup is very annoying when normal Click-To-Play?
If you have no plan now (you'll work on other bug), may I file the new bug?

Comment 14

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

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #13)
> Bug 820348 has been duplicated.
> But I seem that the patch which Jared attached is only related to blocking
> outdated plugins.
> 
> Jared, will your next/other patch fix that showing many popup is very
> annoying when normal Click-To-Play?
> If you have no plan now (you'll work on other bug), may I file the new bug?

Please file a new bug for general click-to-play.

Comment 15

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Jared Wein [:jaws] from comment #14)
> Please file a new bug for general click-to-play.

OK. I filed bug 820437 ;)

Comment 16

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

Attachment: Patch v.3

Thanks for the quick review Dao and feedback from :saneyuki_s.

This solution also fixes bug 820437 so I'll dupe that here.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 690981 [details] [diff] [review]
Patch v.3

>+#blocked-plugins-notification-icon[showing] {
>+  animation: flashPluginBlockedNotification 500ms ease 0s 5 alternate both;
>+}
>+
>+@keyframes flashPluginBlockedNotification {

"flashPlugin" sounds like you're referring to Adobe Flash. Let's just call this "pluginBlockedNotification".

>+  from {
>+    opacity: 0;
>+  }
>+
>+  to {
>+    opacity: 1;
>+  }
>+}

nit: remove blank line

>--- a/toolkit/themes/pinstripe/mozapps/jar.mn
>+++ b/toolkit/themes/pinstripe/mozapps/jar.mn
>@@ -63,16 +63,17 @@ toolkit.jar:
>   skin/classic/mozapps/plugins/notifyPluginBlocked.png            (plugins/notifyPluginGeneric.png)
>   skin/classic/mozapps/plugins/notifyPluginCrashed.png            (plugins/notifyPluginGeneric.png)
>   skin/classic/mozapps/plugins/notifyPluginGeneric.png            (plugins/notifyPluginGeneric.png)
>   skin/classic/mozapps/plugins/notifyPluginOutdated.png           (plugins/notifyPluginGeneric.png)
>   skin/classic/mozapps/plugins/pluginProblem.css                  (plugins/pluginProblem.css)
>   skin/classic/mozapps/plugins/pluginGeneric.png                  (plugins/pluginGeneric.png)
>   skin/classic/mozapps/plugins/pluginDisabled.png                 (plugins/pluginDisabled.png)
>   skin/classic/mozapps/plugins/pluginBlocked.png                  (plugins/pluginBlocked.png)
>+  skin/classic/mozapps/plugins/pluginBlocked-16.png               (plugins/pluginBlocked-16.png)

It's confusing that this is packed as notifyPluginBlocked.png in winstripe and gnomestripe but pluginBlocked-16.png in pinstripe, while pinstripe also packages notifyPluginBlocked.png but uses it differently.

Comment 19

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

Attachment: Patch v4

(In reply to Dão Gottwald [:dao] from comment #18)
> >+@keyframes flashPluginBlockedNotification {
> 
> "flashPlugin" sounds like you're referring to Adobe Flash. Let's just call
> this "pluginBlockedNotification".

Good call, thanks.

> It's confusing that this is packed as notifyPluginBlocked.png in winstripe
> and gnomestripe but pluginBlocked-16.png in pinstripe, while pinstripe also
> packages notifyPluginBlocked.png but uses it differently.

Yeah, I tried to fix it in this revision by using the correct icon for notifyPluginBlocked.png on pinstripe as well as including the 16px version for pluginBlocked-16.png. Changing notifyPluginBlocked.png to use the 16px gets confusing because it looks to be used for notification bars and there doesn't appear to be an easy way to dynamically switch that icon for HiDPI (maybe navigator.platform.contains("Mac") && window.matchMedia("(min-resolution: 2dppx)")).

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 691036 [details] [diff] [review]
Patch v4

>+@media (min-resolution: 2dppx) {
>+  #blocked-plugins-notification-icon {
>+    list-style-image: url(chrome://mozapps/skin/plugins/notifyPluginBlocked.png);

Isn't this a 16px icon? That is, if it existed (see below), wouldn't it be a 16px icon?

>--- a/toolkit/themes/pinstripe/mozapps/jar.mn
>+++ b/toolkit/themes/pinstripe/mozapps/jar.mn

>-  skin/classic/mozapps/plugins/notifyPluginBlocked.png            (plugins/notifyPluginGeneric.png)
>+  skin/classic/mozapps/plugins/notifyPluginBlocked.png            (plugins/notifyPluginBlocked.png)

toolkit/themes/pinstripe/mozapps/plugins/notifyPluginBlocked.png doesn't exist, as far as I can tell.

Comment 21

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

Attachment: Patch v5

Sigh, sorry for missing that.

Comment 22

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

Attachment: Patch for beta and aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 810082
User impact if declined: the plugin popup appears too often
Testing completed (on m-c, etc.): tested locally in a beta build
Risk to taking this patch (and alternatives if risky): click-to-play/plugin blocklisting notification weirdness, none expected though
String or UUID changes made by this patch: none

Comment 23

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

Comment on attachment 691103 [details] [diff] [review]
Patch for beta and aurora

This patch also applies to aurora.

Comment 24

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

https://hg.mozilla.org/releases/mozilla-beta/rev/6f40f3113263

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Sorry, had to back this out for test failures. Note that typically it's preferred that a patch land on m-c first before going onto the branches.
https://hg.mozilla.org/releases/mozilla-beta/rev/a3e25b271de5

https://tbpl.mozilla.org/php/getParsedLog.php?id=17843525&tree=Mozilla-Beta

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js | waited too long for popup notification to show (plugin_test_scriptedPopup2.html)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js | notification should be showing (plugin_test_scriptedPopup2.html)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js | waited too long for popup notification to show (plugin_test_scriptedPopup1.html)
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js | notification should be showing (plugin_test_scriptedPopup1.html)

Comment 26

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Comment on attachment 691041 [details] [diff] [review]
Patch v5

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

::: browser/base/content/browser-plugins.js
@@ +286,1 @@
>      }

this._notificationDisplayedOnce should be checked *at first*. Because, after this._notificationDisplayedOnce=true, we need not to check haveVisibleCTPPlugin.

So like this:

> if (!this._notificationDisplayedOnce) {
>   ...
>   ...
>   if (notification && !haveVisibleCTPPlugin) {
>     ...
>   }
> }

Comment 27

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

Relanded with test fixes on beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/e904ee016207
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb34bd8957ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/2605aa5980f9

Sorry about landing on beta so quick.

Comment 28

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

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #26)
> Comment on attachment 691041 [details] [diff] [review]
> Patch v5
> 
> Review of attachment 691041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-plugins.js
> @@ +286,1 @@
> >      }
> 
> this._notificationDisplayedOnce should be checked *at first*. Because, after
> this._notificationDisplayedOnce=true, we need not to check
> haveVisibleCTPPlugin.
> 
> So like this:
> 
> > if (!this._notificationDisplayedOnce) {
> >   ...
> >   ...
> >   if (notification && !haveVisibleCTPPlugin) {
> >     ...
> >   }
> > }

This won't break correctness, but is a nice optimization we can make. Can you file a follow-up to make this change?

Comment 29

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I tried patch v5 attachment 691041 [details] [diff] [review]. below is my feeling.

(In reply to Alex Keybl [:akeybl] from comment #3)
> We've discussed how to resolve this.
> 
> 1) Let's do bug 820109 only once per browser session
> 2) Let's change the icon from the blue plugin to red blocked plugin in the
> awesomebar
> 3) Whenever the red blocked plugin icon is visible, let's animate it (fade
> out, fade in, fade out, fade in) 

I think these change are good at blocking outdated plugins automatically. These indicate more clear information to user.

However, at normal click-to-play, I feel this change (1) is not good.
A user using normal click-to-play are regarded as that they understand the feature of normal click-to-play, because it is an opt-in feature.
So showing the doorhanger popup per browser window might be very annoying for user who is used to use normal opt-in click-to-play.

Comment 30

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Jared Wein [:jaws] from comment #28)
> This won't break correctness, but is a nice optimization we can make. Can
> you file a follow-up to make this change?

I filed bug 820678.

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2605aa5980f9

Comment 32

[Paul Silaghi, QA [:pauly]]

I've been testing this today on FF 18b4:

(In reply to Alex Keybl [:akeybl] from comment #3)
> We've discussed how to resolve this.
> 1) Let's do bug 820109 only once per browser session
> 2) Let's change the icon from the blue plugin to red blocked plugin in the
> awesomebar
a) The red icon is outdated plugins related only, the blue one still appears for normal plugins. Also this bug is flash related, but the red icon is displayed for other outdated plugins.

> 3) Whenever the red blocked plugin icon is visible, let's animate it (fade
> out, fade in, fade out, fade in) 
b) The fading animation is also only once per session

c) https://bugzilla.mozilla.org/show_bug.cgi?id=809846#c15
d) https://bugzilla.mozilla.org/show_bug.cgi?id=775857#c4

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e904ee016207

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/cdeb836c9366

Comment 36

[Paul Silaghi, QA [:pauly]]

Any objections considering comment 32?

Comment 37

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

(In reply to Paul Silaghi [QA] from comment #32)
> I've been testing this today on FF 18b4:
> 
> (In reply to Alex Keybl [:akeybl] from comment #3)
> > We've discussed how to resolve this.
> > 1) Let's do bug 820109 only once per browser session
> > 2) Let's change the icon from the blue plugin to red blocked plugin in the
> > awesomebar
> a) The red icon is outdated plugins related only, the blue one still appears
> for normal plugins. Also this bug is flash related, but the red icon is
> displayed for other outdated plugins.

Yes, this is intended.

> > 3) Whenever the red blocked plugin icon is visible, let's animate it (fade
> > out, fade in, fade out, fade in) 
> b) The fading animation is also only once per session

This is happening once per window per session. I'm not sure why the animation doesn't happen each time. We should file a bug on it to investigate further.
 
> c) https://bugzilla.mozilla.org/show_bug.cgi?id=809846#c15

I reopened that bug based on your comment there.

> d) https://bugzilla.mozilla.org/show_bug.cgi?id=775857#c4

We don't have a way to override blocklisted plugin warnings on a per-domain basis.

Comment 38

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

(In reply to Jared Wein [:jaws] from comment #37)
> > > 3) Whenever the red blocked plugin icon is visible, let's animate it (fade
> > > out, fade in, fade out, fade in) 
> > b) The fading animation is also only once per session
> 
> This is happening once per window per session. I'm not sure why the
> animation doesn't happen each time. We should file a bug on it to
> investigate further.

I filed bug 825035 for this.

Comment 39

[Paul Silaghi, QA [:pauly]]

I think before verifying this it would be better to wait for bug 825035, bug 809846 to be fixed and for feedback in bug https://bugzilla.mozilla.org/show_bug.cgi?id=810082#c69 which is part of this.

Comment 40

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 19, 20b1, 17.0.3 ESR.

Comment 41

[Paul Silaghi, QA [:pauly]]

Actually there is a problem here. I don't understand why the first time CTP notification is not displayed on Ubuntu on FF 20, 21, 22, but is displayed on FF 19, 17.0.3 ESR. On all other OSes works fine on all FFs. Tested on http://imgur.com/