Issue 29490698: Issue 5422 - Properly build props selector (Closed)
|
Created:
July 17, 2017, 5:07 p.m. by hub Modified:
Aug. 21, 2017, 7:59 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 5422 - Properly build props selector
Patch Set 1 #Patch Set 2 : Rebased on master #
Total comments: 4
Patch Set 3 : Changes following review. #
Total comments: 2
Patch Set 4 : Updated following review. #
Total comments: 2
Patch Set 5 : Another nit #MessagesTotal messages: 10
this is on top of the patches for issues 5395 and 5404 on master. I'm not sure of the dependence.
this is now on top of master.
This needs at least one unit test, one that fails without your changes. I wouldn't object to additional unit tests however, to cover the scenarios mentioned below. https://codereview.adblockplus.org/29490698/diff/29492588/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29490698/diff/29492588/chrome/content/elem... chrome/content/elemHideEmulation.js:277: prefix + " " : prefix; No, this is not what we want. Consider .abp-testsuite-fail:-abp-properties(border: 4px solid #B30B0B) for example. This should resolve into .abp-testsuite-fail.abp-testsuite-fail and hide the element. Your changes will insert a space into this selector which will no longer match anything. If a space is needed, it should be part of the filter. https://codereview.adblockplus.org/29490698/diff/29492588/chrome/content/elem... chrome/content/elemHideEmulation.js:283: subSelector = ""; What if we have a filter like `:-abp-properties(foo)` and the only matching subselector is "*"? This will produce an empty selector. Also, please consider a filter like `foo+:-abp-properties(foo)`, we'll produce an invalid selector here. Besides, this doesn't address the case where subSelector is something like "*.foo" It seems that we need to check whether subSelector starts with an asterisk. If that's the case and we have a prefix that isn't incomplete, we should strip the asterisk from subSelector. All the other changes here seem unnecessary.
https://codereview.adblockplus.org/29490698/diff/29492588/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29490698/diff/29492588/chrome/content/elem... chrome/content/elemHideEmulation.js:277: prefix + " " : prefix; On 2017/08/16 08:37:02, Wladimir Palant wrote: > No, this is not what we want. Consider > .abp-testsuite-fail:-abp-properties(border: 4px solid #B30B0B) for example. This > should resolve into .abp-testsuite-fail.abp-testsuite-fail and hide the element. > Your changes will insert a space into this selector which will no longer match > anything. If a space is needed, it should be part of the filter. Acknowledged. https://codereview.adblockplus.org/29490698/diff/29492588/chrome/content/elem... chrome/content/elemHideEmulation.js:283: subSelector = ""; On 2017/08/16 08:37:02, Wladimir Palant wrote: > What if we have a filter like `:-abp-properties(foo)` and the only matching > subselector is "*"? This will produce an empty selector. Also, please consider a > filter like `foo+:-abp-properties(foo)`, we'll produce an invalid selector here. > Besides, this doesn't address the case where subSelector is something like > "*.foo" > > It seems that we need to check whether subSelector starts with an asterisk. If > that's the case and we have a prefix that isn't incomplete, we should strip the > asterisk from subSelector. All the other changes here seem unnecessary. Done.
Unfortunately, this isn't going to be the end of it. Looking through https://www.w3.org/TR/css3-selectors/, `foo[bar]:-abp-properties(...)` will be problematic as well if the matching selector starts with a tag name. There are similar issues with pseudo-classes (you cannot really have anything follow them), but at least these are very unexpected at the end of the prefix. I guess, in the end the question is going to be whether any of this really matters in practice and is thus worth addressing. https://codereview.adblockplus.org/29490698/diff/29517757/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29490698/diff/29517757/chrome/content/elem... chrome/content/elemHideEmulation.js:284: if (!incompletePrefixRegexp.test(prefix)) Please use && operator instead of nested ifs.
there is also the case if subSelector starts with a letter it while concatenate it to the prefix. I shall update the patch with this case handled. https://codereview.adblockplus.org/29490698/diff/29517757/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29490698/diff/29517757/chrome/content/elem... chrome/content/elemHideEmulation.js:284: if (!incompletePrefixRegexp.test(prefix)) On 2017/08/17 08:53:08, Wladimir Palant wrote: > Please use && operator instead of nested ifs. Done.
https://codereview.adblockplus.org/29490698/diff/29519689/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29490698/diff/29519689/chrome/content/elem... chrome/content/elemHideEmulation.js:285: subSelector = subSelector.substr(1); Nit: we require brackets around the if body for multiline conditions.
https://codereview.adblockplus.org/29490698/diff/29519689/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29490698/diff/29519689/chrome/content/elem... chrome/content/elemHideEmulation.js:285: subSelector = subSelector.substr(1); On 2017/08/19 07:39:13, Wladimir Palant wrote: > Nit: we require brackets around the if body for multiline conditions. Done.
LGTM 
|
