578724 - Add in image blocking while on a domain

Status: RESOLVED INACTIVE

(Core :: Graphics: Image Blocking, defect)

Description

[Mehdi Mulani [:mmm] (I don't check this)]

Currently image blocking disallows images from a certain domain. However, without the use of add-ons (such as Adblock Plus), there is no way to block images from loading while visiting pages from a certain domain.

Not sure on how this behavior should be exposed through permissions or if it would override the current mechanism.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why do we want this?  We're slowly removing image blocking (see Bug 514739).

Comment 2

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #1)
> Why do we want this?  We're slowly removing image blocking (see Bug 514739).

This feature is slated to be in Site Preferences (bug 573176).
From bug 514739, it seems that only the context menu option is to be removed, while the feature is still accessible from Page Info -> Permissions or Preferences -> Content.

Comment 3

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Initial patch that adds the desired functionality with some caveats.

This patch tests permissions for content but without taking into account what type of permission it is testing (i.e. it doesn't distinguish between an images test and a geolocation test).

It is a bit inefficient in that each item M frames/iframes deep will take O(M) tests.

Comment 4

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #3)
> This patch tests permissions for content but without taking into account what
> type of permission it is testing (i.e. it doesn't distinguish between an images
> test and a geolocation test).

When I wrote "tests permissions" I meant changes the behaviour to check permissions, I did not mean that this patch adds to the test suite (it does not!).

I realized a problem with the previous patch, it short circuits on the decision that a content should not be loaded when it should be short circuiting on any decision from a set permission.

Not sure how the content tree should be enumerated, root node first or lowest child first.

Comment 5

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Changes testing method to be more efficient and different shortcircuiting condition.

Condition shortcircuits based on finding a decision from a permission.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So I'm trying to understand the logic here.  We're first checking whether the original site is allowed to load the image.  Then we're checking whether the original site is allowed to load all its ancestor frames?  If not, are we changing the meaning of preferences that our users already have set?  And if that's the case, what's the migration story?

Apart from that logic, this line

  item->GetSameTypeParent(getter_AddRefs(item));

is wrong because the order of evaluation is not defined in C++.  With some compilers this will reliably crash with a null-pointer dereference, with some it will sometimes work and sometimes not, and with some it will always work..

Comment 7

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Updated patch per bz's comments.

As for the migration story: we are changing the current meaning of image permissions but still implementing the old functionality. In this way, we do not expect users to be confused by the new behaviour, especially when it shows up in Site Preferences.

So for I have consulted Boriss and dwitte about this behaviour and both seemed to think that it will not surprise many.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> we are changing the current meaning of image permissions but still implementing
> the old functionality

You're changing things so that the same preferences will now block a different set of images (a superset of the old set), no?

Comment 9

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #8)
> You're changing things so that the same preferences will now block a different
> set of images (a superset of the old set), no?

Yup, a superset of the old set.

To better explain:
Suppose a permission is set to block images on example.com. While surfing on example.com, no images will be shown. Suppose also that example.net uses images from example.com and example.net, then while surfing example.net only some images will be blocked (the ones from example.com).
The latter is the previous behaviour while the former is the new addition.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Right.  And I'm just saying that this is a weird behavior change that doesn't necessarily make sense.  It'd confuse the heck out of me (and as a side-bonus it leads to _really_ confusing looking code, since the |currentURI| in your new TestPermission calls is completely irrelevant, right?)

Comment 11

[Shawn Wilsher :sdwilsh]

I think that from a programming perspective, this might seem a bit off, but for the user, I think it makes a lot of sense.  If I'm on facebook.com, and I block images, I don't want to also have to block it's CDN, and any other random site that has images on it.  Not that I've ever blocked images on a site before though...

Perhaps we need user experience input here (although, I'm pretty sure that that is why Mehdi filed this bug in the first place)?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I have no problems with the proposed behavior.  My issue is with overloading existing preferences that users already have set to mean something new.  If we want to add a new permission for the new behavior and have "block images" set it from now on, I'm all on board.

And again, that would make the code actually comprehensible...

Comment 13

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #12)
> I have no problems with the proposed behavior.  My issue is with overloading
> existing preferences that users already have set to mean something new.  If we
> want to add a new permission for the new behavior and have "block images" set
> it from now on, I'm all on board.
> 
> And again, that would make the code actually comprehensible...

The overloading part is quite sucky. We've already removed the ability to right-click images and select "Block images from ..." but domains can be disabled in the Page Info dialog.
Transitioning users to the new behaviour is a bit odd as a new permission would mean two types of image blocking. Possibly we could copy how cookie permissions work and have three options.

As for the code, it is true that |currentURI| is confusing as I am using TestPermission to handle the burden of checking permissions for the domain. It seems like some of TestPermission could also be improved (i.e. the domain checking code moved to use extendedTLD service.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

> as a new permission would mean two types of image blocking

Yes, but we have that already with this patch...  And a UI that exposes them both at once in a single option.  Is that the intent?

Comment 15

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #14)
> > as a new permission would mean two types of image blocking
> 
> Yes, but we have that already with this patch...  And a UI that exposes them
> both at once in a single option.  Is that the intent?

The intent with this changed behavior is really to simplify rather than handle an additional case.  As Shawn pointed out in Comment 11, we're trying to bring site-preferences to the level of the site itself the way users identify sites.  Currently, even the language we use regarding image blocking is inaccurate.  We say "You can specify which web sites are allowed to load images," but in fact web sites are not the distinction which determines if an image can load.  Its domain is.  Mehdi's proposed functionality changes image blocking so that it is consistent no only with the language we are using, but with a user's reasonable expectation: blocking for a "web site" rather than merely a domain.

It is unfortunate that this changes behavior for those who have already set their preference, as bz correctly points out, but I think this is preferable to leaving this preference with behavior that only those who are familiar with it would expect.

Comment 16

[Shawn Wilsher :sdwilsh]

Note that comment 12 says that we can add a new state so we can not break the old behavior too.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Right.  This isn't an either-or dichotomy.  I have no problem with the new behavior; I just have a problem with changing the behavior of existing prefs when we don't have to do anything of the sort.

Comment 18

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Part 1: Change nsContentBlocker to separately allow blocking images while on a site.

Added an extra permission value to handle this image blocking case separately. This ensures that the old behaviour is still preserved!

Next patch will be changing the Exceptions window in Preferences UI to reflect this value.

Comment 19

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Part 2: Change UI to reflect new image permission value.

Initial patch as this uses inline values instead of constants. Not sure where to put the permission constant used by image blocking.

Comment 20

[Boris Zbarsky [:bzbarsky]]

So why do you still need to pass a non-null aFirstURI to TestPermission inthe new code?  It's unused, right?  Passing null there would be a lot clearer, I think.

And please use an enum for the enumerated sources instead of #defines?

Still sorting through the rest; should be done early tomorrow.

Comment 21

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #20)
> So why do you still need to pass a non-null aFirstURI to TestPermission inthe
> new code?  It's unused, right?  Passing null there would be a lot clearer, I
> think.
> 
> And please use an enum for the enumerated sources instead of #defines?

Agh, forgot about aFirstURI. Will change that and switch to an enum after the full review.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Why are we using the docshell's URI (which might be about:blank or whatnot) instead of the URI from the relevant principal?

Take out the extra parens around the != test in the docshell tree loop condition.

Aren't we losing the shouldLoad value from the first TestPermission call in some cases?  Should that whole new block be conditioned on shouldLoad?

The rest looks fine.

Comment 23

[Justin Dolske [:Dolske]]

TBH, I think we should be _removing_ front end support for image blocking, not extending what's already there. It's just not a very useful feature as it currently exists, and things like AdBlock do a far better job the of the task.

As noted above, bug 514739 removed the context menu support for activating it. I'd just as soon remove the Page Info parts too, though we probably need to retain some way of indicating/disabling it (for people who have already enabled it).

I'm ambivalent about the backend support: similar reasoning as above, but maybe it makes sense to provide capabilities that match our other permissions UI for addons who want to implement image blocking thusly?

Comment 24

[Alex Limi (:limi) — Firefox UX Team]

From a user experience perspective, I'd personally also argue against image blocking in the main UI — manually managing a blocklist for images is an extreme edge case (as opposed to AdBlock, which is somewhat mainstream), and it's more likely to just lead to random user confusion of the type "where did the image go" if it was blocked accidentally or by someone else using your computer.

If you want ad or image blocking, that should be in an add-on.

Comment 25

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #23)
> TBH, I think we should be _removing_ front end support for image blocking, not
> extending what's already there. It's just not a very useful feature as it
> currently exists, and things like AdBlock do a far better job the of the task.
> 
> As noted above, bug 514739 removed the context menu support for activating it.
> I'd just as soon remove the Page Info parts too, though we probably need to
> retain some way of indicating/disabling it (for people who have already enabled
> it).
> 
> I'm ambivalent about the backend support: similar reasoning as above, but maybe
> it makes sense to provide capabilities that match our other permissions UI for
> addons who want to implement image blocking thusly?

Removing front end support seems to make sense as from what I gathered at the summit, the original intent of image blocking was to foil image tracking bugs.

Backend support would be useful and I think the most appropriate place for an interface would be the current Exceptions window in Preferences -> Content -> Load images automatically -> Exceptions. I suggest that interface as it can modify sites which could be hard to navigate to, such as ad tracking sites where the domain is difficult to remember.

The concept implemented here (having a permission check the embedded context) is a bit interesting though, maybe we should expose it?

Comment 26

[Justin Dolske [:Dolske]]

Comment on attachment 464639 [details] [diff] [review]
Part 2: Change UI to reflect new image permission value.

feedback- per comment 23.

Comment 27

[:Margaret Leibovic]

This doesn't block bug 573176 anymore because we didn't include the image blocking permission in the about:permissions interface. We can file a bug to include it if this bug ever gets worked out, but it doesn't seem like a high priority right now.

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.