Closed
Bug 926605
Opened 10 years ago
Closed 10 years ago
The click-to-activate doorhanger should show all plugins the page tried to use, even if they were removed
Categories
(Core Graveyard :: Plug-ins, defect)
Core Graveyard
Plug-ins
Tracking
(firefox26+ fixed, b2g-v1.2 fixed)
RESOLVED
FIXED
mozilla27
People
(Reporter: benjamin, Assigned: benjamin)
References
Details
Attachments
(7 files)
30.62 KB,
patch
|
jaws
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
18.80 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
2.12 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
4.59 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
8.54 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
Some pages try to use a plugin, and if it doesn't work they immediately remove the plugin from the page. We should continue to include those plugins in the click-to-activate doorhanger until the next navigation. I believe this will require changing how we show the doorhanger: currently we re-create the PopupNotifications object every time a plugin is added to the page; we should try to create the PopupNotification only once, and merely add more items to it they are discovered.
Assignee | ||
Comment 1•10 years ago
|
||
This was surprisingly not as invasive as I thought it was going to be. There is a second part that I haven't written yet: if a site removes a plugin and the user subsequently chooses to allow, we should refresh the page. I started try run https://tbpl.mozilla.org/?tree=Try&rev=61f44f18eae1 on these.
Attachment #821306 -
Flags: review?(jaws)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #821307 -
Flags: review?(jaws)
Assignee | ||
Updated•10 years ago
|
Assignee: georg.fritzsche → benjamin
Assignee | ||
Comment 3•10 years ago
|
||
I ran a separate try run with this extra change: https://tbpl.mozilla.org/?tree=Try&rev=dfb19415e569
Attachment #821315 -
Flags: review?(jaws)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #821459 -
Flags: review?(jaws)
Assignee | ||
Comment 5•10 years ago
|
||
This re-enables this test on Linux because I believe I understand what was previously causing it to fail (asynchronous popup focus after the popupshowing DOM event was fired). But if this causes intermittent linux-only orange it can be re-disabled (bug 909342).
Attachment #821473 -
Flags: review?(jaws)
Comment 6•10 years ago
|
||
Comment on attachment 821306 [details] [diff] [review] part A - When a plugin is removed from a page, continue showing it in the plugin doorhanger, to deal with the cases where the site removes a plugin immediately after trying to use it Review of attachment 821306 [details] [diff] [review]: ----------------------------------------------------------------- There are a lot of CSS changes in this patch, of which I'm not sure why any of them are required. ::: browser/base/content/browser-plugins.js @@ +729,5 @@ > } > } > }, > > + _showClickToPlayNotification: function PH_showClickToPlayNotification(aBrowser, aPlugin, aShowNow) { This function is now extremely long and does many things. Why remove the _makeCenterActions function? @@ +780,5 @@ > + pluginInfo.hidden = true; > + > + let overlay = this.getPluginUI(plugin, "main"); > + if (overlay && overlay.style.visibility != "hidden" && overlay.style.visibility != "") { > + pluginInfo.hidden = false; Why is this checking if the overlay's visibility property is an empty string? Only comparing against "hidden" should be sufficient (we never use visibility:collapse for this overlay). @@ +845,5 @@ > } > > + if (notification) { > + // Don't modify the notification UI while it's on the screen, that would be > + // jumpy and might allow clickjacking. I think this clickjacking concern is a bit of a stretch, although I can understand the desire to stay away from a jumpy UI. ::: browser/base/content/browser.xul @@ -534,5 @@ > <image id="webapps-notification-icon" class="notification-anchor-icon" role="button"/> > <image id="plugins-notification-icon" class="notification-anchor-icon" role="button"/> > <image id="web-notifications-notification-icon" class="notification-anchor-icon" role="button"/> > - <image id="alert-plugins-notification-icon" class="notification-anchor-icon" role="button"/> > - <image id="blocked-plugins-notification-icon" class="notification-anchor-icon" role="button"/> I don't understand why this change is necessary to fix this bug.
Attachment #821306 -
Flags: review?(jaws)
Comment 7•10 years ago
|
||
Comment on attachment 821306 [details] [diff] [review] part A - When a plugin is removed from a page, continue showing it in the plugin doorhanger, to deal with the cases where the site removes a plugin immediately after trying to use it Review of attachment 821306 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-plugins.js @@ -720,5 @@ > - return; > - } > - > - centerActions.sort(function(a, b) { > - return a.pluginName.localeCompare(b.pluginName); The sorting of the center actions shouldn't have been removed.
Attachment #821306 -
Flags: review-
Comment 8•10 years ago
|
||
Tracking so this stays on our radar for FF26, would want this in if possible when we ship CTP by default.
tracking-firefox26:
--- → +
Comment 9•10 years ago
|
||
The entirety of these patches feels very risky to consider for a last-day Beta uplift. I'm trying to put together a simpler patch that would be less risky.
Updated•10 years ago
|
Attachment #821307 -
Flags: review?(jaws)
Updated•10 years ago
|
Attachment #821315 -
Flags: review?(jaws)
Updated•10 years ago
|
Attachment #821459 -
Attachment description: more test fixup - browser_CTP_drag_drop needs to wait for focus before opening the plugin doorhanger, because the window "open" event can actually occur before the widget is visible, which means that the popup may be dismissed by its own parent's delayed → more test fixup - browser_CTP_drag_drop needs to wait for focus before opening the plugin doorhanger, because the window "open" event can actually occur before the widget is visible, which means that the popup may be dismissed by its own parent's delayed
Attachment #821459 -
Flags: review?(jaws)
Updated•10 years ago
|
Attachment #821473 -
Flags: review?(jaws)
Comment 10•10 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #9) > The entirety of these patches feels very risky to consider for a last-day > Beta uplift. I'm trying to put together a simpler patch that would be less > risky. Sorry, I misunderstood the nature of this urgency. This would be a last-day Aurora uplift, or early Beta uplift. I'm still concerned about the reach of these patches, but that does make me feel a bit better.
Updated•10 years ago
|
Attachment #821306 -
Flags: review-
Comment 11•10 years ago
|
||
Comment on attachment 821306 [details] [diff] [review] part A - When a plugin is removed from a page, continue showing it in the plugin doorhanger, to deal with the cases where the site removes a plugin immediately after trying to use it Review of attachment 821306 [details] [diff] [review]: ----------------------------------------------------------------- r=me on these changes, I'm handing nits/issues in a followup patch.
Attachment #821306 -
Flags: review+
Comment 12•10 years ago
|
||
Comment on attachment 821307 [details] [diff] [review] test fixup. Review of attachment 821307 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_pluginnotification.js @@ +441,5 @@ > var objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent); > ok(!objLoadingContent.activated, "Test 18f, Plugin should not be activated"); > > + notification.options.eventCallback = function() { > + notification.options.eventCallback = gPluginHandler._clickToPlayNotificationEventCallback; This looks broken. The event callback will no longer actually call the original event callback, it just sets the callback.
Attachment #821307 -
Flags: review+
Updated•10 years ago
|
Attachment #821315 -
Flags: review+
Comment 13•10 years ago
|
||
Comment on attachment 821459 [details] [diff] [review] more test fixup - browser_CTP_drag_drop needs to wait for focus before opening the plugin doorhanger, because the window "open" event can actually occur before the widget is visible, which means that the popup may be dismissed by its own parent's delayed Review of attachment 821459 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_CTP_drag_drop.js @@ +69,4 @@ > } > > function part6() { > gNewWindow.removeEventListener("load", handleEvent); This line is obsolete now.
Attachment #821459 -
Flags: review+
Comment 14•10 years ago
|
||
Comment on attachment 821473 [details] [diff] [review] part D - fix activation via the context menu: I didn't realize nsContextMenu.js was calling _showClickToPlayNotification Review of attachment 821473 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/Makefile.in @@ -58,5 @@ > $(NULL) > endif > > # browser_CTP_context_menu.js fails intermittently on Linux (bug 909342) > -ifndef MOZ_WIDGET_GTK I'd rather not try to solve this in the same bug. We can land this change (as well as removing the above comment) in bug 909342.
Attachment #821473 -
Flags: review+
Comment 15•10 years ago
|
||
Pushed the full series to try, https://tbpl.mozilla.org/?tree=Try&rev=57a790618db1
Attachment #822531 -
Flags: review?(dkeeler)
Comment on attachment 822531 [details] [diff] [review] Part E - Followups from the previous set of patches Review of attachment 822531 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: browser/base/content/browser-plugins.js @@ +750,5 @@ > let cwu = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Ci.nsIDOMWindowUtils); > // cwu.plugins may contain non-plugin <object>s, filter them out > plugins = cwu.plugins.filter((plugin) => > + plugin.getContentTypeForMIMEType(plugin.actualType) == Ci.nsIObjectLoadingContent.TYPE_PLUGIN); Should this be indented more as an indication it's not it's own statement?
Attachment #822531 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 17•10 years ago
|
||
> There are a lot of CSS changes in this patch, of which I'm not sure why any > of them are required. Previously we would give the notification a different icon ID based on whether it was normal/hidden/insecure. But that status can now change dynamically as plugins are added, so I changed things so that we use a single icon ID and add the other styles via classes. I didn't see a low-risk way to make PopupNotifications let you change the icon ID of a notification after it was created. > > ::: browser/base/content/browser-plugins.js > @@ +729,5 @@ > > } > > } > > }, > > > > + _showClickToPlayNotification: function PH_showClickToPlayNotification(aBrowser, aPlugin, aShowNow) { > > This function is now extremely long and does many things. Why remove the > _makeCenterActions function? Primarily because it no longer operates on all plugins in a page, just a subset, but we could keep it factored out if that would be better. > > @@ +780,5 @@ > > + pluginInfo.hidden = true; > > + > > + let overlay = this.getPluginUI(plugin, "main"); > > + if (overlay && overlay.style.visibility != "hidden" && overlay.style.visibility != "") { > > + pluginInfo.hidden = false; > > Why is this checking if the overlay's visibility property is an empty > string? Only comparing against "hidden" should be sufficient (we never use > visibility:collapse for this overlay). Right after an element is inserted into the DOM but before reflow, it doesn't have any computed style at all, so it was actually returning '' in some cases. This was necessary to fix tests. > This looks broken. The event callback will no longer actually call the > original event callback, it just sets the callback. That is true but actually irrelevant in this case because the only thing the showing callback does is telemetry. Wouldn't hurt to forward, but it doesn't break anything. Yes it should be indented.
Assignee | ||
Comment 18•10 years ago
|
||
> The sorting of the center actions shouldn't have been removed.
Actually I removed it intentionally. Previously I had to sort them because I needed a stable ordering and that was the only way to get it. Now, though, they are sorted by the order they were actually used on the page, which I think is a better choice because advertising-only plugins are more likely to be later/last in the list.
Comment 19•10 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] (Away 24-25 October) from comment #18) > because advertising-only plugins are more likely to > be later/last in the list. I'm not sure what data you have to show this, but I doubt users will understand it. Unless we were to include data on the popup notification stating the order that plugins were instantiated (which is too technical for users), I highly doubt that users will understand why the plugins are displayed in instantiation order. Absent of an obvious-to-users natural sorting order, we should continue to sort the plugins alphabetically. Further, we don't update the popup if it is already being displayed, so I'm not worried about users accidentally clicking on the wrong plugin based on a new one getting inserted in the middle.
Assignee | ||
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/69ccec8b45fa https://hg.mozilla.org/integration/fx-team/rev/809832214af3
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/809832214af3 https://hg.mozilla.org/mozilla-central/rev/69ccec8b45fa
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment 22•10 years ago
|
||
Typo? 2587 #plugins-notification-icon { This should be: 2587 #plugins-notification-icon:active {
Comment 23•10 years ago
|
||
Good catch.
Comment 24•10 years ago
|
||
Comment on attachment 823266 [details] [diff] [review] followup to fix icon on Windows, Landed directly on m-c with approval from the sheriffs over IRC, as https://hg.mozilla.org/mozilla-central/rev/c5e4bb7db0b9
Attachment #823266 -
Flags: review+
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 821306 [details] [diff] [review] part A - When a plugin is removed from a page, continue showing it in the plugin doorhanger, to deal with the cases where the site removes a plugin immediately after trying to use it [Approval Request Comment] Bug caused by (feature/regressing bug #): Missing feature in CtP plugins, required for correct functioning of some Java sites as well as Google Earth User impact if declined: We need to disable CtP-by-default Testing completed (on m-c, etc.): in today's nightly, and I've done some personal focused testing Risk to taking this patch (and alternatives if risky): This patch is not tiny, but is limited to the CtP code, so regressions are unlikely to affect the rest of the browsing experience and are likely to show up with some targeted QA. String or IDL/UUID changes made by this patch: None. There are theme changes that might affect theme authors, so I'll set addon-compat.
Attachment #821306 -
Flags: approval-mozilla-beta?
Comment 26•10 years ago
|
||
Comment on attachment 821306 [details] [diff] [review] part A - When a plugin is removed from a page, continue showing it in the plugin doorhanger, to deal with the cases where the site removes a plugin immediately after trying to use it Approving for uplift to Beta (26) now that we've completed the merge. This should go into our first Beta.
Attachment #821306 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 27•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/5d9759153b18 https://hg.mozilla.org/releases/mozilla-beta/rev/00d300f0de04 https://hg.mozilla.org/releases/mozilla-beta/rev/f966e94fd7c4 Some merging was required, but only for the mochitest manifest file and for the file move from tests/ to tests/general, so no functional changes.
status-firefox26:
--- → fixed
Comment 28•10 years ago
|
||
Do we have websites where this is known to fail so QA can verify this is fixed in Firefox 26.0b1?
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #28) > Do we have websites where this is known to fail so QA can verify this is > fixed in Firefox 26.0b1? Google Earth, I think.
Assignee | ||
Comment 30•10 years ago
|
||
Currently this does not work with Google Earth. I'm investigating as to why. This certainly does work with the "Remove Plugin" at http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html I'm getting some odd results at https://www.mojebanka.cz/InternetBanking/ where the icon shows up if I click several times, but I think there's still an issue here when the plugin is removed quickly.
Comment 31•10 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] (Away 24-25 October) from comment #30) > Currently this does not work with Google Earth. I'm investigating as to why. Okay, thanks. Paul reported this is still not working in bug 745187.
Comment 32•10 years ago
|
||
Automatic merge from m-b to b2g26: https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5d9759153b18 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/00d300f0de04 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/f966e94fd7c4
status-b2g-v1.2:
--- → fixed
Comment 33•10 years ago
|
||
Using this demo http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html I can see that this is fixed on Windows 7 x64 and Mac OS X 10.9 with Firefox 26 beta 1.
Comment 34•10 years ago
|
||
Actually i think it would be better to wait the fixes for bug 745187 and bug 932786 first.
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•