360288 - SeaMonkey Front End Support for New Remote Content Policy

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Scott MacGregor]

This is the seamonkey version of Bug 357321. 

I've done the address book changes for both seamonkey and thunderbird already, but we'll need seamonkey specific changes for:

* Modifying the remote content bar to show the link for allowing remote content from this sender. 
* Removing the remote content address book white listing pref from the options UI. 

I took the liberty of checking in the new strings on the branch before seamonkey's string freeze. Thunderbird's string freeze is still a month away.

Comment 1

[Scott MacGregor]

dependency got lost

Comment 2

[Ian Neal]

Attachment: Port TB's front end support for new remote content policy v0.1

This patch:
* Drops the addressbook / whitelist code from the Message Display pref pane
* Makes the necessary changes to the mailWindowOverlay code
* Updates help to reflect above changes
* Updates help to include what "Allow remote images in HTML mail" is used for

Comment 3

[[e-mail]]

Comment on attachment 246556 [details] [diff] [review]
Port TB's front end support for new remote content policy v0.1

>-function LoadMsgWithRemoteContent()
>+/**
>+ * LoadMsgWithRemoteContent
>+ *   Reload the current message, allowing remote content
>+ */
>+function loadMsgWithRemoteContent()
Why the L/l change? (and note that the comment still has an L...)

>+function msgHdrForCurrentMessage()
>+{
>+  var msgURI = GetLoadedMessage();
>+  return (msgURI && !(/type=application\/x-message-display/.test(msgURI))) ? messenger.msgHdrFromURI(msgURI) : null;
>+}
Can you find out why this needs to be different from a) just calling msgHdrFromURI or b) calling gDBView.hdrForFirstSelectedMessage instead?

>+  numAddresses = headerParser.parseHeadersWithArray(msgHdr.author, addresses, names, fullNames);
>+  var authorEmailAddress = addresses.value[0];
Why is this different to the code that displays the email address?

>+  // reload the message now that we've updated the remote content policy for the sender  
>+  MsgReload();
What happens if the user cancels the new address card, or adds the card but changes his mind about allowing remote content?

>+      <description id="allowRemoteContentForAuthorDesc" class="text-link" flex="1" 
>+                   onclick="allowRemoteContentForSender();"></description>
Ideally text-link is for labels only. Also use <label ... />.

Comment 4

[Ian Neal]

Attachment: Revised Port of TB's front end v0.1a

Changes since v0.1:
* Fixed setting of AllowRemoteContent checkbox in abCardOverlay.js
* Fixed various DOS line endings in abCardOverlay.js
* Make abCardOverlay.js return current setting of allowRemoteContent
* Make use of above value to determine whether need to reload message

Note: Did not change from <description> to <label> as using <label> left artifacts after clicking on it.

Comment 5

[[e-mail]]

Comment on attachment 246581 [details] [diff] [review]
Revised Port of TB's front end v0.1a

>+      gEditCard.card.allowRemoteContent =
>+        window.arguments[0].allowRemoteContent == 'true';
>+      window.arguments[0].allowRemoteContent = 'false';
Why are these strings? Oh wait, this is shared with Thunderbird? That sucks.

>+/**
>+ * LoadMsgWithRemoteContent
>+ *   Reload the current message, allowing remote content
>+ */
> function LoadMsgWithRemoteContent()
I've got no problem with you changing this to a lower case l to match Thunderbird but the comment should match ;-)

>+  var cardForEmailAddress;
>+  var addrbook;
>+  while (!cardForEmailAddress && enumerator.hasMoreElements())
>+  {
>+    addrbook = enumerator.getNext();
>+    if (addrbook instanceof Components.interfaces.nsIAbMDBDirectory)
>+      cardForEmailAddress = addrbook.cardForEmailAddress(authorEmailAddress);
>+  }
>+
>+  if (cardForEmailAddress)
>+  {
>+    // set the property for remote content
>+    cardForEmailAddress.allowRemoteContent = true;
>+    cardForEmailAddress.editCardToDatabase(""); 
>+  }
>+  else
>+  {
>+    var args = {primaryEmail:authorEmailAddress, displayName:names.value[0],
>+                allowRemoteContent:'true'};
>+    // create a new card and set the property
>+    window.openDialog("chrome://messenger/content/addressbook/abNewCardDialog.xul",
>+                      "", "chrome,resizable=no,titlebar,modal,centerscreen", args);
>+    cardForEmailAddress = args.allowRemoteContent == 'true';
>+  } 
>+
>+  // reload the message if we've updated the remote content policy for the sender  
>+  if (cardForEmailAddress)
>+    MsgReload();
>+}
This is cheating... in one case it's a card, in the other it's true/false.

>+      <description id="allowRemoteContentForAuthorDesc" class="text-link" flex="1"
>+                   onclick="allowRemoteContentForSender();"/>
I thought that by using a label it would make it tabbable so that you could activate it from the keyboard, so hopefully the artefact you're seeing is a focus outline!

Comment 6

[Ian Neal]

Attachment: Boolean rather than string patch v0.1b

Changes since v0.1a:
* Changed to using boolean for allowRemoteContent - abCardOverlay.js is forked from TB
* Changed from <description> to <label>
* Stop cheating and use new variable for showing whether allowRemoteContent is set or not.

Comment 7

[Scott MacGregor]

I'll try to synch up the Thunderbird changes with the good review comments from Neil if someone doesn't beat me to it.

Comment 8

[[e-mail]]

Comment on attachment 246674 [details] [diff] [review]
Boolean rather than string patch v0.1b

>+      window.arguments[0].allowRemoteContent = gEditCard.card.allowRemoteContent;
File/New Card opens the dialog without any arguments, so you need to allow for that (OnLoadNewCard already does this). r=me with that fixed.

Comment 9

[Ian Neal]

Attachment: Test for arguments patch v0.1c (Checked into trunk & 1.8.1 branch)

Changes since v0.1b:
* Adds check for arguments in window before trying to set .allowRemoteContent

Carrying forward r= and requesting sr=

Comment 10

[Ian Neal]

Comment on attachment 246731 [details] [diff] [review]
Test for arguments patch v0.1c (Checked into trunk & 1.8.1 branch)

we need this for SM1.1 as the backend changes went into the branch in bug 357321. The string changes, outside of help, do not matter if they don't get picked up by l10n.

Comment 11

[Robert Kaiser]

Comment on attachment 246731 [details] [diff] [review]
Test for arguments patch v0.1c (Checked into trunk & 1.8.1 branch)

a=me for SeaMonkey 1.1 - please add the late-l10n keyword and notify m.d.l10n on checkin. Even if the string changes are minor, localizers should know about them.

Comment 12

[Ian Neal]

Comment on attachment 246731 [details] [diff] [review]
Test for arguments patch v0.1c (Checked into trunk & 1.8.1 branch)

Checking in (trunk)
mailnews/addrbook/resources/content/abCardOverlay.js;
new revision: 1.64; previous revision: 1.63
mailnews/base/prefs/resources/content/pref-viewing_messages.xul;
new revision: 1.52; previous revision: 1.51
mailnews/base/prefs/resources/locale/en-US/pref-viewing_messages.dtd;
new revision: 1.28; previous revision: 1.27
mailnews/base/resources/content/mailWindowOverlay.js;
new revision: 1.247; previous revision: 1.246
mailnews/base/resources/content/mailWindowOverlay.xul;
new revision: 1.322; previous revision: 1.321
mailnews/base/resources/locale/en-US/messenger.properties;
new revision: 1.137; previous revision: 1.136
suite/locales/en-US/chrome/common/help/mail_help.xhtml;
new revision: 1.79; previous revision: 1.78
themes/classic/messenger/primaryToolbar.css;
new revision: 1.14; previous revision: 1.13
themes/modern/messenger/primaryToolbar.css;
new revision: 1.18; previous revision: 1.17
done

Comment 13

[Ian Neal]

Comment on attachment 246731 [details] [diff] [review]
Test for arguments patch v0.1c (Checked into trunk & 1.8.1 branch)

Checking in (1.8.1 branch)
extensions/help/resources/locale/en-US/mail_help.xhtml;
new revision: 1.62.2.15; previous revision: 1.62.2.14
mailnews/addrbook/resources/content/abCardOverlay.js;
new revision: 1.55.2.7; previous revision: 1.55.2.6
mailnews/base/prefs/resources/content/pref-viewing_messages.xul;
new revision: 1.47.6.5; previous revision: 1.47.6.4
mailnews/base/prefs/resources/locale/en-US/pref-viewing_messages.dtd;
new revision: 1.23.6.5; previous revision: 1.23.6.4
mailnews/base/resources/content/mailWindowOverlay.js;
new revision: 1.222.2.18; previous revision: 1.222.2.17
mailnews/base/resources/content/mailWindowOverlay.xul;
new revision: 1.298.2.20; previous revision: 1.298.2.19
mailnews/base/resources/locale/en-US/messenger.properties;
new revision: 1.122.2.10; previous revision: 1.122.2.9
themes/classic/messenger/primaryToolbar.css;
new revision: 1.12.2.1; previous revision: 1.12
themes/modern/messenger/primaryToolbar.css;
new revision: 1.17.2.1; previous revision: 1.17
done