Closed Bug 746110 Opened 8 years ago Closed 8 years ago

Expose click-to-play plugins preference in the UI

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.13

People

(Reporter: iann_bugzilla, Assigned: InvisibleSmiley)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 743312 implemented click-to-play plugins, we should probably expose the pref in SM's preferences UI.
Hmm, the Advanced/Scripts & Plugins pane doesn't have much space left, but I wonder whether we can make the "Enable Plugins for:" box half the size and thus gain some space for this new pref.

Another option would be to put it under Appearance/Content (which is probably easier and not too wrong after all).
Whiteboard: [good first bug][mentor=InvisibleSmiley][lang=xul]
The "Allow scripts to" list is automatically resizeable and scrollable, so you can easily e.g. add a checkbox to the Enable Plugins groupbox.
(In reply to comment #2)
> The "Allow scripts to" list is automatically resizeable and scrollable, so you
> can easily e.g. add a checkbox to the Enable Plugins groupbox.

True, but it doesn't really fit /into/ that groupbox, does it? Read: "Enable plugins for:" ... "Require a click to activate plugins" - That's why I came up with the idea of putting a new groupbox to the right of the existing one. Otherwise, if we changed the groupbox label (caption), we'd probably have to change the labels of the two existing checkboxes. And then it would be /some much easier/ to just add a checkbox to the list on Appearance/Content. ;-)
I'm willing to work on this. Is this going in Appearance/Content or Advanced/Scripts & Plugins?
(In reply to Pranav Ravichandran [:pranavrc] from comment #4)
> I'm willing to work on this.

Great! :-) Just assign the bug to yourself (take). If you have any further questions (e.g. how to provide a patch or work with Bugzilla), feel free to contact me by mail or just add a comment here.

> Is this going in Appearance/Content or
> Advanced/Scripts & Plugins?

Right now, both appear as valid options to me (see the discussion above). Ultimately, this is between the patch author (i.e. you) and the reviewer. Pick one (e.g. Neil from comment 2, or IanN) and either ask them on IRC (irc.mozilla.org, #seamonkey) upfront or implement it one way and see what the reviewer replies after you uploaded your patch and requested review here on this bug.
Assignee: nobody → prp.1111
(In reply to Jens Hatlak from comment #3)
> (In reply to comment #2)
> > The "Allow scripts to" list is automatically resizeable and scrollable, so you
> > can easily e.g. add a checkbox to the Enable Plugins groupbox.
> True, but it doesn't really fit /into/ that groupbox, does it?
Ideally I'd want the checkbox to be disabled if the "Enable Plugins" checkbox is disabled, which is easier to do on the Scripts pane.
Also, we could label it "Automatically activate plugins" (i.e. inverting the preference) which would make it fit more neatly into the groupbox.
Thinking about it I think maybe the word "always" wouldn't go amiss e.g. "Always activate all plugins". This is because sometimes plugins get activated even though click-to-play is on.
Status: NEW → ASSIGNED
Pranav, what's the status?
Attached patch patch (obsolete) — Splinter Review
No reaction to comment 8, taking over.

I took a slightly different approach, looking at it from the "page requires plugins" POV, thus grouping the new preference with the existing one for notification bars. Also I find this pref is pretty much independent from the general plugins enabling pref since it mirrors a user preference, just like the one for notification bars. Hence no JS disabling code needed.

The notification bars pref label is now a bit longer and wraps, but I feel that's OK. The only thing that worries me a bit is that the "Allow scripts to:" list (which now shows a slider by default) cannot be scrolled down completely, at least for me on Win7. :-/

While I was it I also removed the colon from the "Enable Plugins for" caption - we're not doing that anywhere else AFAICS (esp. on the same pane, "Enable JavaScript for").
Assignee: prp.1111 → jh
Attachment #633921 - Flags: review?(neil)
Whiteboard: [good first bug][mentor=InvisibleSmiley][lang=xul]
Comment on attachment 633921 [details] [diff] [review]
patch

The "Allow scripts to:" listbox displayed fine for me on Windows 8 Preview, even when I resized the preferences window.

>-<!ENTITY enablePlugin.label              "Enable Plugins for:">
>+<!ENTITY enablePlugins.label              "Enable Plugins for">
Nit: too much space

>+<!ENTITY displayNotification2.label      "Display a notification bar at the top of the content area if additional plugins are required">
Maybe this should say "Warn me if additional plugins need to be installed"? (It might also help you give the entity a nicer name.)
Attachment #633921 - Flags: review?(neil) → review+
Sorry, forgot the Help changes.

Thankfully accepted your suggestion - the scroll bars are gone! :-)
Attachment #633921 - Attachment is obsolete: true
Attachment #634079 - Flags: review?(neil)
Comment on attachment 634079 [details] [diff] [review]
patch incl. Help [Checkin: Comment 17]

>       <checkbox id="displayPluginsNotification"
>-                label="&displayNotification.label;"
>-                accesskey="&displayNotification.accesskey;"
>+                label="&warnPluginsRequired.label;"
>+                accesskey="&warnPluginsRequired.accesskey;"
Nit: change the id of the checkbox to "warnPluginsRequired" to match?

>   <li><strong>Enable Plugins for</strong>: Use these checkboxes to control how
>     plugins are used:
Interestingly the help does include a colon (but outside of the bold) ;-)

>+        You can activate individual plugin instances with a left mouse click,
Might be worth adding the word visible somewhere?

>+        or activate all plugins on the page using the doorhanger (or
>+        notification bar). The latter also gives you the option to do the
Not sure that notification bar needs ()s?

>+        activtaion either temporarily or remember your choice for the current
>+        website. Remembered choices can be edited using the Data Manager
>+        (Permissions tab).</li>
Aside from the misspelling of activation, this doesn't quite sound right to me, although the best I can do is a rather overlong sentence. IanN?
You can activate individual visible plugin instances with a left mouse click or use the notification to activate all the plugins on the page, optionally remembering your choice for the current website.
Attachment #634079 - Flags: ui-review?(iann_bugzilla)
Attachment #634079 - Flags: review?(neil)
Attachment #634079 - Flags: review+
(In reply to neil@parkwaycc.co.uk from comment #12)
> >+        or activate all plugins on the page using the doorhanger (or
> >+        notification bar). The latter also gives you the option to do the
> Not sure that notification bar needs ()s?

Without the parentheses, "The latter" does not refer to what I intended - both provide the option, not just the notification bar.
How about:
You can activate individual visible plugin instances with a left mouse click or use the notification to activate all the plugins on the page. From the notification you have the option to either temporarily activate the plugins or remember the choice for the current website. Remembered choices can be edited using the Data Manager (Permissions tab).
Ian, I'm mostly fine with your suggestion, but it occurred to me that just saying "notification" is a bit too vague in case of the doorhanger, because it doesn't appear by itself (as opposed to e.g. the doorhangers for add-on downloads). I've missed the small plugin icon in the location bar myself more than once.

The best I could come up with goes something like this:

"... Otherwise, if the page requires plugins, &brandShortName; will show a notification (plugin icon in the location bar or notification bar), plus a placeholder for every plugin instance found on the page (unless the website is whitelisted, see below)."

What do the two of you think?
Attachment #634079 - Flags: ui-review?(iann_bugzilla) → ui-review+
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #15)
> Ian, I'm mostly fine with your suggestion, but it occurred to me that just
> saying "notification" is a bit too vague in case of the doorhanger, because
> it doesn't appear by itself (as opposed to e.g. the doorhangers for add-on
> downloads). I've missed the small plugin icon in the location bar myself
> more than once.
> 
> The best I could come up with goes something like this:
> 
> "... Otherwise, if the page requires plugins, &brandShortName; will show a
> notification (plugin icon in the location bar or notification bar), plus a
> placeholder for every plugin instance found on the page (unless the website
> is whitelisted, see below)."
> 
> What do the two of you think?
Seems good to me.
Comment on attachment 634079 [details] [diff] [review]
patch incl. Help [Checkin: Comment 17]

http://hg.mozilla.org/comm-central/rev/f9abd1dbd599
(with review comments addressed)
Attachment #634079 - Attachment description: patch incl. Help → patch incl. Help [Checkin: Comment 17]
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.13
You need to log in before you can comment on or make changes to this bug.