Issue 6619 - Qualify CSS selectors in document style sheet correctly

[Manish Jethani, 2018-04-24 19:15:42]

Patch Set 1

See test/common.js first.

[Manish Jethani, 2018-04-24 19:56:34]

Oh shoot, this won't work for filters like the following from EasyList:

    webfail.com###aside > [-abp-properties='cursor: pointer;']

We'll have to be a little more clever about this. I think we should just discard
the rest of the original selector and just keep the last part. e.g. if the style
sheets contains a rule like "body #foo { cursor: pointer }" would should just
inject "aside > #foo { display: none !important }". Or perhaps we should try to
make sure we're matching both selectors somehow.

[hub, 2018-04-26 03:31:16]

Also to would be nice to have a concrete test in elemHideEmulation with DOM that
would trigger the bug.

https://codereview.adblockplus.org/29760699/diff/29760700/test/common.js
File test/common.js (right):

https://codereview.adblockplus.org/29760699/diff/29760700/test/common.js#newc...
test/common.js:41: test.equal(qualifySelector("div.bar", "#foo"),
"div#foo.bar");
I'm not sure I understand why the order is different between the two cases.

[Manish Jethani, 2018-05-10 02:20:10]

On 2018/04/24 19:56:34, Manish Jethani wrote:
> Oh shoot, this won't work for filters like the following from EasyList:
> 
>     webfail.com###aside > [-abp-properties='cursor: pointer;']
> 
> We'll have to be a little more clever about this. I think we should just
discard
> the rest of the original selector and just keep the last part. e.g. if the
style
> sheets contains a rule like "body #foo { cursor: pointer }" would should just
> inject "aside > #foo { display: none !important }". Or perhaps we should try
to
> make sure we're matching both selectors somehow.

On second thoughts, this is still better than what we have now. If you have a
selector in the document like "body #foo" and we try to qualify it with "aside >
.bar", with this code we get "body aside > .bar#foo", which is still correct and
would work as far as hiding the element is concerned.

If the selector in the document is "body > #foo" though, then the result here
would be "body > aside > .bar#foo", which will not match the intended element. I
haven't thought about what to do about this yet, but anyway it's still has the
same result as "aside > .barbody > #foo" (which is what we get now). Overall
this patch only improves the situation without breaking anything that already
works.

[Manish Jethani, 2018-05-10 02:20:44]

On 2018/04/26 03:31:16, hub wrote:
> Also to would be nice to have a concrete test in elemHideEmulation with DOM
that
> would trigger the bug.

I'll try to do that.

https://codereview.adblockplus.org/29760699/diff/29760700/test/common.js
File test/common.js (right):

https://codereview.adblockplus.org/29760699/diff/29760700/test/common.js#newc...
test/common.js:41: test.equal(qualifySelector("div.bar", "#foo"),
"div#foo.bar");
On 2018/04/26 03:31:16, hub wrote:
> I'm not sure I understand why the order is different between the two cases.

The order is different because the code doesn't care about the order between IDs
and classes (since it doesn't matter), it just looks for the first place where
it can insert the qualifier that results in a valid CSS selector.

[hub, 2018-05-14 19:35:13]

A few comments. Otherwise, this is looking good.

https://codereview.adblockplus.org/29760699/diff/29760700/lib/common.js
File lib/common.js (right):

https://codereview.adblockplus.org/29760699/diff/29760700/lib/common.js#newco...
lib/common.js:175: function qualifySelector(selector, qualifier)
It would be nice to have a bit of documentation to explain what this function
does.

https://codereview.adblockplus.org/29760699/diff/29760700/test/common.js
File test/common.js (right):

https://codereview.adblockplus.org/29760699/diff/29760700/test/common.js#newc...
test/common.js:41: test.equal(qualifySelector("div.bar", "#foo"),
"div#foo.bar");
On 2018/05/10 02:20:44, Manish Jethani wrote:
> On 2018/04/26 03:31:16, hub wrote:
> > I'm not sure I understand why the order is different between the two cases.
> 
> The order is different because the code doesn't care about the order between
IDs
> and classes (since it doesn't matter), it just looks for the first place where
> it can insert the qualifier that results in a valid CSS selector.

Acknowledged.

[Manish Jethani, 2018-05-15 12:03:58]

Patch Set 2: Rebase
Patch Set 3: Add JSDoc for qualifySelector

https://codereview.adblockplus.org/29760699/diff/29760700/lib/common.js
File lib/common.js (right):

https://codereview.adblockplus.org/29760699/diff/29760700/lib/common.js#newco...
lib/common.js:175: function qualifySelector(selector, qualifier)
On 2018/05/14 19:35:12, hub wrote:
> It would be nice to have a bit of documentation to explain what this function
> does.

Done.

[hub, 2018-05-15 17:05:50]

test/common.js seems to be gone in this revision of the patch.

[Manish Jethani, 2018-05-15 17:19:52]

Patch Set 4: Reinclude test/common.js

Sorry about that.

[hub, 2018-05-15 17:39:33]

LGTM

Dave, any comments ?

[Manish Jethani, 2018-05-29 00:24:25]

On 2018/05/15 17:39:33, hub wrote:
> LGTM

Thanks!

> Dave, any comments ?

Moved Dave to reviewers.