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

RESOLVED FIXED in Firefox 55

Status

()

P5
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: MikkCZ, Assigned: perry, Mentored)

Tracking

({good-first-bug, nightly-community})

unspecified
mozilla56
good-first-bug, nightly-community
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox55 fixed, firefox56 fixed)

Details

(Whiteboard: [good-first-bug], triaged [fce-active-legacy])

Attachments

(1 attachment, 3 obsolete attachments)

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.
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.
No longer blocks: 1348089
Mentor: felipc
Keywords: good-first-bug
Whiteboard: [good-first-bug]

Updated

2 years ago
Priority: -- → P5
Whiteboard: [good-first-bug] → [good-first-bug], triaged
Assignee: nobody → jiangperry
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created 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.
(Assignee)

Comment 3

2 years ago
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.
Attachment #8875923 - Flags: review?(rhelmer)
Comment hidden (obsolete)
(Assignee)

Comment 5

2 years ago
Created attachment 8875928 [details] [diff] [review]
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.
Attachment #8875923 - Attachment is obsolete: true
Attachment #8875923 - Flags: review?(rhelmer)
Attachment #8875928 - Flags: review?(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
Attachment #8875928 - Flags: review?(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(...)`
(Assignee)

Comment 8

2 years ago
Created attachment 8876330 [details] [diff] [review]
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.
Attachment #8875928 - Attachment is obsolete: true
Attachment #8876330 - Flags: review?(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.
Attachment #8876330 - Flags: review?(rhelmer) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8876871 [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.
Attachment #8876330 - Attachment is obsolete: true
Attachment #8876871 - Flags: review+

Comment 11

2 years ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c652308e569
Enable flash plugin preferences even when set to 'Never Activate'. r=rhelmer

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5c652308e569
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 13

2 years ago
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]:
Attachment #8876871 - Flags: approval-mozilla-beta?
(Assignee)

Comment 14

2 years ago
Amending Comment 13:
[String changes made/needed]: None
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
Attachment #8876871 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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!
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(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.
Flags: qe-verify-
Whiteboard: [good-first-bug], triaged → [good-first-bug], triaged [fce-active]
Whiteboard: [good-first-bug], triaged [fce-active] → [good-first-bug], triaged [fce-active-legacy]
You need to log in before you can comment on or make changes to this bug.