Issue 29401596: Issue 5094 - Implement support for :has() in chrome extension (Closed)
|
Created:
April 3, 2017, 2:10 p.m. by hub Modified:
June 19, 2017, 3:09 p.m. CC:
Felix Dahlke Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 5094 - Implement support for :has() in chrome extension
Patch Set 1 #
Total comments: 10
Patch Set 2 : Properly implement the tracer. Improve the element hiding logic. #
Total comments: 8
Patch Set 3 : Changes following the feedback. #Patch Set 4 : Rebased (conflicts resolved) #
Total comments: 2
Patch Set 5 : Remove no longer needed code. #Patch Set 6 : Simplify patch to deal with latest version of the implementation #
Total comments: 1
Patch Set 7 : Updated to use the latest version of the core patch #Patch Set 8 : Rebased. With the proper revision in dependencies. #
Total comments: 4
Patch Set 9 : nits addressed #Patch Set 10 : Update dependencies to the latest. #
MessagesTotal messages: 20
you need my patch for adblockpluscore (review #29383960). The depency change point to that patch and will need to be adjusted. Is mentionned bug 5094, it is possible that the callback be unecessary (I assumed it was) as we could delegate all of this to the content script.
Thank a lot, for having a go, Hub. This will also make it much easier to review the related core changes. And it seems I found already one potential issue there (see the last comment below). https://codereview.adblockplus.org/29401596/diff/29401597/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29401596/diff/29401597/include.preload.js#... include.preload.js:549: element.style.display = "none"; We should use the same logic as we have in checkCollapse(), i.e. "display: none !important" + MutationObserver preventing the web page from changing that style. In order to avoid duplication, that logic should be moved to a separate function that is called both, here and in checkCollapse(). https://codereview.adblockplus.org/29401596/diff/29401597/include.preload.js#... include.preload.js:553: this.tracer.addSelectors(filters, filters); ElementHidingTracer.addSelectors() will search for hidden elements that match the given selectors (now and on DOM mutation) and reports the corresponding filters to the devtools panel if it found any match. This step is unnecessary in the scenario here, as the elements just got hidden there is no need to confirm that, and in fact we don't even have any selectors that we could rely on for this purpose. Instead we should add a method to ElementHidingTracer that reports a list of filters to the devtools panel no matter what, and use this method here instead. https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... File lib/filterValidation.js (right): https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... lib/filterValidation.js:147: !isValidCSSSelector(filter.selector)) When we implemented CSS property filters we deliberately decided against pseudo-class syntax, but to rather misuse attribute lookup syntax, e.g.: [-abp-properties="color: red"] That way these filters not only pass the validation here, but even more importantly, it doesn't cause any problems with older version of Adblock Plus which aren't aware of the new filter syntax. Otherwise, if we implement :has() with this pseudo-class syntax, it will essentially break element hiding on older Adblock Plus versions, as soon as filter lists use it.
updating patch. (no syntax change yet) https://codereview.adblockplus.org/29401596/diff/29401597/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29401596/diff/29401597/include.preload.js#... include.preload.js:549: element.style.display = "none"; On 2017/04/04 10:14:45, Sebastian Noack wrote: > We should use the same logic as we have in checkCollapse(), i.e. "display: none > !important" + MutationObserver preventing the web page from changing that style. > In order to avoid duplication, that logic should be moved to a separate function > that is called both, here and in checkCollapse(). Done. https://codereview.adblockplus.org/29401596/diff/29401597/include.preload.js#... include.preload.js:553: this.tracer.addSelectors(filters, filters); On 2017/04/04 10:14:45, Sebastian Noack wrote: > ElementHidingTracer.addSelectors() will search for hidden elements that match > the given selectors (now and on DOM mutation) and reports the corresponding > filters to the devtools panel if it found any match. > > This step is unnecessary in the scenario here, as the elements just got hidden > there is no need to confirm that, and in fact we don't even have any selectors > that we could rely on for this purpose. > > Instead we should add a method to ElementHidingTracer that reports a list of > filters to the devtools panel no matter what, and use this method here instead. Done. https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... File lib/filterValidation.js (right): https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... lib/filterValidation.js:147: !isValidCSSSelector(filter.selector)) On 2017/04/04 10:14:45, Sebastian Noack wrote: > When we implemented CSS property filters we deliberately decided against > pseudo-class syntax, but to rather misuse attribute lookup syntax, e.g.: > > [-abp-properties="color: red"] > > That way these filters not only pass the validation here, but even more > importantly, it doesn't cause any problems with older version of Adblock Plus > which aren't aware of the new filter syntax. Otherwise, if we implement :has() > with this pseudo-class syntax, it will essentially break element hiding on older > Adblock Plus versions, as soon as filter lists use it. ok. this will need to have changes in the adblockpluscore patch first.
https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... File lib/filterValidation.js (right): https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... lib/filterValidation.js:147: !isValidCSSSelector(filter.selector)) > ok. this will need to have changes in the adblockpluscore patch first. I suggest that we use something like: [-abp-selector=':has()'] Basically the CSS selector is the value of the attribute '-abp-selector'. This allow for a simpler syntax, easy to move forward when CSS4 is supported.
https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... File lib/filterValidation.js (right): https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... lib/filterValidation.js:147: !isValidCSSSelector(filter.selector)) On 2017/04/05 13:18:43, hub wrote: > > > ok. this will need to have changes in the adblockpluscore patch first. > > I suggest that we use something like: > > [-abp-selector=':has()'] > > Basically the CSS selector is the value of the attribute '-abp-selector'. > This allow for a simpler syntax, easy to move forward when CSS4 is supported. > This appears unnecessarily verbose (and potentially confusing) to me. Also it will be inconsistent with CSS property filters, except we change the syntax for them as well, but then we will have an unnecessary breaking change. How is this any simpler than just going with [-abp-has=""]? https://codereview.adblockplus.org/29401596/diff/29403560/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29401596/diff/29403560/include.preload.js#... include.preload.js:130: /* collapse the element and ensure it stays that way */ If you feel that this function needs to be documented (I'm personally not even sure), please use JSDoc syntax like we do in other files. https://codereview.adblockplus.org/29401596/diff/29403560/include.preload.js#... include.preload.js:131: function collapseElement(element) "element collapsing" is a special term in the context of Adblock Plus that describes when a request has been blocked, and its associated element then is also hidden in order to avoid showing placeholders which waste space. Since the code in this function is no longer limited to this use case, "hide" seems to be a better name than "collapse" here. https://codereview.adblockplus.org/29401596/diff/29403560/include.preload.js#... include.preload.js:219: hideElements(filters) The name of this function isn't optimal. It doesn't hide any elements, but reports filter matches for elements that have been hidden. Essentially it just reports filters that have been applied. So how about "reportFilters"? https://codereview.adblockplus.org/29401596/diff/29403560/include.preload.js#... include.preload.js:223: matchedSelectors.push(filter.replace(/^.*?##/, "")); This logic is redundant with code in checkNodes(). I suggest to remove the replace there, and call this function from checkNodes() when when it would message the background page, instead. https://codereview.adblockplus.org/29401596/diff/29403560/include.preload.js#... include.preload.js:225: if (document.readyState != "loading") This check seems wrong. ElementHidingTracer.addSelectors() only triggers the checks when readyState isn't loading, because the checks are performed anyway on DOMContentLoaded. But this isn't the case here. So we have to message the background page no matter what, when this function is called.
https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... File lib/filterValidation.js (right): https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... lib/filterValidation.js:147: !isValidCSSSelector(filter.selector)) On 2017/04/05 14:58:49, Sebastian Noack wrote: > On 2017/04/05 13:18:43, hub wrote: > > > > > ok. this will need to have changes in the adblockpluscore patch first. > > > > I suggest that we use something like: > > > > [-abp-selector=':has()'] > > > > Basically the CSS selector is the value of the attribute '-abp-selector'. > > This allow for a simpler syntax, easy to move forward when CSS4 is supported. > > > > This appears unnecessarily verbose (and potentially confusing) to me. Also it > will be inconsistent with CSS property filters, except we change the syntax for > them as well, but then we will have an unnecessary breaking change. How is this > any simpler than just going with [-abp-has=""]? How do you express ":has(:has(...))"? The idea was to have well formed selector as the attribute value. And when we implement :has-text(), it will be easy. It also will make the transition to CSS4 easier.
https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... File lib/filterValidation.js (right): https://codereview.adblockplus.org/29401596/diff/29401597/lib/filterValidatio... lib/filterValidation.js:147: !isValidCSSSelector(filter.selector)) On 2017/04/05 15:44:57, hub wrote: > On 2017/04/05 14:58:49, Sebastian Noack wrote: > > On 2017/04/05 13:18:43, hub wrote: > > > > > > > ok. this will need to have changes in the adblockpluscore patch first. > > > > > > I suggest that we use something like: > > > > > > [-abp-selector=':has()'] > > > > > > Basically the CSS selector is the value of the attribute '-abp-selector'. > > > This allow for a simpler syntax, easy to move forward when CSS4 is > supported. > > > > > > > This appears unnecessarily verbose (and potentially confusing) to me. Also it > > will be inconsistent with CSS property filters, except we change the syntax > for > > them as well, but then we will have an unnecessary breaking change. How is > this > > any simpler than just going with [-abp-has=""]? > > How do you express ":has(:has(...))"? > > The idea was to have well formed selector as the attribute value. > And when we implement :has-text(), it will be easy. > > It also will make the transition to CSS4 easier. I see, nesting might become ugly, mostly because of quotes as far as I understand. I guess, [-abp-selector=":has()"] is the way to go then. As for CSS property filters, we should allow [-abp-selector=":properties()"] as well then, but should also support the old syntax (i.e. [-abp-properties]) for compatibility, at least for some time. However, one thing I still wonder about, what if browsers implement :has() and/or :properties() in the future, with different semantics than we provide? Wouldn't it make sense, for this reason to prefix :has()/:properties() even if we wrap them into [-abp-selector=""]?
https://codereview.adblockplus.org/29401596/diff/29403560/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29401596/diff/29403560/include.preload.js#... include.preload.js:219: hideElements(filters) On 2017/04/05 14:58:49, Sebastian Noack wrote: > The name of this function isn't optimal. It doesn't hide any elements, but > reports filter matches for elements that have been hidden. Essentially it just > reports filters that have been applied. So how about "reportFilters"? ok https://codereview.adblockplus.org/29401596/diff/29403560/include.preload.js#... include.preload.js:223: matchedSelectors.push(filter.replace(/^.*?##/, "")); On 2017/04/05 14:58:49, Sebastian Noack wrote: > This logic is redundant with code in checkNodes(). I suggest to remove the > replace there, and call this function from checkNodes() when when it would > message the background page, instead. Acknowledged. https://codereview.adblockplus.org/29401596/diff/29403560/include.preload.js#... include.preload.js:225: if (document.readyState != "loading") On 2017/04/05 14:58:49, Sebastian Noack wrote: > This check seems wrong. ElementHidingTracer.addSelectors() only triggers the > checks when readyState isn't loading, because the checks are performed anyway on > DOMContentLoaded. But this isn't the case here. So we have to message the > background page no matter what, when this function is called. Acknowledged.
> I see, nesting might become ugly, mostly because of quotes as far as I > understand. I guess, [-abp-selector=":has()"] is the way to go then. > > As for CSS property filters, we should allow [-abp-selector=":properties()"] as > well then, but should also support the old syntax (i.e. [-abp-properties]) for > compatibility, at least for some time. > > However, one thing I still wonder about, what if browsers implement :has() > and/or :properties() in the future, with different semantics than we provide? > Wouldn't it make sense, for this reason to prefix :has()/:properties() even if > we wrap them into [-abp-selector=""]? So let's do the following: - pseudo class :-abp-properties() and :has() wrapped into -abp-selector attribute. - keep [-abp-properties=""] attribute selection for a bit. As for browser implementing it differently, I don't think it is a big issue. Since it is wrapped, we can keep handling them the same way.
https://codereview.adblockplus.org/29401596/diff/29405640/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29401596/diff/29405640/include.preload.js#... include.preload.js:220: matchedSelectors.push(filter.replace(/^.*?##/, "")); This is essentially dead code. Now after rebasing, thanks to another change that landed, no changes to ElemementHidingTracer are necessary anymore, but you can just send devtools.traceElemHide message with the list of selectors from hideElements(): function hideElements(elements, filters) { ... if (this.tracer) ext.backgroundPage.sendMessage({type: "devtools.traceElemHide", filters}); }
https://codereview.adblockplus.org/29401596/diff/29405640/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29401596/diff/29405640/include.preload.js#... include.preload.js:220: matchedSelectors.push(filter.replace(/^.*?##/, "")); On 2017/04/07 13:14:47, Sebastian Noack wrote: > This is essentially dead code. > > Now after rebasing, thanks to another change that landed, no changes to > ElemementHidingTracer are necessary anymore, but you can just send > devtools.traceElemHide message with the list of selectors from hideElements(): > > function hideElements(elements, filters) > { > ... > > if (this.tracer) > ext.backgroundPage.sendMessage({type: "devtools.traceElemHide", filters}); > } ah ok. done now.
The integration in the content script looks good to me now. Thanks. Now we just need to finish up the core changes.
(Please could you Cc me on platform issues / code reviews?) https://codereview.adblockplus.org/29401596/diff/29429613/lib/filterValidatio... File lib/filterValidation.js (right): https://codereview.adblockplus.org/29401596/diff/29429613/lib/filterValidatio... lib/filterValidation.js:22: const {Filter, InvalidFilter, ElemHideBase, ElemHideEmulationFilter} = require("filterClasses"); Nit: This line is now too long. (Did you check for ESLint failures?)
The dependencies will need to be update when the patch land on adblockpluscore. I have brought back the hideElement() callback.
You can use the patch as is. I have been for a while. Ready for review.
https://codereview.adblockplus.org/29401596/diff/29464699/dependencies File dependencies (right): https://codereview.adblockplus.org/29401596/diff/29464699/dependencies#newcode5 dependencies:5: adblockpluscore = adblockpluscore hg:d3cd9c1c85e4 git:57a2198 Quite a lot of other adblockpluscore commits were included in this dependency update, but those aren't mentioned in the issue description. Could you go through all those commits and check if any code that adblockpluschrome uses was changed? If you find any list them in the description and also add instructions explaining how to test the functionality those changes touch in the "Hints for testers" section. That way the QA guys know what to test so that we avoid regressions, since sometimes a dependency update will break something seemingly completely unrelated. (Even if you don't find any relevant files were changed by those other commits please still mention that there were other unrelated changes, and that you checked through them in the issue.) https://codereview.adblockplus.org/29401596/diff/29464699/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29401596/diff/29464699/include.preload.js#... include.preload.js:451: if (this.tracer) Nit: Since this if clause spans multiple lines please add the braces. Also did this pass ESLint?
patch updated. (also issue updated with the list of changes from core this pulls) https://codereview.adblockplus.org/29401596/diff/29464699/dependencies File dependencies (right): https://codereview.adblockplus.org/29401596/diff/29464699/dependencies#newcode5 dependencies:5: adblockpluscore = adblockpluscore hg:d3cd9c1c85e4 git:57a2198 On 2017/06/14 10:20:30, kzar wrote: > Quite a lot of other adblockpluscore commits were included in this dependency > update, but those aren't mentioned in the issue description. Could you go > through all those commits and check if any code that adblockpluschrome uses was > changed? > > If you find any list them in the description and also add instructions > explaining how to test the functionality those changes touch in the "Hints for > testers" section. That way the QA guys know what to test so that we avoid > regressions, since sometimes a dependency update will break something seemingly > completely unrelated. > > (Even if you don't find any relevant files were changed by those other commits > please still mention that there were other unrelated changes, and that you > checked through them in the issue.) good point. I have added them to the issue dscription. https://codereview.adblockplus.org/29401596/diff/29464699/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29401596/diff/29464699/include.preload.js#... include.preload.js:451: if (this.tracer) On 2017/06/14 10:20:30, kzar wrote: > Nit: Since this if clause spans multiple lines please add the braces. Also did > this pass ESLint? it did pass ESLint. Adding the braces.
patch updated to the latest dependencies.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
