Closed Bug 324992 Opened 15 years ago Closed 15 years ago

Add pref UI for mail remote content's new prefs

Categories

(SeaMonkey :: Preferences, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

Details

(Keywords: fixed-seamonkey1.1a, fixed1.8.1)

Attachments

(3 files, 11 obsolete files)

17.83 KB, image/png
Details
6.67 KB, patch
mnyromyr
: review+
iann_bugzilla
: superreview+
Details | Diff | Splinter Review
4.75 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
The patch that landed as part of bug 294307 added some new prefs to control what remote content/images are displayed in mail messages:
disable_remote_images.useWhitelist and 
disable_remote_images.whiteListAbURI

This probably means reversing the logic of the current pref setting for blocking/displaying remote content
Attached image Possible pref layout (obsolete) —
This screenshot shows a possible layout for the new prefs - any comments?
Assignee: prefs → iann_bugzilla
Status: NEW → ASSIGNED
(In reply to comment #1)
> Created an attachment (id=210085) [edit]
> Possible pref layout
> 
> This screenshot shows a possible layout for the new prefs - any comments?
> 

Just some thoughts (mainly brain-storming here):

I guess a separate section called "Security" or something would be a bit confusing since we already have that in another place. And then it won't really fit in "Message Display"... Hmm, "Message security"?

Thinking of the text:"Block from loading and displaying remote images and other content". Perhaps something like "Block content from remote sources" (if it's in message display we already know that it's about displaying stuff in messages) or "Do not load/show remote content (images blah blah)"?

Attached image Revised pref layout v0.1b (obsolete) —
Changes since first screen shot:
* Slightly tweaked wording
* Thin separator between blocking and mark as read prefs
*** Bug 325268 has been marked as a duplicate of this bug. ***
Attached patch Patch for help v0.1b (obsolete) — Splinter Review
This patch:
* Changes help files to reflect updated pref UI.
Attachment #210281 - Flags: review?(stefanh)
This patch:
* Reverses logic of Allow remote content to Block remote content
* Adds pref for whitelist to allow remote content
* Adds drop down for choosing address book for whitelist
Attachment #210085 - Attachment is obsolete: true
Attachment #210282 - Flags: superreview?
Attachment #210282 - Flags: review?(mnyromyr)
Attachment #210282 - Flags: superreview? → superreview?(neil)
(In reply to comment #6)
>Adds drop down for choosing address book for whitelist
Can't we use a template for that, as per (say) pref-addressing.xul ?
Comment on attachment 210281 [details] [diff] [review]
Patch for help v0.1b

>Index: mail_help.xhtml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/help/resources/locale/en-US/mail_help.xhtml,v
>retrieving revision 1.66
>diff -p -u -d -8 -r1.66 mail_help.xhtml
>--- mail_help.xhtml	9 Dec 2005 00:39:45 -0000	1.66
>+++ mail_help.xhtml	31 Jan 2006 22:35:15 -0000
>@@ -547,18 +547,20 @@
>   embedded in web pages sent as message attachments:</p>
> 
> <ol>
>   <li>Open the <span class="mac">&brandShortName;</span>
>     <span class="noMac">Edit</span> menu and choose Preferences.</li>
>   <li>Under the Mail &amp; Newsgroups category, click Message Display. (If
>     no subcategories are visible, double-click Mail &amp; Newsgroups to
>     expand the list.)</li>
>-  <li>Uncheck <q>Allow messages to load and display remote images and other
>-    content</q>.</li>
>+  <li>Check <q>Block images and other content from remote sources</q>.</li>
>+  <li>If you want to still allow remote images and other remote content from
>+    senders in one of your address books then check <q>Allow if the sender is
>+    in my:</q> and select the correct address book from the drop down.</li>
>   <li>Click OK to have your change take affect.</li>
> </ol>
> 
> <p>By default, JavaScript is not enabled and plug-ins are enabled for mail
>   messages you receive. To change these settings:</p>
> 
> <ol>
>   <li>Open the <span class="mac">&brandShortName;</span>
>@@ -4350,20 +4352,24 @@ to filter unwanted mail.</p>
>     subcategories are visible, double-click Mail &amp; Newsgroups to expand
>     the list.)</li>
> </ol>
> 
> <ul>
>   <li><strong>When opening messages, display them in</strong>: Here you can
>     choose if you want to reuse a message window for the next mail or if you
>     want to open a new one for each.</li>
>-  <li><strong>Allow messages to load and display remote images and other
>-    content</strong>: Deselect this checkbox if you want to avoid downloading
>-    images and other content embedded in web pages sent as message attachments.
>-    (This checkbox is selected by default.)</li>
>+  <li><strong>Block images and other content from remote sourcest</strong>:

Sources :)

>+    Select this checkbox if you want to avoid downloading images and other
>+    content embedded in web pages sent as message attachments. (This checkbox
>+    is selected by default.)</li>

When I read this I get the impression that the pref only apply to messages that has an attachment. Lets say, for example, that I recieve a HTML message that has an image with a remote src - wouldn't that be blocked as well? In that case we colud re-phrase the "web pages sent as message attachment" (in various places) to something simpler.
(In reply to comment #8)
> When I read this I get the impression that the pref only apply to messages that
> has an attachment. Lets say, for example, that I recieve a HTML message that
> has an image with a remote src - wouldn't that be blocked as well? In that case
> we colud re-phrase the "web pages sent as message attachment" (in various
> places) to something simpler.
> 
Yes I was thinking that too, but was feeling too lazy to rewrite all those bits as well as that would probably mean spinning the help off into a separate patch. What do u think?
Comment on attachment 210281 [details] [diff] [review]
Patch for help v0.1b

Well, you could file a separate bug if you want. Otherwise, I have some suggestions - won't take you long to polish them ;)

First, we need to change a few more lines:

545 <p>By default, remote images and other remote content are displayed in
546   messages you receive. To avoid downloading images and other content
547   embedded in web pages sent as message attachments:</p>

This could be changed to something like:

"<p>By default, content that is hosted remotedly (like certain images), will not display in messages you recieve. To change this preference:</p>"


>Index: mail_help.xhtml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/help/resources/locale/en-US/mail_help.xhtml,v
>retrieving revision 1.66
>diff -p -u -d -8 -r1.66 mail_help.xhtml
>--- mail_help.xhtml	9 Dec 2005 00:39:45 -0000	1.66
>+++ mail_help.xhtml	31 Jan 2006 22:35:15 -0000
>@@ -547,18 +547,20 @@
>   embedded in web pages sent as message attachments:</p>
> 
> <ol>
>   <li>Open the <span class="mac">&brandShortName;</span>
>     <span class="noMac">Edit</span> menu and choose Preferences.</li>
>   <li>Under the Mail &amp; Newsgroups category, click Message Display. (If
>     no subcategories are visible, double-click Mail &amp; Newsgroups to
>     expand the list.)</li>
>-  <li>Uncheck <q>Allow messages to load and display remote images and other
>-    content</q>.</li>
>+  <li>Check <q>Block images and other content from remote sources</q>.</li>
>+  <li>If you want to still allow remote images and other remote content from
>+    senders in one of your address books then check


"If you still want messages from senders in one of your address books to display remote images and other content, check"



 <q>Allow if the sender is
>+    in my:</q> and select the correct address book from the drop down.</li>
>   <li>Click OK to have your change take affect.</li>
> </ol>
> 
> <p>By default, JavaScript is not enabled and plug-ins are enabled for mail
>   messages you receive. To change these settings:</p>
> 
> <ol>
>   <li>Open the <span class="mac">&brandShortName;</span>
>@@ -4350,20 +4352,24 @@ to filter unwanted mail.</p>
>     subcategories are visible, double-click Mail &amp; Newsgroups to expand
>     the list.)</li>
> </ol>
> 
> <ul>
>   <li><strong>When opening messages, display them in</strong>: Here you can
>     choose if you want to reuse a message window for the next mail or if you
>     want to open a new one for each.</li>
>-  <li><strong>Allow messages to load and display remote images and other
>-    content</strong>: Deselect this checkbox if you want to avoid downloading
>-    images and other content embedded in web pages sent as message attachments.
>-    (This checkbox is selected by default.)</li>
>+  <li><strong>Block images and other content from remote sourcest</strong>:
>+    Select this checkbox if you want to avoid downloading images and other
>+    content embedded in web pages sent as message attachments. (This checkbox
>+    is selected by default.)</li>

"Select this checkbox if you don't want to load images and other remote content in recieved messages. (This checkbox is selected by default.)"


>+  <li><strong>Allow if the sender is in my</strong>: Select this checkbox if
>+    you want to allow messages from those senders in the chosen address book to
>+    display images and other content embedded in web pages sent as message
>+    attachments. (This checkbox is selected by default.)</li>

"Select this checkbox to allow remote content in messages from senders in the chosen address book. (This checkbox is selected by default.)"
Attachment #210282 - Flags: superreview?(neil)
Attachment #210282 - Flags: review?(mnyromyr)
Attached patch Whitelist pref patch v0.1c (obsolete) — Splinter Review
Changes since patch v0.1b:
* Uses templates / rules as per pref-addressing.xul
Attachment #210282 - Attachment is obsolete: true
Attachment #210519 - Flags: superreview?(neil)
Attachment #210519 - Flags: review?(mnyromyr)
Attachment #210281 - Flags: review?(stefanh)
Attached patch Revised help patch v0.1c (obsolete) — Splinter Review
Changes since help patch v0.1b:
* Removed references to remote content only being in attachments
* Reworded previous changes along the lines suggested by Stefan
Attachment #210281 - Attachment is obsolete: true
Attachment #210521 - Flags: review?(stefanh)
Attachment #210521 - Flags: review?(stefanh)
Changes from help patch v0.1c:
* Changed first part to make more sense, might still need more work.
* Added corrections to default state for plug-ins in mailnews.
Attachment #210521 - Attachment is obsolete: true
Attachment #210536 - Flags: review?(stefanh)
Comment on attachment 210536 [details] [diff] [review]
Another revised help patch v0.1c1

>Index: mail_help.xhtml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/help/resources/locale/en-US/mail_help.xhtml,v
>retrieving revision 1.66
>diff -p -u -d -8 -r1.66 mail_help.xhtml
>--- mail_help.xhtml	9 Dec 2005 00:39:45 -0000	1.66
>+++ mail_help.xhtml	2 Feb 2006 23:32:45 -0000
>@@ -537,44 +537,49 @@
>   <li>Click Print.</li>
> </ul>
> 
> <p>[<a href="#reading_messages">Return to beginning of section</a>]</p>
> 
> <h2 id="controlling_images_scripts_and_plug-ins">Controlling Images, Scripts,
>   and Plug-ins</h2>
> 
>-<p>By default, remote images and other remote content are displayed in
>-  messages you receive. To avoid downloading images and other content
>-  embedded in web pages sent as message attachments:</p>
>+<p>By default, images and other content, that is hosted remotely, will not
>+  display in messages you receive, except from senders in your Personal
>+  Address Book. To change these settings, alter one of the following:</p>

This is fine, but just end the last sentence with "To change these settings:" (you'll see what I mean further down).
> 
> <ol>
>   <li>Open the <span class="mac">&brandShortName;</span>
>     <span class="noMac">Edit</span> menu and choose Preferences.</li>
>   <li>Under the Mail &amp; Newsgroups category, click Message Display. (If
>     no subcategories are visible, double-click Mail &amp; Newsgroups to
>     expand the list.)</li>

Start a new <li> here and say something like "In the General section, do one of the following:"

Wrap the 3 <li>:s below in an unordered list:

>-  <li>Uncheck <q>Allow messages to load and display remote images and other
>-    content</q>.</li>
>+  <li>If you want to allow all remote content, uncheck <q>Block images and
>+    other content from remote sources</q>.</li>
>+  <li>If you want to block all remote content, uncheck <q>Allow if the sender
>+    is in my:</q>.</li>
>+  <li>If you want to change which address book you use for people who send
>+    messages that are allowed to display remote content then select the

(Not sure, but shouldn't there be a "," between "content" and "then"?)

>+    correct address book from the drop down.</li>

End the unordered list here (and the "main" <li>, of course)

>   <li>Click OK to have your change take affect.</li>
> </ol>
> 
>-<p>By default, JavaScript is not enabled and plug-ins are enabled for mail
>+<p>By default, JavaScript is not enabled and plug-ins are not enabled for mail

Just make that "By default, JavaScript and plug-ins are not enabled for mail"


>   messages you receive. To change these settings:</p>
> 
> <ol>
>   <li>Open the <span class="mac">&brandShortName;</span>
>     <span class="noMac">Edit</span> menu and choose Preferences.</li>
>   <li>Under the Advanced category, click Scripts &amp; Plug-ins. (If no
>     subcategories are visible, double-click Advanced to expand the
>     list.)</li>

Here you can remove the ending </li> above and start an unordered list:

>   <li>Under <q>Enable JavaScript for</q>, check <q>Mail &amp; Newsgroups</q> to
>     enable JavaScript for web pages viewed in mail messages.</li>
>-  <li>Under <q>Enable Plug-ins for</q>, uncheck <q>Mail &amp; Newsgroups</q> to
>-    disable plug-ins.</li>
>+  <li>Under <q>Enable Plug-ins for</q>, check <q>Mail &amp; Newsgroups</q> to
>+    enable plug-ins.</li>

And then you end the unordered list here

>   <li>Click OK to have your changes take affect.</li>
> </ol>
>
Attachment #210536 - Flags: review?(stefanh) → review-
Comment on attachment 210519 [details] [diff] [review]
Whitelist pref patch v0.1c

Just another glance through (review rsn):

>+      if (disabled) {
>+        checkbox.setAttribute("disabled", "true");
>+      } else {
>+        checkbox.removeAttribute("disabled");
>+      }

>+      if (aChecked) {
>+        document.getElementById("whiteListAbURI").removeAttribute("disabled");
>+      } else {
>+        document.getElementById("whiteListAbURI").setAttribute("disabled", "true");
>+      }
Does .disabled not work on these widgets?

>+<!ENTITY useWhiteList.label               "Allow if the sender is in my:">
I believe other panels (e.g. JMC) include the words "address book".
Changes since screenshot v0.1b:
* Address book drop down has had to be pushed down to next line to accommodate long checkbox label.
Attachment #210191 - Attachment is obsolete: true
Changes since v0.1c:
* Used .disabled for checkboxes
* Added "address book" to whilelist checkbox label and so had to push dropdown onto next line to make room.
Attachment #210519 - Attachment is obsolete: true
Attachment #210693 - Flags: superreview?(neil)
Attachment #210693 - Flags: review?(mnyromyr)
Attachment #210519 - Flags: superreview?(neil)
Attachment #210519 - Flags: review?(mnyromyr)
Attached patch Updated help patch v0.1d (obsolete) — Splinter Review
Changes since v0.1c1:
* Made corrections as suggested by reviewer
* Added "address book" to whitelist checkbox help text
* Removed another reference to web page attachments in mail in "Enable Javascript" help
Attachment #210536 - Attachment is obsolete: true
Attachment #210694 - Flags: review?(stefanh)
Comment on attachment 210693 [details] [diff] [review]
Updated whitelist pref patch v0.1d

Note: I'm going to nit stuff you may have copied from other broken files, but at least we have a chance to fix it here ;-)

>+    function enableWhiteList(aChecked) {
>+      var checkbox = document.getElementById("useWhiteList");
>+      var disabled = isDisabled(aChecked, checkbox);
>+      checkbox.disabled = disabled;
>+      enableWLPopup(!disabled && checkbox.checked);
>+    }
>+
>+    function enableWLPopup(aChecked) {
>+      document.getElementById("whiteListAbURI").disabled = !aChecked;
>+    }
This doesn't seem to respect pref locking on the popup's preference correctly? sr=me if you fix this or explain how your way works.

>+      <hbox class="indent" align="center">
Don't need this, just add class="indent" to th menulist.

>+          <menupopup id="whitelist-menupopup" ref="moz-abdirectory://"
Unused id.

>+                     datasources="rdf:addressdirectory"
>+                     sortActive="true" sortDirection="ascending"
>+                     sortResource="http://home.netscape.com/NC-rdf#DirTreeNameSort">
>+            <template>
Actually my preference is for the RDF stuff to go on the menulist, then the menupopup goes inside the rule (you only get one popup because the menuitem has the uri).

>+                <menuitem uri="..."
I prefer uri="rdf:*" - I asked waterson@netscape.com about it aeons ago and he said it made no difference but the rdf:* is a nicer style.

>+<!ENTITY disableContent.label             "Block images and other content from remote sources">
>+<!ENTITY disableContent.accesskey         "B">
>+<!ENTITY useWhiteList.label               "Allow if the sender is in my address book:">
>+<!ENTITY useWhiteList.accesskey           "A">
I was trying to think of alternative ways of expressing this, such as "Allow messages to download images and other content" and "But only if the sender is in my address book:" but then I haven't seen TB's equivalent options.
Attachment #210693 - Flags: superreview?(neil) → superreview+
(In reply to comment #17)
>Added "address book" to whilelist checkbox label and so had to push dropdown
>onto next line to make room.
Which is probably a good thing, given that the label is likely to be longer in other locales.
Attached patch Pref lock patch v0.1d1 (obsolete) — Splinter Review
Changes since v0.1d:
* Locked prefs honoured for all new pref strings.
* rdf moved to menulist as suggested and other rdf suggestions done.

Carrying forward sr=
Attachment #210693 - Attachment is obsolete: true
Attachment #210716 - Flags: superreview+
Attachment #210716 - Flags: review?(mnyromyr)
Attachment #210693 - Flags: review?(mnyromyr)
Comment on attachment 210716 [details] [diff] [review]
Pref lock patch v0.1d1

>+    function enableWLPopup(aChecked) {
>+      enableCheckbox(aChecked, document.getElementById("whiteListAbURI"));
>+    }
Maybe enableCheckbox is not such a good name after all ;-) But don't change anything yet, Mnyromyr hates getting multiple requests for revisions of patches he hasn't seen :-)
Comment on attachment 210694 [details] [diff] [review]
Updated help patch v0.1d

Looks fine. Assuming this reflects the final ui, of course ;)
Attachment #210694 - Flags: review?(stefanh) → review+
Comment on attachment 210716 [details] [diff] [review]
Pref lock patch v0.1d1

First of all, when [x] blocking remote content while not [_] allowing senders from an address book, the address book dropdownbox is enabled when entering the panel; thus minussing.

>Index: content/pref-viewing_messages.xul
>===================================================================
>+      xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"

What's that for? You don't use this namespace.

>+                  datasources="rdf:addressdirectory"

Not even here. ;-)
(Stuff inside attribute values doesn't care for namespaces.)

>Index: locale/en-US/pref-viewing_messages.dtd
>===================================================================
>+<!ENTITY disableContent.label             "Block images and other content from remote sources">
>+<!ENTITY disableContent.accesskey         "B">
>+<!ENTITY useWhiteList.label               "Allow if the sender is in my address book:">

useWhiteList.label plus the following dropdownbox is quite confusing, imo: allow what? The PAB?

How about this:

[x] Block images and other content from remote sources
    [x] but show it if the sender is in this address book:
        [Personal Address Book|\/]
Attachment #210716 - Flags: review?(mnyromyr) → review-
Changes since v0.1d1:
* Removed unneeded namespace
* Fixed disabling of dropdown
* Tweaked wording of whitelist text
Attachment #210716 - Attachment is obsolete: true
Attachment #214242 - Flags: superreview+
Attachment #214242 - Flags: review?(mnyromyr)
Attached patch Tweaked help patch v0.1e (obsolete) — Splinter Review
Changes since v0.1d:
* Changed text to reflect tweaked whitelist wording.

Carrying forward r=
Attachment #210694 - Attachment is obsolete: true
Attachment #214243 - Flags: review+
Attachment #214242 - Flags: review?(mnyromyr) → review+
Changes since help patch v0.1e:
* Changed "If you want to" -> "To" as suggested on IRC by stefanh

Carrying forward r=
Attachment #214243 - Attachment is obsolete: true
Attachment #214375 - Flags: review+
Comment on attachment 214242 [details] [diff] [review]
Tweaked pref lock patch v0.1e (Checked in trunk / branch 1.8.1)

Checking in (trunk)
mailnews/base/prefs/resources/content/pref-viewing_messages.xul;
new revision: 1.51; previous revision: 1.50
mailnews/base/prefs/resources/locale/en-US/pref-viewing_messages.dtd;
new revision: 1.27; previous revision: 1.26
done
Attachment #214242 - Attachment description: Tweaked pref lock patch v0.1e → Tweaked pref lock patch v0.1e (Checked in trunk)
Comment on attachment 214375 [details] [diff] [review]
New tweaked help patch v0.1e1 (Checked in trunk / branch 1.8.1)

Checking in (trunk)
extensions/help/resources/locale/en-US/mail_help.xhtml;
new revision: 1.68; previous revision: 1.67
done
Attachment #214375 - Attachment description: New tweaked help patch v0.1e1 → New tweaked help patch v0.1e1 (Checked in trunk)
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 214242 [details] [diff] [review]
Tweaked pref lock patch v0.1e (Checked in trunk / branch 1.8.1)

Requesting a= for SM1.1a for both this patch and the help patch once fix for bug 294307 goes into the 1.8 branch.
Attachment #214242 - Flags: approval-seamonkey1.1a?
Comment on attachment 214375 [details] [diff] [review]
New tweaked help patch v0.1e1 (Checked in trunk / branch 1.8.1)

" ... remote content, then select .. ." should be " ... remote content, select .. ."

Checking in (trunk)
mail_help.xhtml;
new revision: 1.69; previous revision: 1.68
done
Comment on attachment 214242 [details] [diff] [review]
Tweaked pref lock patch v0.1e (Checked in trunk / branch 1.8.1)

a=me for SeaMonkey 1.1

A note from the L10n side though, for future changes: Please change the entity name when you completely change the meaning of the displayed text. The reason for this is that localizers then can't just leave it as-is and display completely wrong descriptions in the UI.
Attachment #214242 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
Depends on: 292668
Comment on attachment 214242 [details] [diff] [review]
Tweaked pref lock patch v0.1e (Checked in trunk / branch 1.8.1)

Checking in (branch 1.8.1)
mailnews/base/prefs/resources/content/pref-viewing_messages.xul;
new revision: 1.47.6.4; previous revision: 1.47.6.3
mailnews/base/prefs/resources/locale/en-US/pref-viewing_messages.dtd;
new revision: 1.23.6.4; previous revision: 1.23.6.3
extensions/help/resources/locale/en-US/mail_help.xhtml;
new revision: 1.62.2.5; previous revision: 1.62.2.4
done
Attachment #214242 - Attachment description: Tweaked pref lock patch v0.1e (Checked in trunk) → Tweaked pref lock patch v0.1e (Checked in trunk / branch 1.8.1)
Attachment #214375 - Attachment description: New tweaked help patch v0.1e1 (Checked in trunk) → New tweaked help patch v0.1e1 (Checked in trunk / branch 1.8.1)
You need to log in before you can comment on or make changes to this bug.