Issue 700 - Generate filters based on the style attribute as last resort

Issue 700 - Make the "Block Element" dialog generate filters based on arbitrary attributes

[Wladimir Palant, 2014-06-24 10:46:35]

This is a footgun. The "Block Element" dialog already is dangerous as it is, the
class-based rules have quite some potential for hiding unrelated stuff. This
pushes it even further, without properly communicating it to the user and
without allowing the user to select the "important" attributes. As such, I don't
think we want to do it.

The long-term solution should be porting Element Hiding Helper to Chrome. The
assistant there allows choosing from several options and will show the effects
immediately.

[Sebastian Noack, 2014-06-24 11:00:04]

On 2014/06/24 10:46:35, Wladimir Palant wrote:
> This is a footgun. The "Block Element" dialog already is dangerous as it is,
the
> class-based rules have quite some potential for hiding unrelated stuff. This
> pushes it even further, without properly communicating it to the user and
> without allowing the user to select the "important" attributes. As such, I
don't
> think we want to do it.
> 
> The long-term solution should be porting Element Hiding Helper to Chrome. The
> assistant there allows choosing from several options and will show the effects
> immediately.

Yes, false positives are an issue here. But that is what users have to expect,
when automatically generating filters based on properties given in the DOM,
which don't necessary have to be unique. Still this is the best we can do. But I
agree that we could do better in communicating the implications. But this is a
separate issue.

However, given that we only generate the fallback filter if no other filter
could be generated, and only if attributes are given which all have to match in
addition to the tag name, I don't consider it more dangerous than the class
based filters, probably even less.

[Wladimir Palant, 2014-06-24 11:10:51]

On 2014/06/24 11:00:04, Sebastian Noack wrote:
> However, given that we only generate the fallback filter if no other filter
> could be generated, and only if attributes are given which all have to match
in
> addition to the tag name, I don't consider it more dangerous than the class
> based filters, probably even less.

I do - and I am inclined to resolve that issue as "rejected" until we can
provide a proper UI. "Implement footgun, deal with implications later" isn't
something I am willing to accept.

[Sebastian Noack, 2014-06-24 11:25:18]

On 2014/06/24 11:10:51, Wladimir Palant wrote:
> On 2014/06/24 11:00:04, Sebastian Noack wrote:
> > However, given that we only generate the fallback filter if no other filter
> > could be generated, and only if attributes are given which all have to match
> in
> > addition to the tag name, I don't consider it more dangerous than the class
> > based filters, probably even less.
> 
> I do - and I am inclined to resolve that issue as "rejected" until we can
> provide a proper UI. "Implement footgun, deal with implications later" isn't
> something I am willing to accept.

I just don't think that this is a larger "footgun" than the class based filters,
we are already generating.

While the UI we currently have here isn't as nice as Element Hiding Helper, it
still discloses the filters that have been generated.

And I think that having this is very useful for a lot of users, since we won't
port the element hiding helper to Chrome/Opera/Safari anytime soon.

[Sebastian Noack, 2014-06-24 11:33:26]

On 2014/06/24 11:25:18, Sebastian Noack wrote:
> On 2014/06/24 11:10:51, Wladimir Palant wrote:
> > On 2014/06/24 11:00:04, Sebastian Noack wrote:
> > > However, given that we only generate the fallback filter if no other
filter
> > > could be generated, and only if attributes are given which all have to
match
> > in
> > > addition to the tag name, I don't consider it more dangerous than the
class
> > > based filters, probably even less.
> > 
> > I do - and I am inclined to resolve that issue as "rejected" until we can
> > provide a proper UI. "Implement footgun, deal with implications later" isn't
> > something I am willing to accept.
> 
> I just don't think that this is a larger "footgun" than the class based
filters,
> we are already generating.
> 
> While the UI we currently have here isn't as nice as Element Hiding Helper, it
> still discloses the filters that have been generated.
> 
> And I think that having this is very useful for a lot of users, since we won't
> port the element hiding helper to Chrome/Opera/Safari anytime soon.

How about making the fallback less generic, by only using the style attribute
and requiring at least two CSS rules. That would cover almost all cases where it
works reliable. And it certainly wouldn't lead to more false positives than
class based filters.

[Wladimir Palant, 2014-06-24 11:51:34]

On 2014/06/24 11:33:26, Sebastian Noack wrote:
> How about making the fallback less generic, by only using the style attribute
> and requiring at least two CSS rules. That would cover almost all cases where
it
> works reliable.

Yes, sounds like a good compromise, agreed.

[Sebastian Noack, 2014-09-25 08:47:36]

On 2014/06/24 11:51:34, Wladimir Palant wrote:
> On 2014/06/24 11:33:26, Sebastian Noack wrote:
> > How about making the fallback less generic, by only using the style
attribute
> > and requiring at least two CSS rules. That would cover almost all cases
where
> it
> > works reliable.
> 
> Yes, sounds like a good compromise, agreed.

I've implemented that compromise, we agreed on, 3 months ago. So would you mind
giving LGTM? ;)

[Wladimir Palant, 2014-11-03 18:11:33]

LGTM

http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
include.postload.js:404: selector += "." + elt.classList[i].replace(/([^\w-])/,
"\\$1");
Somehow I feel that changing Array.map() into a loop here was a change for
change's sake...

[Thomas Greiner, 2014-11-04 09:53:21]

Just one minor issue

http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
include.postload.js:403: for (var i = 0; i < elt.classList.length; i++)
It's certainly not wrong to use elt.classList here but it's inconsistent since
we've been using elementClasses in all of the lines above.

[Sebastian Noack, 2014-11-04 14:51:07]

http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
include.postload.js:403: for (var i = 0; i < elt.classList.length; i++)
On 2014/11/04 09:53:21, Thomas Greiner wrote:
> It's certainly not wrong to use elt.classList here but it's inconsistent since
> we've been using elementClasses in all of the lines above.

The patch has already been merged, but I might file a separate issue. However,
I'm in the favor of getting rid of elementClasses (which is initialized by
splitting elt.className with two regexes) and using .classList everywhere
instead. Does anybody know why the original code did it that way in the first
place?

[Thomas Greiner, 2014-11-04 15:15:53]

http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
include.postload.js:403: for (var i = 0; i < elt.classList.length; i++)
On 2014/11/04 14:51:07, Sebastian Noack wrote:
> On 2014/11/04 09:53:21, Thomas Greiner wrote:
> > It's certainly not wrong to use elt.classList here but it's inconsistent
since
> > we've been using elementClasses in all of the lines above.
> 
> The patch has already been merged, but I might file a separate issue. However,
> I'm in the favor of getting rid of elementClasses (which is initialized by
> splitting elt.className with two regexes) and using .classList everywhere
> instead. Does anybody know why the original code did it that way in the first
> place?

That's totally fine with me. Looks like the origin of this code can be traced
back to AdThwart when there was no support for classList yet.

[Sebastian Noack, 2014-11-04 15:44:54]

http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/4829486721794048/diff/5689792285114368/incl...
include.postload.js:403: for (var i = 0; i < elt.classList.length; i++)
On 2014/11/04 15:15:53, Thomas Greiner wrote:
> On 2014/11/04 14:51:07, Sebastian Noack wrote:
> > On 2014/11/04 09:53:21, Thomas Greiner wrote:
> > > It's certainly not wrong to use elt.classList here but it's inconsistent
> since
> > > we've been using elementClasses in all of the lines above.
> > 
> > The patch has already been merged, but I might file a separate issue.
However,
> > I'm in the favor of getting rid of elementClasses (which is initialized by
> > splitting elt.className with two regexes) and using .classList everywhere
> > instead. Does anybody know why the original code did it that way in the
first
> > place?
> 
> That's totally fine with me. Looks like the origin of this code can be traced
> back to AdThwart when there was no support for classList yet.

For reference, https://issues.adblockplus.org/ticket/1522