1209910 - Remote content exceptions: Add option "Allow remote content from all N origins listed above"

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, enhancement)

Description

[Jorg K (CEST = GMT+2)]

This has been discussed widely in bug 1193200:

An option to "Allow remote content in this message" would be useful, so the users who don't want to use "Allow remote content for <e-mail>" don't have to click multiple times to allow all URLs offered in the popup.

Advantages of this option are:
- Protection against spoofed senders
- newsletter sender can change, but remote content is still retrieved.
- no obfuscated e-mail addresses as used by e.g. Amazon or eBay,
  like [e-mail], need to be added to
  the exceptions.

Of course this option would have to be used with caution, since once a tracker is allowed, it will be allowed in other messages as well.

Comment 1

[:aceman]

Yes, I can look at this.

Comment 2

[:aceman]

Attachment: WIP patch

This is a WIP patch that implements the functionality. We can now discuss where to put the item in the menu to not make it too easy for the user to click it accidentally (e.g. it could be on the bottom of the menu).

Notice, the action only allows all URLs, not the sender's email (but the info bar vanishes, which is what we want).

Also it fixes a bug that if there is no From address (may be some spam), we would display a half empty "Allow remote content from" menuitem for the email.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot showing the WIP.

Nice work.

I don't like the order of the menu items.
I'd do:
Show remote content
---
Edit options
---  <-- maybe not necessary  
Allow for e-mail
Allow for all xx listed below
Allow content from site 1
...

Richard, what do you think?

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot showing the WIP.

(oops, wrong file attached, sorry)

Comment 5

[:aceman]

Now when I look at the screenshot I notice I used "content FROM all" but the original items use "content FOR". As I think we pull the content from the URL (or server), "from" seems more appropriate. I am not sure about the "from/for email" item.

Native speakers? :)

Comment 6

[Jorg K (CEST = GMT+2)]

I'm only half native ;-) - but "for" would be more consistent, since we have
  Allow remote content for http://www.xx.com
which means
  Allow remote content for this URL http://www.xx.com

If you put
  Allow remote content from website addressed by http://www.xx.com
I'd agree with the "from".

Where it's an address, URL or e-mail, I'd stick with the "for".

How about changing the order?

Comment 7

[:aceman]

(In reply to Jorg K (GMT+2) from comment #3)
> Show remote content
> ---
> Edit options
> ---  <-- maybe not necessary  
> Allow for e-mail
> Allow for all xx listed below
> Allow content from site 1
> ...
> 
> Richard, what do you think?

I think more like:
> Show remote content
> Edit options
> ---
> Allow for e-mail
> Allow content from site 1
> ...
> ---
> Allow for all xx URLs listed above

So that if there are 50 URLs with trackers the user has to scroll over them to find the option. Maybe then he notices some junk URLs. We could also put the "allow from email" item as the last one :)

Comment 8

[Jorg K (CEST = GMT+2)]

Yes, that's cool, too. When can I have it ;-)
Note that currently we have a separator between "Show" and "Edit".

> We could also put the "allow from email" item as the last one :)
Yes!! So:

Show remote content
---
Edit options
--- 
Allow content from site 1
...
Allow for all xx listed above
Allow for e-mail

Comment 9

[Richard Marti (:Paenglab)]

Yes, placing "Allow for all xx URLs listed above" below all the URLs looks good to let the user thinking a bit what he all wants to allow. Also placing the "Allow for e-mail" at the end is good because the email address can be easily spoofed and shouldn't be the first option in the list.

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 8669929 [details] [diff] [review]
WIP patch

Tried the patch and with the changes discussed f+

Comment 11

[:aceman]

Attachment: WIP patch v2

OK, try this. It has some more fine-tuning, like if there is only one URL (excluding sender address), do not show the "allow all item". If there is no sender and only 1 URL, then the last separator can also be hidden.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: Screenshot showing the WIP 2

Nice, but please, s/from/for/ !

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8670461 [details] [diff] [review]
WIP patch v2

Very good!

Comment 14

[Jorg K (CEST = GMT+2)]

I'm confused now. On IRC first Kent convinced us that "from" is correct in all cases:
Quote: ... from, these are all variants of the same idea.

But then he changed his tune:
OK here's my recommendation, reversing what I said before. When you put "FROM [e-mail]" you have anchored the meaning of FROM to refer to an email sender. So I think you should not use the same word for the httls:// urls. Go ahead and use FOR [for the URLs].

Later he said:
I think use FOR for web urls, FROM for email addresses

and then:
the other way around works to

So my personal preference is:
Accept remote content from URL.
Accept remote content in messages from [e-mail].

"Accept remote content for URL" sounds like: "Accept remote content to be given to URL".
And an e-mail for Suzie is an e-mail sent to Suzie. So "for" expresses the wrong direction.

Comment 15

[Richard Marti (:Paenglab)]

Comment on attachment 8670461 [details] [diff] [review]
WIP patch v2

Not tested yet but looking at the screenshot from Jörg it looks good. If for or from is used I let Magnus or Neil decide.

Comment 16

[:aceman]

Attachment: patch v2.1

Cleaned up the "from" and fixed proper plural forms.

Comment 17

[[e-mail]]

"Allow remote content from all of the above domains?"

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to [e-mail] from comment #17)
> "Allow remote content from all of the above domains?"

That's not quite correct: The list will contain URLs, so protocol and domain.

Comment 19

[:aceman]

There is 'URL' used in the patch so I can keep that.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 8670997 [details] [diff] [review]
patch v2.1

Review of attachment 8670997 [details] [diff] [review]:
-----------------------------------------------------------------

Thx, looks good! r=mkmelin with some minor changes.

::: mail/base/content/mailWindowOverlay.js
@@ +3163,5 @@
>  
>    let addresses = {};
>    MailServices.headerParser.parseHeadersWithArray(
>      gMessageDisplay.displayedMessage.author, addresses, {}, {});
> +  let adrCount = (addresses.value[0] != "") ? 1 : 0;

let adrCount = addresses.value[0] ? 1 : 0;

@@ +3184,5 @@
>        childNodes[i].remove();
>    }
>  
> +  let URLSepar = document.getElementById("remoteContentAllMenuSeparator");
> +

please lowercase url: urlSepar

@@ +3206,5 @@
> +  let allURLLabel = messengerBundle.getString("remoteAllowAll");
> +  allowAllItem.label = PluralForm.get(URLcount, allURLLabel).replace("#1", URLcount);
> +
> +  allowAllItem.collapsed = (URLcount < 2);
> +  URLSepar.collapsed = allowAllItem.collapsed && (adrCount == 0);

you should collaps the other one too.
now if there's only one host there's a separator between every menu item, which doesn't look so nice. In fact it would look better without the separator after "Edit remote.... " too for that case (only have them for the multi-uri case)

@@ +3228,5 @@
> + *
> + * @param aListNode  The menulist element containing the URIs to allow.
> + */
> +function allowRemoteContentForAll(aListNode) {
> +  let URInodes = aListNode.querySelectorAll(".allow-remote-uri");

lowercase uri: uriNodes

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +341,5 @@
>  remoteContentPrefAccesskey=O
>  remoteContentPrefLabelUnix=Preferences
>  remoteContentPrefAccesskeyUnix=P
>  
> +# LOCALIZATION NOTE(remoteAllowResource): %S is host name

origin

@@ +346,5 @@
> +remoteAllowResource=Allow remote content from %S
> +# LOCALIZATION NOTE(remoteAllowAll): Semi-colon list of plural forms.
> +# See: http://developer.mozilla.org/en/Localization_and_Plurals
> +# #1 is the number of URLs
> +remoteAllowAll=Allow remote content from all #1 URL listed above;Allow remote content from all #1 URLs listed above

They aren't URLs, but origins, so let's use the correct term.

Comment 21

[:aceman]

Attachment: patch v2.2

Thanks, fixed.

Comment 22

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/b65f89541ec444346abd811fbb8f886c1d0275b4
Bug 1209910 - add "allow remote content from all URL menuitem" in the "allow remove content" menu. r=Paenglab, r=mkmelin

Comment 23

[Thomas D. (:thomas8)]

Adjusting summary to avoid confusion with "Show remote content in this message", which is quite different...

Comment 24

[Thomas D. (:thomas8)]

So this was nice and useful addition based on bug 953426, improving ux-efficiency by allowing user to add all remote origins found in a message with one click.