Add UI for new plugin.disable preference

RESOLVED FIXED

Status

SeaMonkey
Preferences
--
enhancement
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: ewong)

Tracking

({helpwanted})

Trunk
helpwanted
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

7 years ago
Bug 189378 added a preference to disable all preferences globally.
We could add UI for this to the Scripts & Plugins preference panel.
I'd rather not have something disable all preferences globally, but otherwise confirming valid RFE. ;-)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

6 years ago
Where should this new plugin.disabled UI be?  

Under Advanced->Scripts&Plugins?  But then it looks pretty full unless
we decrease the height of the enable/disable list of prefs box (the
one starting with "Move or resize existing windows"...)
(In reply to comment #2)
> Where should this new plugin.disabled UI be?  

Several options:
a) directly on the Advanced pane, below "Enable Java" (same groupbox)
b) on the Scripts & Plugins pane (which already has it in the name, so this should be preferred), as the general case of "Enable Plugins for" (disabling that if the pref discussed here is true). I'd expect the box containing the checkboxes below "Allow scripts to" to reduce itself in vertical size itself.
(Reporter)

Comment 4

6 years ago
(In reply to comment #3)
> (In reply to comment #2)
> > Where should this new plugin.disabled UI be?  
> b) on the Scripts & Plugins pane (which already has it in the name, so this
> should be preferred)
SeaMonkey 1.x's preference pane had a placeholder for it there (added by bug 147877). See also bug 189378 comment 0!
(Assignee)

Updated

6 years ago
Assignee: nobody → ewong
Status: NEW → ASSIGNED
(Assignee)

Comment 5

6 years ago
Created attachment 584930 [details] [diff] [review]
Added UI for plugins.disabled preference. (v1)
Attachment #584930 - Flags: review?(iann_bugzilla)

Comment 6

6 years ago
Comment on attachment 584930 [details] [diff] [review]
Added UI for plugins.disabled preference. (v1)

>+function updatePluginPreferences() {
>+  var test = document.getElementById("pluginPreferences");
Could do with a better variable name here. Perhaps it might be better to get the relevant children by TagName as well.
>+  let i;
Just put this within the for declaration
>+  for (i=1; i <= test.childElementCount-1; i++){
Nit: need spacing around = and use < then don't need the -1
>+    let loopChild = test.children[i]
>+    loopChild.disabled = document.getElementById("pluginsDisabled").checked;
Would probably be better to get the checked status outside of the loop and then use it within the loop.
r- for the moment as I would like to see the new patch.
Attachment #584930 - Flags: review?(iann_bugzilla) → review-

Comment 7

6 years ago
> +  var test = document.getElementById("pluginPreferences");
"test" isn't terribly edifying. how about "plugins"?

> +  let i;
> +  
> +  for (i=1; i <= test.childElementCount-1; i++){

Use for (let i = 1; ... also spaces around the "="

https://developer.mozilla.org/en/DOM/Element.childElementCount
> Warning: Don't use it. Support might be dropped in future versions of Gecko. See bug 673790.
Use .children instead. e.g.

for (let i = 0; i < plugins.children.length -1; i++)

Also did you also intend to disable the <caption> inside the groupbox?

> +    let loopChild = test.children[i]
suggest: |let child =| loopChild is redundant since you are already inside a loop.

> +    loopChild.disabled = document.getElementById("pluginsDisabled").checked;

Move |document.getElementById("pluginsDisabled").checked| out of the loop.

> +  }

> +      <preference id="plugins.disabled"
> +                  name="plugins.disabled"
> +                  type="bool"

As far as I can tell it's |plugin.disable| singular without any "s", and disable rather than disableD.

http://hg.mozilla.org/mozilla-central/annotate/0d684c34d1e4/dom/plugins/base/nsPluginHost.cpp#l402
(Reporter)

Comment 8

6 years ago
Comment on attachment 584930 [details] [diff] [review]
Added UI for plugins.disabled preference. (v1)

>+  
Nit: trailing whitespace

>+  for (i=1; i <= test.childElementCount-1; i++){
[Because this is XUL you don't have to worry about non-elements; just use childCount and childNodes everywhere.]

> <!ENTITY enablePlugin.label              "Enable Plugins for">
As I understand it, the original prefpane was to look something like this:
+- Enable Plugins for --+
| [X] Browser           |
| [ ] Mail & Newsgroups |
+-----------------------+
The Mail & Newsgroups checkbox disabling would still apply of course, but maybe it belongs in mailPrefsOverlay instead.
(Assignee)

Comment 9

6 years ago
Created attachment 585145 [details] [diff] [review]
Add UI for new plugins.disable preference. (v2)
Attachment #584930 - Attachment is obsolete: true
Attachment #585145 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 10

6 years ago
Created attachment 585176 [details] [diff] [review]
Add UI for new plugins.disable preference. (v3)
Attachment #585145 - Attachment is obsolete: true
Attachment #585176 - Flags: review?(iann_bugzilla)
Attachment #585145 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 11

6 years ago
Created attachment 585249 [details] [diff] [review]
Add UI for new plugins.disable preference. (v4)
Attachment #585176 - Attachment is obsolete: true
Attachment #585249 - Flags: review?(iann_bugzilla)
Attachment #585176 - Flags: review?(iann_bugzilla)

Comment 12

6 years ago
Comment on attachment 585249 [details] [diff] [review]
Add UI for new plugins.disable preference. (v4)

>+  for (let i = 1; i < pluginGroup.children.length; i++){
Nit: space between ) and { please.
>+    let child = pluginGroup.children[i];
>+    child.disabled = pluginDisableChecked;
It still seems readable with these combined into a single line (then you won't need {} either)
>+  }
I presume caption will always be pluginGroup.children[0]

Neil did suggest using childNodes so might be worth checking he is happy for you to use children instead, both give a list of XULElements - one is a NodeList the other HTMLCollection.
Attachment #585249 - Flags: review?(iann_bugzilla) → review+

Comment 13

6 years ago
Both return NodeList(s)

Comment 14

6 years ago
https://developer.mozilla.org/en/childNodes
https://developer.mozilla.org/en/DOM/Element.children

Comment 15

6 years ago
> Both return NodeList(s)
Documentation bug. I've fixed DevMo.
(Assignee)

Comment 16

6 years ago
Created attachment 585279 [details] [diff] [review]
Add UI for new plugins.disable preference. (v5)

used getElementsByTagName()
Attachment #585249 - Attachment is obsolete: true
Attachment #585279 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 17

6 years ago
Created attachment 585313 [details] [diff] [review]
What I was thinking of
Attachment #585313 - Flags: feedback?(iann_bugzilla)

Comment 18

6 years ago
Comment on attachment 585313 [details] [diff] [review]
What I was thinking of

>+++ b/suite/common/pref/pref-scripts.xul
>+    <groupbox id="pluginPreferences">
>       <caption label="&enablePlugin.label;"/>
>+
>+      <checkbox id="pluginAllowNavigator"
>+                label="&enablePluginNavigator.label;"
>+                accesskey="&enablePluginNavigator.accesskey;"
>+                preference="plugin.disable"/>
Hmmm, this doesn't just control Browser, so perhaps Suite or SeaMonkey or something like "Browser and potentially other components listed below:"
Then anything overlayed should be indented as is usually the case when there is a dependency.

Generally f+ though if the above can be sorted.

Does Lightning or anything else overlay this?
Attachment #585313 - Flags: feedback?(iann_bugzilla) → feedback+

Comment 19

6 years ago
> Does Lightning or anything else overlay this?
Lightning: No, it adds it's own prefpanes to the tree.
Anything else: only the SHADOW knows! But otherwise highly unlikely.
(Reporter)

Comment 20

6 years ago
(In reply to Ian Neal from comment #18)
> Hmmm, this doesn't just control Browser, so perhaps Suite or SeaMonkey
Well, that could be said of the Scripts preference too...

Comment 21

6 years ago
Both patches work fine. I didn't detect any peculiarities.
Edmund's patch can be improved by adding
+pref("plugin.disable", false);
to browser-prefs.js and inverting the preference as in Neil's patch. However I prefer Edmunds UI.

>> Hmmm, this doesn't just control Browser, so perhaps Suite or SeaMonkey
Agreed.
> Well, that could be said of the Scripts preference too...
Separate bug?
(Assignee)

Comment 22

5 years ago
Created attachment 586750 [details] [diff] [review]
Add UI for new plugins.disable preference. (v6)
Attachment #585279 - Attachment is obsolete: true
Attachment #586750 - Flags: review?(iann_bugzilla)
Attachment #585279 - Flags: review?(iann_bugzilla)

Comment 23

5 years ago
Comment on attachment 586750 [details] [diff] [review]
Add UI for new plugins.disable preference. (v6)

>+    <groupbox id="pluginPreferences">
>       <caption label="&enablePlugin.label;"/>
>+      <checkbox id="pluginForSuite"
>+                class="indent"
You don't need to indent the suite checkbox, only the mailnews one.
>+                label="&enablePluginForSuite.label;"
>+                accesskey="&enablePluginForSuite.accesskey;"
>+                preference="plugin.disable"/>
>+
r=me with that fixed
Attachment #586750 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 24

5 years ago
Created attachment 588629 [details]
Add UI for new plugin.disable preference. (v7)

nits fixed.
Attachment #586750 - Attachment is obsolete: true
Attachment #588629 - Flags: ui-review?(neil)
Attachment #588629 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #588629 - Flags: ui-review?(neil) → ui-review+
(Assignee)

Comment 25

5 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/600face6bd77
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 718302
Summary: Add UI for new plugins.disabled preference → Add UI for new plugin.disable preference
Attachment #588629 - Attachment description: Add UI for new plugins.disable preference. (v7) → Add UI for new plugin.disable preference. (v7)

Updated

5 years ago
Blocks: 725973
You need to log in before you can comment on or make changes to this bug.