Closed Bug 1209910 Opened 4 years ago Closed 4 years ago

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

Categories

(Thunderbird :: Message Reader UI, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 45.0

People

(Reporter: jorgk, Assigned: aceman)

References

Details

(Keywords: ux-efficiency)

Attachments

(2 files, 5 obsolete files)

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 57fzqhzthk4s020@marketplace.amazon.de, 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.
Severity: normal → enhancement
Yes, I can look at this.
Assignee: nobody → acelists
Attached patch WIP patch (obsolete) — Splinter Review
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.
Attachment #8669929 - Flags: feedback?(richard.marti)
Attachment #8669929 - Flags: feedback?(mozilla)
Attached patch Screenshot showing the WIP. (obsolete) — Splinter Review
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?
Attachment #8669929 - Flags: feedback?(mozilla)
Attached image Screenshot showing the WIP. (obsolete) —
(oops, wrong file attached, sorry)
Attachment #8670148 - Attachment is obsolete: true
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? :)
Status: NEW → ASSIGNED
Flags: needinfo?(neil)
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?
(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 :)
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
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 on attachment 8669929 [details] [diff] [review]
WIP patch

Tried the patch and with the changes discussed f+
Attachment #8669929 - Flags: feedback?(richard.marti) → feedback+
Attached patch WIP patch v2 (obsolete) — Splinter Review
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.
Attachment #8669929 - Attachment is obsolete: true
Attachment #8670149 - Attachment is obsolete: true
Attachment #8670461 - Flags: feedback?(richard.marti)
Attachment #8670461 - Flags: feedback?(mozilla)
Nice, but please, s/from/for/ !
Comment on attachment 8670461 [details] [diff] [review]
WIP patch v2

Very good!
Attachment #8670461 - Flags: feedback?(mozilla) → feedback+
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 user@example.com" 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 xx@xx.com.

"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 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.
Attachment #8670461 - Flags: feedback?(richard.marti) → feedback+
Attached patch patch v2.1 (obsolete) — Splinter Review
Cleaned up the "from" and fixed proper plural forms.
Attachment #8670461 - Attachment is obsolete: true
Attachment #8670997 - Flags: ui-review?(richard.marti)
Attachment #8670997 - Flags: review?(mkmelin+mozilla)
Attachment #8670997 - Flags: ui-review?(richard.marti) → ui-review+
"Allow remote content from all of the above domains?"
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk 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.
There is 'URL' used in the patch so I can keep that.
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.
Attachment #8670997 - Flags: review?(mkmelin+mozilla) → review+
Attached patch patch v2.2Splinter Review
Thanks, fixed.
Attachment #8670997 - Attachment is obsolete: true
Attachment #8675446 - Flags: review+
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
Adjusting summary to avoid confusion with "Show remote content in this message", which is quite different...
Summary: Remote content exceptions: Add option "Allow remote content in this message" → Remote content exceptions: Add option "Allow remote content from all N origins listed above"
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.
Blocks: 953426
Keywords: ux-efficiency
You need to log in before you can comment on or make changes to this bug.