324992 - Add pref UI for mail remote content's new prefs

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, enhancement)

Description

[Ian Neal]

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

Comment 1

[Ian Neal]

Attachment: Possible pref layout

This screenshot shows a possible layout for the new prefs - any comments?

Comment 2

[Stefan [:stefanh]]

(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)"?

Comment 3

[Ian Neal]

Attachment: Revised pref layout v0.1b

Changes since first screen shot:
* Slightly tweaked wording
* Thin separator between blocking and mark as read prefs

Comment 4

[Andrew Schultz]

*** Bug 325268 has been marked as a duplicate of this bug. ***

Comment 5

[Ian Neal]

Attachment: Patch for help v0.1b

This patch:
* Changes help files to reflect updated pref UI.

Comment 6

[Ian Neal]

Attachment: Patch for revised pref layout v0.1b

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

Comment 7

[[e-mail]]

(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 8

[Stefan [:stefanh]]

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.

Comment 9

[Ian Neal]

(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 10

[Stefan [:stefanh]]

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.)"

Comment 11

[Ian Neal]

Attachment: Whitelist pref patch v0.1c

Changes since patch v0.1b:
* Uses templates / rules as per pref-addressing.xul

Comment 12

[Ian Neal]

Attachment: Revised help patch v0.1c

Changes since help patch v0.1b:
* Removed references to remote content only being in attachments
* Reworded previous changes along the lines suggested by Stefan

Comment 13

[Ian Neal]

Attachment: Another revised help patch v0.1c1

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.

Comment 14

[Stefan [: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>
>

Comment 15

[[e-mail]]

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".

Comment 16

[Ian Neal]

Attachment: Tweaked pref layout screenshot v0.1d

Changes since screenshot v0.1b:
* Address book drop down has had to be pushed down to next line to accommodate long checkbox label.

Comment 17

[Ian Neal]

Attachment: Updated whitelist pref patch v0.1d

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.

Comment 18

[Ian Neal]

Attachment: Updated help patch v0.1d

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

Comment 19

[[e-mail]]

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 [e-mail] 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.

Comment 20

[[e-mail]]

(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.

Comment 21

[Ian Neal]

Attachment: Pref lock patch v0.1d1

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=

Comment 22

[[e-mail]]

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 23

[Stefan [:stefanh]]

Comment on attachment 210694 [details] [diff] [review]
Updated help patch v0.1d

Looks fine. Assuming this reflects the final ui, of course ;)

Comment 24

[Karsten Düsterloh]

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|\/]

Comment 25

[Ian Neal]

Attachment: Tweaked pref lock patch v0.1e (Checked in trunk / branch 1.8.1)

Changes since v0.1d1:
* Removed unneeded namespace
* Fixed disabling of dropdown
* Tweaked wording of whitelist text

Comment 26

[Ian Neal]

Attachment: Tweaked help patch v0.1e

Changes since v0.1d:
* Changed text to reflect tweaked whitelist wording.

Carrying forward r=

Comment 27

[Ian Neal]

Attachment: New tweaked help patch v0.1e1 (Checked in trunk / branch 1.8.1)

Changes since help patch v0.1e:
* Changed "If you want to" -> "To" as suggested on IRC by stefanh

Carrying forward r=

Comment 28

[Ian Neal]

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

Comment 29

[Ian Neal]

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

Comment 30

[Ian Neal]

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.

Comment 31

[Ian Neal]

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 32

[Robert Kaiser]

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.

Comment 33

[Ian Neal]

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