Closed Bug 819992 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox :: General, defect)

20 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 20
Tracking Status
firefox18 + fixed
firefox19 + verified
firefox20 + verified
firefox-esr17 18+ verified
b2g18 --- fixed

People

(Reporter: rcampbell, Assigned: jaws)

References

Details

Attachments

(2 files, 4 obsolete files)

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
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
Component: General → Plug-ins
Product: Firefox → Core
Version: unspecified → 20 Branch
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
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!
We're targeting this for landing in tomorrow's beta, if at all possible.
(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.
Attached patch Patch (untested) (obsolete) (deleted) — Splinter Review
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
Attached patch Patch v.2 (untested) (obsolete) (deleted) — Splinter Review
Thanks for catching that!
Attachment #690704 - Attachment is obsolete: true
Attachment #690704 - Flags: review?(dao)
Attachment #690734 - Flags: review?(dao)
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.
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 ;)
Attached patch Patch v.3 (obsolete) (deleted) — Splinter Review
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)
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.
Attached patch Patch v4 (obsolete) (deleted) — Splinter Review
(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-
Attached patch Patch v5 (deleted) — Splinter Review
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)
Attachment #691041 - Flags: review?(dao) → review+
Attached patch Patch for beta and aurora (deleted) — Splinter Review
[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?
Attachment #691103 - Flags: approval-mozilla-beta?
Attachment #691103 - Flags: approval-mozilla-beta+
Attachment #691103 - Flags: approval-mozilla-aurora?
Attachment #691103 - Flags: approval-mozilla-aurora+
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 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) { > ... > } > }
(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?
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.
(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)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
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
Depends on: 809867
Attachment #691103 - Flags: approval-mozilla-esr17+
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.
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.
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
Closed: 12 years ago12 years ago
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: