1357300 - Flash plugin preferences are not accessible when it is set to "never activate"

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P5)

Description

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0 ID:20170417100236 CSet: c697e756f738ce37abc56f31bfbc48f55625d617

STR 1:
1. Open the add-ons manager.
2. Set the Flash plugin "never activate".
3. Click the plugin preferences button.
4. Nothing happens.

STR 2:
1. Open the add-ons manager.
2. Set the Flash plugin other then "never activate".
3. Click the plugin preferences button.
4. Set the state to "never activate".
5. Plugin preferences will disappear.

What should happen:
Flash plugin preferences should open and be visible regardless of the plugin activation state. This is mainly useful when you use some add-on to enable Flash just for a single page, but have it disabled in general.

Interesting fact:
When you double-click the plugin entry in the add-ons manager instead of clicking the preferences button, everything displays as expected.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

This change has been made more visible since bug 1348089 made the Preferences button visible for Flash for all users, whereas it was only visible on Win32 before. But it's a pre-existing problem that affected other plugins and plugin types too.

What's happening is that the Preferences button is disabled, but the Add-ons Manager stylesheets make this button look the same as if it was enabled.  Instead of fixing this, the proper fix here would be to not disable this button when Flash is "Never Activate", because it's not necessary to do so.

Comment 2

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Changes to enabling plugin preferences and minor changes to testcases

Enable flash's (and other plugins') preference button even if they are set to "Never Active". Also includes a test case for the bug and 2 minor fixes for other test cases.

Comment 3

[Perry Jiang [:perry] [no longer employee, use ni?]]

Comment on attachment 8875923 [details] [diff] [review]
Changes to enabling plugin preferences and minor changes to testcases

Enable flash's (and other plugins') preference button even if they are set to "Never Active". Also includes a test case for the bug and 2 minor fixes for other test cases.

Comment 4

[Perry Jiang [:perry] [no longer employee, use ni?]]

Comment on attachment 8875923 [details] [diff] [review]
Changes to enabling plugin preferences and minor changes to testcases

diff -r 6cd0639e02de toolkit/mozapps/extensions/content/extensions.js
--- a/toolkit/mozapps/extensions/content/extensions.js	Tue Jun 06 11:09:58 2017 -0700
+++ b/toolkit/mozapps/extensions/content/extensions.js	Thu Jun 08 16:11:02 2017 -0700
@@ -1220,17 +1220,17 @@
         gEventManager.delegateAddonEvent("onCheckingUpdate", [aAddon]);
         aAddon.findUpdates(listener, AddonManager.UPDATE_WHEN_USER_REQUESTED);
       }
     },
 
     cmd_showItemPreferences: {
       isEnabled(aAddon) {
         if (!aAddon ||
-            (!aAddon.isActive && !aAddon.isGMPlugin) ||
+            (!aAddon.isActive && aAddon.type !== "plugin") ||
             !aAddon.optionsURL) {
           return false;
         }
         if (gViewController.currentViewObj == gDetailView &&
             (aAddon.optionsType == AddonManager.OPTIONS_TYPE_INLINE ||
              aAddon.optionsType == AddonManager.OPTIONS_TYPE_INLINE_BROWSER)) {
           return false;
         }
diff -r 6cd0639e02de toolkit/mozapps/extensions/test/browser/browser-common.ini
--- a/toolkit/mozapps/extensions/test/browser/browser-common.ini	Tue Jun 06 11:09:58 2017 -0700
+++ b/toolkit/mozapps/extensions/test/browser/browser-common.ini	Thu Jun 08 16:11:02 2017 -0700
@@ -52,14 +52,15 @@
 skip-if = os == 'win' # Disabled on Windows due to intermittent failures (bug 1135866)
 [browser_types.js]
 [browser_inlinesettings.js]
 [browser_inlinesettings_browser.js]
 [browser_inlinesettings_custom.js]
 [browser_inlinesettings_info.js]
 [browser_tabsettings.js]
 [browser_pluginprefs.js]
+[browser_pluginprefs_is_not_disabled.js]
 skip-if = buildapp == 'mulet'
 [browser_CTP_plugins.js]
 tags = blocklist
 skip-if = buildapp == 'mulet'
 [browser_webext_options.js]
 tags = webextensions
diff -r 6cd0639e02de toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js
--- a/toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js	Tue Jun 06 11:09:58 2017 -0700
+++ b/toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js	Thu Jun 08 16:11:02 2017 -0700
@@ -55,17 +55,17 @@
 
   let testPlugin = await new Promise(resolve => AddonManager.getAddonByID(testPluginId, resolve));
   isnot(testPlugin, null, "part2.1: Test Plug-in should exist");
 
   let pluginEl = get_addon_element(managerWindow, testPluginId);
   pluginEl.parentNode.ensureElementIsVisible(pluginEl);
   let enableButton = managerWindow.document.getAnonymousElementByAttribute(pluginEl, "anonid", "enable-btn");
   is_element_hidden(enableButton, "part3: enable button should not be visible");
-  let disableButton = managerWindow.document.getAnonymousElementByAttribute(pluginEl, "anonid", "enable-btn");
+  let disableButton = managerWindow.document.getAnonymousElementByAttribute(pluginEl, "anonid", "disable-btn");
   is_element_hidden(disableButton, "part3: disable button should not be visible");
   let menu = managerWindow.document.getAnonymousElementByAttribute(pluginEl, "anonid", "state-menulist");
   is_element_visible(menu, "part3: state menu should be visible");
   let askToActivateItem = managerWindow.document.getAnonymousElementByAttribute(pluginEl, "anonid", "ask-to-activate-menuitem");
   is(menu.selectedItem, askToActivateItem, "part3: state menu should have 'Ask To Activate' selected");
 
   let pluginTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, gHttpTestRoot + "plugin_test.html");
   let pluginBrowser = pluginTab.linkedBrowser;
diff -r 6cd0639e02de toolkit/mozapps/extensions/test/browser/browser_pluginprefs.js
--- a/toolkit/mozapps/extensions/test/browser/browser_pluginprefs.js	Tue Jun 06 11:09:58 2017 -0700
+++ b/toolkit/mozapps/extensions/test/browser/browser_pluginprefs.js	Thu Jun 08 16:11:02 2017 -0700
@@ -34,17 +34,17 @@
     ok(testPluginId, "Test Plug-in should exist")
 
     AddonManager.getAddonByID(testPluginId, function(testPlugin) {
       let pluginEl = get_addon_element(gManagerWindow, testPluginId);
       is(pluginEl.mAddon.optionsType, AddonManager.OPTIONS_TYPE_INLINE, "Options should be inline type");
       pluginEl.parentNode.ensureElementIsVisible(pluginEl);
 
       let button = gManagerWindow.document.getAnonymousElementByAttribute(pluginEl, "anonid", "preferences-btn");
-      is_element_visible(button, "Preferences button should be hidden");
+      is_element_visible(button, "Preferences button should be visible");
 
       button = gManagerWindow.document.getAnonymousElementByAttribute(pluginEl, "anonid", "details-btn");
       EventUtils.synthesizeMouseAtCenter(button, { clickCount: 1 }, gManagerWindow);
 
       wait_for_view_load(gManagerWindow, function() {
         let pluginLibraries = gManagerWindow.document.getElementById("pluginLibraries");
         ok(pluginLibraries, "Plugin file name row should be displayed");
         // the file name depends on the platform
diff -r 6cd0639e02de toolkit/mozapps/extensions/test/browser/browser_pluginprefs_is_not_disabled.js
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/toolkit/mozapps/extensions/test/browser/browser_pluginprefs_is_not_disabled.js	Thu Jun 08 16:11:02 2017 -0700
@@ -0,0 +1,49 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+
+// Tests plugin prefs being enabled
+
+let gManagerWindow;
+
+function test() {
+  waitForExplicitFinish();
+
+  open_manager("addons://list/plugin", function(aWindow) {
+    gManagerWindow = aWindow;
+
+    run_next_test();
+  });
+}
+
+function end_test() {
+  close_manager(gManagerWindow, function() {
+    finish();
+  });
+
+  gManagerWindow = null;
+}
+
+add_test(function() {
+  AddonManager.getAddonsByTypes(["plugin"], function(plugins) {
+    let plugin;
+    for (plugin of plugins) {
+      if (plugin.name === "Test Plug-in") {
+        break;
+      }
+    }
+    ok(plugin, "Test Plug-in snould exist");
+    ok(gManagerWindow.gViewController.commands.cmd_showItemPreferences.isEnabled(plugin), "Test Plug-in preferences should be enabled");
+
+    const pluginTag = getTestPluginTag();
+    ok(pluginTag, "Test Plug-in snould exist");
+    const initialPluginState = pluginTag.enabledState;
+    pluginTag.enabledState = Ci.nsIPluginTag.STATE_DISABLED;
+
+    // Disable the plugin ("Never Active"), but its "Preferences" button should still be enabled
+    ok(gManagerWindow.gViewController.commands.cmd_showItemPreferences.isEnabled(plugin), "Test Plug-in preferences should be enabled");
+    pluginTag.enabledState = initialPluginState;
+
+    run_next_test();
+  });
+});
\ No newline at end of file

Comment 5

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Changes to enabling plugin preferences and minor changes to testcases

Changes to enabling plugin preferences and minor changes to testcases

Enable flash's (and other plugins') preference button even if they are set to "Never Active". Also includes a test case for the bug and 2 minor fixes for other test cases.

Comment 6

[Robert Helmer [:rhelmer]]

Comment on attachment 8875928 [details] [diff] [review]
Changes to enabling plugin preferences and minor changes to testcases

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

The code itself looks good to me - thanks!

Would you mind rewriting the test to use async/await and add_task() instead of the older add_test and callback style? An example would be browser_plugin_enabled_state_locked.js

Comment 7

[Robert Helmer [:rhelmer]]

Comment on attachment 8875928 [details] [diff] [review]
Changes to enabling plugin preferences and minor changes to testcases

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

::: toolkit/mozapps/extensions/test/browser/browser_pluginprefs_is_not_disabled.js
@@ +24,5 @@
> +  gManagerWindow = null;
> +}
> +
> +add_test(function() {
> +  AddonManager.getAddonsByTypes(["plugin"], function(plugins) {

BTW, the AddonManager API now returns a promise if you omit the callback arg:

`AddonManager.getAddonsByTypes(["plugin"]).then(...)`

Comment 8

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Changes to enabling plugin preferences and minor changes to testcases

(Changed testcase to async/await and add_task style.)

Enable flash's (and other plugins') preference button even if they are set to "Never Active". Also includes a test case for the bug and 2 minor fixes for other test cases.

Comment 9

[Robert Helmer [:rhelmer]]

Comment on attachment 8876330 [details] [diff] [review]
Changes to enabling plugin preferences and minor changes to testcases

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

Just a minor nit, this all looks good otherwise. Thanks!

::: toolkit/mozapps/extensions/test/browser/browser_pluginprefs_is_not_disabled.js
@@ +4,5 @@
> +
> +// Tests plugin prefs being enabled
> +
> +function getTestPlugin(aPlugins) {
> +  let testPlugin;

Instead of the for loop here I believe you could just do:

let [testPlugin] = aPlugins.filter(plugin => plugin.name === "Test Plug-in");

You could do this directly in the code below, but I see how you're using the same style as `testPluginTag` in head.js so I think it's fine.

Comment 10

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Changes to enabling plugin preferences and minor changes to testcases.

Enable flash's (and other plugins') preference button even if they are set to "Never Active". Also includes a test case for the bug and 2 minor fixes for other test cases.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c652308e569
Enable flash plugin preferences even when set to 'Never Activate'. r=rhelmer

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c652308e569

Comment 13

[Perry Jiang [:perry] [no longer employee, use ni?]]

Comment on attachment 8876871 [details] [diff] [review]
Changes to enabling plugin preferences and minor changes to testcases.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1357300
[User impact if declined]: Flash plugin preference button appears active but isn't actionable is flash is set to "Never Active"
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Bugfix only changes criteria for a plugin to be enabled
[String changes made/needed]:

Comment 14

[Perry Jiang [:perry] [no longer employee, use ni?]]

Amending Comment 13:
[String changes made/needed]: None

Comment 15

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8876871 [details] [diff] [review]
Changes to enabling plugin preferences and minor changes to testcases.

fix an issue with flash preferences, beta55+

Should be in 55b2

Comment 16

[Caitlin Neiman [:caitmuenster]]

Thanks so much, Perry! Your contribution has been added to our recognition wiki here: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#June_2017. I also just vouched for your profile on mozillians.org. :) 

Welcome onboard!

Comment 17

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

The first scenario now works, but I can still produce the second scenario with Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0 ID:20170614100332 CSet: b266a8d8fd595b84a7d6218d7b8c6b7af0b5027c

STR 2:
1. Open the add-ons manager.
2. Set the Flash plugin other then "never activate".
3. Click the plugin preferences button.
4. Set the state to "never activate". (on the plugin preferences page)
5. Plugin preferences will disappear.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Ah sorry, I forgot to mention here what we talked about on IRC. We're leaving that second scenario for a follow-up. That one is a much less serious issue (and one could argue that it's relatively ok/correct to behave like that, although I agree I'd prefer it to work the way you suggested). But on this bug we were striving to fix the non-working button.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9b5584b94beb

Comment 20

[Andrei Vaida [:avaida]]

(In reply to Perry Jiang [:perry] from comment #13)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: No
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Perry's assessment on manual testing needs and the fact that this fix has automated coverage.