Last Comment Bug 746110 - Expose click-to-play plugins preference in the UI
: Expose click-to-play plugins preference in the UI
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: seamonkey2.13
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on: 743312
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-17 05:33 PDT by Ian Neal
Modified: 2012-07-07 08:15 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.16 KB, patch)
2012-06-17 13:04 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Review
patch incl. Help [Checkin: Comment 17] (6.30 KB, patch)
2012-06-18 09:45 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
iann_bugzilla: ui‑review+
Details | Diff | Review

Description Ian Neal 2012-04-17 05:33:08 PDT
Bug 743312 implemented click-to-play plugins, we should probably expose the pref in SM's preferences UI.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2012-04-17 08:04:02 PDT
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).
Comment 2 neil@parkwaycc.co.uk 2012-04-18 08:26:22 PDT
The "Allow scripts to" list is automatically resizeable and scrollable, so you can easily e.g. add a checkbox to the Enable Plugins groupbox.
Comment 3 Jens Hatlak (:InvisibleSmiley) 2012-04-18 09:46:42 PDT
(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. ;-)
Comment 4 Pranav Ravichandran [:pranavrc] 2012-04-20 10:32:16 PDT
I'm willing to work on this. Is this going in Appearance/Content or Advanced/Scripts & Plugins?
Comment 5 Jens Hatlak (:InvisibleSmiley) 2012-04-20 13:49:20 PDT
(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.
Comment 6 neil@parkwaycc.co.uk 2012-04-21 10:29:08 PDT
(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.
Comment 7 neil@parkwaycc.co.uk 2012-04-22 03:09:25 PDT
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.
Comment 8 Jens Hatlak (:InvisibleSmiley) 2012-06-09 01:29:12 PDT
Pranav, what's the status?
Comment 9 Jens Hatlak (:InvisibleSmiley) 2012-06-17 13:04:55 PDT
Created attachment 633921 [details] [diff] [review]
patch

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").
Comment 10 neil@parkwaycc.co.uk 2012-06-17 16:20:57 PDT
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.)
Comment 11 Jens Hatlak (:InvisibleSmiley) 2012-06-18 09:45:04 PDT
Created attachment 634079 [details] [diff] [review]
patch incl. Help [Checkin: Comment 17]

Sorry, forgot the Help changes.

Thankfully accepted your suggestion - the scroll bars are gone! :-)
Comment 12 neil@parkwaycc.co.uk 2012-06-18 09:57:53 PDT
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.
Comment 13 Jens Hatlak (:InvisibleSmiley) 2012-06-18 10:15:38 PDT
(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.
Comment 14 Ian Neal 2012-07-02 14:22:51 PDT
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).
Comment 15 Jens Hatlak (:InvisibleSmiley) 2012-07-03 13:45:56 PDT
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?
Comment 16 Ian Neal 2012-07-07 06:57:31 PDT
(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 17 Jens Hatlak (:InvisibleSmiley) 2012-07-07 08:15:14 PDT
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)

Note You need to log in before you can comment on or make changes to this bug.