Closed Bug 835134 Opened 11 years ago Closed 11 years ago

Modify Privacy & Security Preferences UI to reflect recent backend additions

Categories

(SeaMonkey :: Preferences, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.18

People

(Reporter: philip.chee, Assigned: rsx11m.pub)

References

()

Details

Attachments

(2 files, 8 obsolete files)

From Bug 477718 Comment 47:

> We should probably expose the preferences for enabling/debugging within the
> UI somewhere.
> Debug probably under the Debug pref pane.
> Enabling somewhere under Browser, Privacy & Security or Advanced?
I'd say Privacy & Security.

Firefox places these prefs in their Security Pref pane:
http://hg.mozilla.org/mozilla-central/annotate/47684913d63d/browser/components/preferences/security.xul#l65

      <checkbox id="blockAttackSites"
                label="&blockAttackSites.label;"
                accesskey="&blockAttackSites.accesskey;"
                preference="browser.safebrowsing.malware.enabled" />
      <checkbox id="blockWebForgeries"
                label="&blockWebForgeries.label;"
                accesskey="&blockWebForgeries.accesskey;"
                preference="browser.safebrowsing.enabled" />
(In reply to Philip Chee)
> I'd say Privacy & Security.

I'd agree with that suggestion - that pane is rather crowed already, though.

At least with my default settings, there wouldn't be enough vertical space for two more checkboxes. However, If at least "Cache" and "Cookies" in the Private Data group could move into a second column next to "Browsing/Location Bar History", the first group could be expanded to "User Tracking and Phishing" or similar.

Or, move "Private Data" into its own pane.

In contrast, plenty of space in the "Advanced" main pane, but it would be a bit hidden there and doesn't quite fit to the other (general) options offered there.
> Or, move "Private Data" into its own pane.
I would prefer this myself.
Blocks: 631566
I'll look into it, give me a few days. This should be a nice exercise to learn how to split and move files around in the tree.  ;-)
Status: NEW → ASSIGNED
Assignee: nobody → rsx11m.pub
Attached patch Proposed patch (hg cp/mv) (obsolete) — Splinter Review
This was actually fairly straight-forward, I hope I've got the jar-file handling right and nothing is missing (works for me). This splits off "Private Data" into its own pane as suggested, just leaving the "User Tracking" checkbox in "Privacy & Security". The new "Phishing Protection" group was taken right from the Firefox implementation, including labels and access keys.

The new "Private Data" pane contains a single "Clear Private Data" group which is the same as was previously offered in the "Privacy & Security" pane. I've put it above the Cookies pane as it's more general than the other panes.

As a drive-by fix, I've corrected askBeforeClear.accesskey as it collided with itemCache.accesskey (both were 'a').

Obviously, this goes on top of bug 477718 which hasn't checked in yet
Attachment #707695 - Flags: ui-review?(neil)
Attachment #707695 - Flags: review?(iann_bugzilla)
Attached patch Proposed patch (hg add/rm) (obsolete) — Splinter Review
This as an alternative version of attachment 707695 [details] [diff] [review] producing exactly the same code, but created in a different way. Initially I was using "hg cp" and "hg mv" to derive the new files, to retain the files' histories. If the first patch doesn't apply, please try this one which was only using "hg add" and "hg rm" to account for the added or moved files.
Attached image Screenshot (Windows 7) (obsolete) —
Here the screenshot of the patch in action. Since I haven't applied Phil's patch, the prefs are initially undefined, thus the boxes not checked by default.
Bug 631566 has been filed already to take care of the Help updates.
Comment on attachment 707695 [details] [diff] [review]
Proposed patch (hg cp/mv)

> copy from suite/common/pref/pref-security.xul
> copy to suite/common/pref/pref-privdata.xul
> --- a/suite/common/pref/pref-security.xul
> +++ b/suite/common/pref/pref-privdata.xul
> @@ -2,30 +2,26 @@

> -<!ENTITY % prefSecurityDTD SYSTEM "chrome://communicator/locale/pref/pref-security.dtd">
> +<!ENTITY % prefSecurityDTD SYSTEM "chrome://communicator/locale/pref/pref-privdata.dtd">
>  %prefSecurityDTD;

Oops, that was supposed to be

> -<!ENTITY % prefSecurityDTD SYSTEM "chrome://communicator/locale/pref/pref-security.dtd">
> -%prefSecurityDTD;
> +<!ENTITY % prefPrivDataDTD SYSTEM "chrome://communicator/locale/pref/pref-privdata.dtd">
> +%prefPrivDataDTD;

I'll change the entity name with the next patch after any comments from you (no impact on function).
Some more thoughts on the main Privacy & Security page:

Should the group label "Phishing Protection" be "Safe Browsing" instead, (a) to match the preference name, and (b) to reflect that this both about phishing and malicious web sites?

Also, since we have a bit more space than Firefox in the preference pane, maybe add a short <description> similar to various other panes? My suggestions:

Track Users - "SeaMonkey can ask web sites not to track how you are using their site to maintain privacy. However, it is up to that site whether or not to honor your request."

Safe Browsing - "SeaMonkey can block web sites which have been reported to contain malicious content or try to obtain your login information for legitimate sites (phishing)."

Or, should those go into the help information per bug 631566? In that case, I can remove %brandDTD from pref-security.xul as well.

Please let me know if you want me to consider any of those in the next iteration.
Comment on attachment 707695 [details] [diff] [review]
Proposed patch (hg cp/mv)

>+<!ENTITY % prefSecurityDTD SYSTEM "chrome://communicator/locale/pref/pref-privdata.dtd">
> %prefSecurityDTD;
> <!ENTITY % prefSanitizeDTD SYSTEM "chrome://communicator/locale/sanitize.dtd">
> %prefSanitizeDTD;
[Perhaps we should just put everything in sanitize.dtd?]

>+  <prefpane id="privdata_pane" label="&pref.privdata.title;"
>+            script="chrome://communicator/content/tasksOverlay.js chrome://communicator/content/pref/pref-privdata.js">
>+    <preferences id="privdata_preferences">
> 
>       <!-- Clear Private Data -->
Nit: lose the blank line here

>     <!-- Do Not Track -->
>     <groupbox id="doNotTrackGroup">
>       <caption label="&tracking.label;"/>
> 
>       <hbox id="doNotTrackBox" align="center">
>         <checkbox id="privacyDoNotTrackPrefs"
>                   flex="1"
>                   label="&doNotTrack.label;"
>                   accesskey="&doNotTrack.accesskey;"
>                   preference="privacy.donottrackheader.enabled"/>
>       </hbox>
>     </groupbox>
[So, it looks as if this got copied from the clear private data...]

>-      <hbox id="clearDataBox" align="center">
>-        <checkbox id="alwaysClear" flex="1"
>-                  preference="privacy.sanitize.sanitizeOnShutdown"
>-                  label="&alwaysClear.label;"
>-                  accesskey="&alwaysClear.accesskey;"/>
>-      </hbox>
>-      <hbox id="askClearBox">
>-        <checkbox id="askBeforeClear" flex="1"
>-                  preference="privacy.sanitize.promptOnSanitize"
>-                  label="&askBeforeClear.label;"
>-                  accesskey="&askBeforeClear.accesskey;"/>
>-        <button id="clearDataNow" icon="clear"
>-                labelDialog="&clearDataDialog.label;"
>-                labelSilent="&clearDataSilent.label;"
>-                accesskey="&clearDataDialog.accesskey;"
>-                oncommand="clearPrivateDataNow();"/>
>-      </hbox>
[... which has this combination of checkbox and button that needed an hbox. And unfortunately the align="center" got set on the wrong hbox too. Oops.]

>+      <vbox id="phishingProtectionBox" align="left">
Now, this is an interesting approach, but you can in fact put the align on the groupbox itself.

>+        <checkbox id="blockAttackSites"
>+                  flex="1"
And you don't need the flex here, again that was only for the checkbox button combo. (And I think the other sanitise items flex for consistency, otherwise it would be unnecessary there too.)
Neil, thanks for your comments and the quick turnaround.

> [Perhaps we should just put everything in sanitize.dtd?]

Actually, I was thinking of pref-privdata.dtd being a superset of sanitize.dtd given that the latter is also used for the "Clear Private Data" dialog coming from the menus. It seems to be cleaner to keep the pane-specific entities parallel to the panes, thus I'd tend to keep it.

> >     <!-- Do Not Track -->
> [So, it looks as if this got copied from the clear private data...]

Maybe, but I didn't program that, I'm just carrying it along...  ;-)
I've made the phishing group following those examples, though, that's true.

> [... which has this combination of checkbox and button that needed an hbox.
> And unfortunately the align="center" got set on the wrong hbox too. Oops.]

Yes, the plenty of alignments and flexes irritated me as well, thus I've left it alone. I can try to clean it up a bit.

> >+      <vbox id="phishingProtectionBox" align="left">
> Now, this is an interesting approach, but you can in fact put the align on
> the groupbox itself.

... and it probably should be align="start" anyway, if needed at all, given that it has to be RTL safe, I assume.
What about these suggestions?

(In reply to rsx11m from comment #9)
> Should the group label "Phishing Protection" be "Safe Browsing" instead,

> Also, since we have a bit more space than Firefox in the preference pane,
> maybe add a short <description> similar to various other panes?
Attached image Screenshot (comment #9) (obsolete) —
Ok, so I could safely get rid of the additional boxes and alignments without any effect on appearance, looks much cleaner now. I took the opportunity to create a mockup of the brief descriptions for each group, with slightly shortened labels as proposed to make them fit into two lines. If you don't like this, I'll stick with just the group headings without the introductory strings.
(In reply to rsx11m from comment #11)
> ... and it probably should be align="start" anyway, if needed at all, given
> that it has to be RTL safe, I assume.
Good catch!

(In reply to rsx11m from comment #9)
> Some more thoughts on the main Privacy & Security page:
[Sorry, I only looked at the patch, I forgot to check out the comments]

> Should the group label "Phishing Protection" be "Safe Browsing" instead, (a)
> to match the preference name, and (b) to reflect that this both about
> phishing and malicious web sites?
Yes, I meant to say that "Phishing Protection" is wrong, since it's 50% phishing, 50% malware. "Safe Browsing" sounds fine though.

> Safe Browsing - "SeaMonkey can block web sites which have been reported to
> contain malicious content or try to obtain your login information for
> legitimate sites (phishing)."
"or try to acquire personal details by masquerading as a trustworthy site"?

> Or, should those go into the help information per bug 631566?
A one-line summary seems fine, and more detailed information goes in Help.
Comment on attachment 707989 [details]
Screenshot (comment #9)

I'd prefer "For your privacy".
[This is just me bikeshedding. Feel free to ignore me]

> +   content/communicator/pref/pref-privdata.js
I'd prefer pref-privatedata.js it's not that much longer.
> +  <prefpane id="privdata_pane" label="&pref.privdata.title;"
pref.privatedata.title
...etc... /privdata/privatedata/g

> +<!ENTITY blockAttackSites.label           "Block reported attack sites">
> +<!ENTITY blockAttackSites.accesskey       "k">
> +<!ENTITY blockWebForgeries.label          "Block reported web forgeries">
> +<!ENTITY blockWebForgeries.accesskey      "B">

I'd say the access key for blocked sites should be "B" and the phishing sites should be "k". I know you copied the access keys from Firefox, but the ordering there is a historical accident, where the phishing preference was created first and then only later was the malware preference was inserted.
"f" for phishing perhaps ;-)
> "f" for phishing perhaps ;-)
isth there a listhp?

> Track Users - "SeaMonkey can ask web sites not to track how you are using their site
> to maintain privacy. However, it is up to that site whether or not to honor your
> request."
[FYI: Bug 765398 - Implement three-state UI for DNT (do not track me, okay to track me, decline to state).]
Attached image Screenshot variants (obsolete) —
(In reply to neil@parkwaycc.co.uk from comment #15)
> I'd prefer "For your privacy".
(In reply to neil@parkwaycc.co.uk from comment #14)
> "or try to acquire personal details by masquerading as a trustworthy site"?

I've changed the 2-line descriptions accordingly, see the upper screenshot.

> > Or, should those go into the help information per bug 631566?
> A one-line summary seems fine, and more detailed information goes in Help.

There is not much room to explain a lot in a single line, thus I've moved some details into the "Block..." checkbox labels in this version (lower screenshot).

(In reply to Philip Chee from comment #16)
> I'd prefer pref-privatedata.js it's not that much longer.

Yes, that should be an easy change, and there are other long file names in pref/.

> I'd say the access key for blocked sites should be "B" and the phishing
> sites should be "k".
(In reply to neil@parkwaycc.co.uk from comment #17)
> "f" for phishing perhaps ;-)

I've used "B" for blocking attach sites and "f" (or "P" for the 1-line version as we have now a matching "Phishing" in the label itself).

So, pick your preferred variation and I'm open for any further suggestions, then I'll update the patch respectively.
Attachment #707989 - Attachment is obsolete: true
(In reply to Philip Chee from comment #18)
> [FYI: Bug 765398 - Implement three-state UI for DNT (do not track me, okay
> to track me, decline to state).]

That should be relevant for the expanded Help documentation, but shouldn't affect the checkbox label or short description in any way (they are generic enough).

This needs to be revisited if and when Toolkit introduces that change.
[oops, that landed already - bummer... thus I'll have to consider that now.]
I guess this needs to land for SM 2.18 so that the user-tracking functionality works properly. I'll try to copy-paste from the Firefox version to match their 3-radiobutton layout (which is a bit tricky as it combines two preferences now, i.e., I'll have to keep pref-security.js but replace its contents entirely with those new functions), that's hopefully sufficiently straight-forward to do.
Summary: Expose Phishing Protection preferences in the Preferences UI → Modify Privacy & Security Preferences UI to reflect recent backend additions
Don't feel you need to copy the Firefox UI; I'd be quite happy with a double checkbox approach where one disables the other.
I'll have to figure out first what these prefs are actually doing... it may be the easiest to just copy the Firefox code if it works out of the box, unless you would prefer a 2-checkbox version over the 3 radiobuttons.
Ok, so I have extracted the two functions for getTrackingPrefs() and setTrackingPrefs() and included that radiogroup. I'm wondering why the Firefox version wraps these functions into

  var gPrivacyPane = { ... };

which in turn isn't done for updateClearNowButtonLabel() and clearPrivateDataNow() in the existing code on SeaMonkey's side. Unless necessary for scoping reasons, I would simply make those get/set*() regular functions.

Another scoping oddity is that

  <prefpane ... script="chrome://communicator/content/pref/pref-security.js">

doesn't work in this case, the functions won't be found for onsync{to,from}preference. It only works when using instead

    <script type="application/javascript" 
            src="chrome://communicator/content/pref/pref-security.js"/>

before the groupbox. Aside from that, I think I've got it working.
(In reply to neil@parkwaycc.co.uk from comment #23)
> Don't feel you need to copy the Firefox UI; I'd be quite happy with a double
> checkbox approach where one disables the other.

The Firefox-style version is now:

  ( ) Don't track me
  ( ) Do track me
  ( ) Don't say anything

where the ".enabled" (bool) pref states whether or not a header is sent at all (thus, if false, that corresponds to "Don't say anything"), and ".value" (int) says whether the header should state "Don't track me" or "Do track me". A direct implementation of those prefs would look like this:

  [ ] Send the server my tracking preferences:
    ( ) Don't track me
    ( ) Do track me

which takes up the same vertical space and may be less intuitive (and some logic is required there as well to enable/disable the radiogroup based on the checkbox value).

Thus, I'll stick with the Firefox version unless there are any issues with it.
(In reply to rsx11m from comment #25)
> Ok, so I have extracted the two functions for getTrackingPrefs() and
> setTrackingPrefs() and included that radiogroup. I'm wondering why the
> Firefox version wraps these functions into
> 
>   var gPrivacyPane = { ... };
> 
> which in turn isn't done for updateClearNowButtonLabel() and
> clearPrivateDataNow() in the existing code on SeaMonkey's side. Unless
> necessary for scoping reasons, I would simply make those get/set*() regular
> functions.
> 
> Another scoping oddity is that
> 
>   <prefpane ... script="chrome://communicator/content/pref/pref-security.js">
> 
> doesn't work in this case, the functions won't be found for
> onsync{to,from}preference. It only works when using instead
> 
>     <script type="application/javascript" 
>             src="chrome://communicator/content/pref/pref-security.js"/>
> 
> before the groupbox. Aside from that, I think I've got it working.
This is because <prefpane ... script> automatically scopes the script to the prefpane element, and the code that handles onsync*preference can't handle that, so we have to use a slightly cumbersome approach to accessing the script methods (I'm sure you can find one in one of our other pref panes as an example).

(In reply to rsx11m from comment #26)
> (In reply to comment #23)
> > Don't feel you need to copy the Firefox UI; I'd be quite happy with a double
> > checkbox approach where one disables the other.
> 
> The Firefox-style version is now:
> 
>   ( ) Don't track me
>   ( ) Do track me
>   ( ) Don't say anything
> 
> where the ".enabled" (bool) pref states whether or not a header is sent at
> all (thus, if false, that corresponds to "Don't say anything"), and ".value"
> (int) says whether the header should state "Don't track me" or "Do track
> me". A direct implementation of those prefs would look like this:
> 
>   [ ] Send the server my tracking preferences:
>     ( ) Don't track me
>     ( ) Do track me
> 
> which takes up the same vertical space and may be less intuitive
Seems as least as intiutive as "Don't say anything" to me.

> (and some logic is required there as well to
> enable/disable the radiogroup based on the checkbox value).
Hardly any logic is needed. I tried looking in pref-keynav but it's using outdated code; pref-findasyoutype has a good example though.
(In reply to neil@parkwaycc.co.uk from comment #27)
> Seems as least as intiutive as "Don't say anything" to me.

That weren't the labels Firefox used, of course, just their implications.
Here their definitions:

>  <!ENTITY dntTrackingNopref.label        "Do not tell sites anything about my tracking preferences.">
>  <!ENTITY dntTrackingNopref.accesskey    "o">
>  <!ENTITY dntTrackingNotOkay.label       "Tell sites that I do not want to be tracked.">
>  <!ENTITY dntTrackingNotOkay.accesskey   "n">
>  <!ENTITY dntTrackingOkay.label          "Tell sites that I want to be tracked.">
>  <!ENTITY dntTrackingOkay.accesskey      "t">


> Hardly any logic is needed. I tried looking in pref-keynav but it's using
> outdated code; pref-findasyoutype has a good example though.

I'll look into that (it appears to me that you'd prefer the box+radio version?).
(In reply to neil@parkwaycc.co.uk from comment #27)
> Hardly any logic is needed. I tried looking in pref-keynav but it's using
> outdated code; pref-findasyoutype has a good example though.

Thanks for the pointer. Yes, the findasyoutype version looks trivial and should be directly applicable to what's needed here. So, let's go with that one with similar labels as quoted in comment #28. This would also avoid the scoping issue.
(In reply to rsx11m from comment #28)
> (it appears to me that you'd prefer the box+radio version?).
Yes please, I don't think the extra complexity of the three-way radio is worth it.

Mostly based on previous comments, the best labels I have come up with so far are as follows:

[X] Tell sites about my tracking preference
    ( ) I do not want to be tracked
    (o) I consent to being tracked
I like the "consent" in that label, this should make people think twice before checking that box. The default is "I do not want to be tracked" for this pref.
Attached patch Proposed patch (v2) (obsolete) — Splinter Review
Issues addressed:
  (comment #10) got rid of redundant empty lines, boxes, and flexes
  (comment #14) added 1-line introductory descriptions
  (comment #14) renamed "Phishing Protection" to "Safe Browsing"
  (comment #16) changed "privdata" to "privatedata" in all instances
  (comment #19) new accesskeys defined per 1-line suggestions
  (comment #30) tracking prefs are now a checkbox + dependent radiogroup

This patch uses "hg cp" again to create the new pref-privatedata.* files from the existing pref-security.* ones, this seemed to have worked.
Attachment #707695 - Attachment is obsolete: true
Attachment #707701 - Attachment is obsolete: true
Attachment #707695 - Flags: ui-review?(neil)
Attachment #707695 - Flags: review?(iann_bugzilla)
Attachment #708580 - Flags: ui-review?(neil)
Attachment #708580 - Flags: review?(iann_bugzilla)
Attached image Screenshots (v2)
Left side: main Privacy & Security pane; not checked (top), checked (bottom).
Right side: new Private Data pane (no changes vs. the initial patch).
Attachment #707704 - Attachment is obsolete: true
Attachment #708106 - Attachment is obsolete: true
Comment on attachment 708580 [details] [diff] [review]
Proposed patch (v2)

>+        <radio id="dntNoTrack" value="1" label="&dntTrackingNotOkay.label;"
>+               accesskey="&dntTrackingNotOkay.accesskey;" />
>+        <radio id="dntDoTrack" value="0" label="&dntTrackingOkay.label;"
>+               accesskey="&dntTrackingOkay.accesskey;" />
Nit: no space before />

>+<!ENTITY blockWebForgeries.label          "Block reported web forgeries (Phishing)">
>+<!ENTITY blockWebForgeries.accesskey      "P">
>+
Nit: blank line at end of file
Attachment #708580 - Flags: ui-review?(neil) → ui-review+
Attached patch Proposed patch (v2b) (obsolete) — Splinter Review
(In reply to neil@parkwaycc.co.uk from comment #34)
> Nit: no space before />

Apparently I've copy-pasted the Firefox code a bit too literally here...

> Nit: blank line at end of file

Taken care of, thanks!

Ian, it's all yours now.
Attachment #708580 - Attachment is obsolete: true
Attachment #708580 - Flags: review?(iann_bugzilla)
Attachment #708794 - Flags: ui-review+
Attachment #708794 - Flags: review?(iann_bugzilla)
Attached patch Proposed patch (v2c) (obsolete) — Splinter Review
Tiny follow-up fix: "websites" is used rather than "web sites" in the labels for the other preference panes, thus removed the spaces in pref-security.dtd.

Carrying over ui-r assuming that this change is ok, otherwise let me know.
Attachment #708794 - Attachment is obsolete: true
Attachment #708794 - Flags: review?(iann_bugzilla)
Attachment #709054 - Flags: ui-review+
Attachment #709054 - Flags: review?(iann_bugzilla)
(In reply to rsx11m from comment #36)
> Tiny follow-up fix: "websites" is used rather than "web sites" in the labels
> for the other preference panes, thus removed the spaces in pref-security.dtd.
> 
> Carrying over ui-r assuming that this change is ok, otherwise let me know.

Indeed, if memory serves me correctly the en-GB localisation uses "web sites" which is probably why I didn't notice ;-)
Comment on attachment 709054 [details] [diff] [review]
Proposed patch (v2c)

>+++ b/suite/common/pref/pref-privatedata.xul
>+  <prefpane id="privatedata_pane" label="&pref.privatedata.title;"
>+            script="chrome://communicator/content/tasksOverlay.js chrome://communicator/content/pref/pref-privatedata.js">
You do not need to load tasksOverlay.js here anymore as it is now loaded for all prefpanes in preferences.xul when bug 588419 landed.

r=me with that fixed.
Attachment #709054 - Flags: review?(iann_bugzilla) → review+
(In reply to Ian Neal from comment #38)
> You do not need to load tasksOverlay.js here anymore as it is now loaded for
> all prefpanes in preferences.xul when bug 588419 landed.

Correct, it works fine with that removed in pref-privatedata.xul. I even changed the respective comment in line #27 of preferences.xul, but obviously didn't quite get its implications at that time.
Attachment #709054 - Attachment is obsolete: true
Attachment #709466 - Flags: ui-review+
Attachment #709466 - Flags: review+
Push for trunk/comm-central please.
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
Comment on attachment 709466 [details] [diff] [review]
Final patch [ui-r=Neil r=IanN check-in comment 41]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/2482caee1e4f
Attachment #709466 - Attachment description: Final patch → Final patch ui-r=Neil r=IanN check-in comment 41]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → seamonkey2.18
Attachment #709466 - Attachment description: Final patch ui-r=Neil r=IanN check-in comment 41] → Final patch [ui-r=Neil r=IanN check-in comment 41]
Depends on: 939481
You need to log in before you can comment on or make changes to this bug.