Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 18

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: rc, Assigned: jaws)

Tracking

(Depends on: 1 bug)

20 Branch
Firefox 20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18+ fixed, firefox19+ verified, firefox20+ verified, firefox-esr1718+ verified, b2g18 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Blocks: 810082

Comment 1

5 years ago
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

Updated

5 years ago
Blocks: 738698
Component: General → Plug-ins
Product: Firefox → Core
Version: unspecified → 20 Branch

Comment 2

5 years ago
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

5 years ago
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!
Assignee: nobody → jaws
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox-esr17: --- → affected
tracking-firefox18: --- → +
tracking-firefox19: --- → +
tracking-firefox20: --- → +
tracking-firefox-esr17: --- → 18+

Comment 4

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

Comment 5

5 years ago
(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.
Created attachment 690704 [details] [diff] [review]
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.
Attachment #690704 - Flags: review?(dao)
(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 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);
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Created attachment 690734 [details] [diff] [review]
Patch v.2 (untested)

Thanks for catching that!
Attachment #690704 - Attachment is obsolete: true
Attachment #690704 - Flags: review?(dao)
Attachment #690734 - Flags: review?(dao)

Updated

5 years ago
Component: Plug-ins → General
Product: Core → Firefox
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.
Attachment #690734 - Flags: review?(dao) → review-
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.
Duplicate of this bug: 820348
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?
Blocks: 820303
(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.
(In reply to Jared Wein [:jaws] from comment #14)
> Please file a new bug for general click-to-play.

OK. I filed bug 820437 ;)
Created attachment 690981 [details] [diff] [review]
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.
Attachment #690734 - Attachment is obsolete: true
Attachment #690981 - Flags: review?(dao)
Duplicate of this bug: 820437
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.
Created attachment 691036 [details] [diff] [review]
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)")).
Attachment #690981 - Attachment is obsolete: true
Attachment #690981 - Flags: review?(dao)
Attachment #691036 - Flags: review?(gavin.sharp)
Attachment #691036 - Flags: review?(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.
Attachment #691036 - Flags: review?(dao) → review-
Created attachment 691041 [details] [diff] [review]
Patch v5

Sigh, sorry for missing that.
Attachment #691036 - Attachment is obsolete: true
Attachment #691036 - Flags: review?(gavin.sharp)
Attachment #691041 - Flags: review?(gavin.sharp)
Attachment #691041 - Flags: review?(dao)

Updated

5 years ago
Attachment #691041 - Flags: review?(dao) → review+
Created attachment 691103 [details] [diff] [review]
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
Attachment #691103 - Flags: approval-mozilla-beta?
Comment on attachment 691103 [details] [diff] [review]
Patch for beta and aurora

This patch also applies to aurora.
Attachment #691103 - Attachment description: Patch for beta → Patch for beta and aurora
Attachment #691103 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #691103 - Flags: approval-mozilla-beta?
Attachment #691103 - Flags: approval-mozilla-beta+
Attachment #691103 - Flags: approval-mozilla-aurora?
Attachment #691103 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-beta/rev/6f40f3113263
status-firefox18: affected → fixed
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)
status-firefox18: fixed → affected
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) {
>     ...
>   }
> }
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.
(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?
status-firefox18: affected → fixed
status-firefox19: affected → fixed
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.
Depends on: 820678
(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.
Attachment #691041 - Flags: review?(gavin.sharp)
https://hg.mozilla.org/mozilla-central/rev/2605aa5980f9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox20: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
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
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e904ee016207
status-b2g18: --- → fixed
Duplicate of this bug: 822192
Depends on: 822720
Depends on: 809867

Updated

5 years ago
Attachment #691103 - Flags: approval-mozilla-esr17+
https://hg.mozilla.org/releases/mozilla-esr17/rev/cdeb836c9366
status-firefox-esr17: affected → fixed
Any objections considering comment 32?
(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.
Depends on: 825035
(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.
Depends on: 825201
Keywords: verifyme
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.
Verified fixed FF 19, 20b1, 17.0.3 ESR.
Status: RESOLVED → VERIFIED
status-firefox19: fixed → verified
status-firefox20: fixed → verified
status-firefox-esr17: fixed → verified
Keywords: verifyme
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/
Status: VERIFIED → RESOLVED
Last Resolved: 5 years ago5 years ago
You need to log in before you can comment on or make changes to this bug.