Issue 29493648: Issue 5436 - Allow relative selectors in :-abp-has() (Closed)
|
Created:
July 20, 2017, 7:18 p.m. by hub Modified:
Aug. 18, 2017, 12:27 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 5436 - Allow relative selectors in :-abp-has()
Patch Set 1 #Patch Set 2 : Forgot a small cleanup change #
Total comments: 8
Patch Set 3 : Updated with review comment. Added other test. #Patch Set 4 : Remove unecessary changes #
Total comments: 6
Patch Set 5 : Updated with review feedback #MessagesTotal messages: 10
Currently based off the adblockpluschrome-1.13.3. This must be checked onto master when the branch patches gets grafted.
https://codereview.adblockplus.org/29493648/diff/29493652/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29493648/diff/29493652/test/browser/elemHi... test/browser/elemHideEmulation.js:395: }; This is the test that was removed as it failed.
https://codereview.adblockplus.org/29493648/diff/29493652/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29493648/diff/29493652/chrome/content/elem... chrome/content/elemHideEmulation.js:232: selector = ":scope" + selector; Nice one, I didn't know about :scope yet. Browser support is problematic however. Scoped stylesheets are being cited as the primary usage example, yet this feature appears to be dead. Chrome deprecated it (https://www.chromestatus.com/features/5374137958662144) and Edge isn't planning to implement it (https://developer.microsoft.com/en-us/microsoft-edge/platform/status/scopedst...). Now using :scoped with element.querySelector() is different and part of the DOM4 spec, so I hope that this feature won't be removed from Firefox and Chrome. Still questionable whether/when Microsoft chooses to implement it in Edge. I just checked with Sebastian and Dave on IRC about what they think about missing Edge support here. It seems that not having this work on Edge is acceptable. However, it shouldn't break things either, so you need to catch the error element.querySelector() will throw there.
https://codereview.adblockplus.org/29493648/diff/29493652/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29493648/diff/29493652/test/browser/elemHi... test/browser/elemHideEmulation.js:394: runTestPseudoClassHasSelectorWithHasAndWithSuffixSibling(test, "div:-abp-has(:-abp-has(> div.inside)) + div > div"); It's probably a good idea to test whether :scope actually has an effect rather than being treated as :root. So we should test that "div:-abp-has(> div.inside) + div > div" won't hide anything.
https://codereview.adblockplus.org/29493648/diff/29493652/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29493648/diff/29493652/chrome/content/elem... chrome/content/elemHideEmulation.js:194: const relativeSelectorRegexp = /^[\s>+~]/; \s doesn't belong in here. https://codereview.adblockplus.org/29493648/diff/29493652/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29493648/diff/29493652/test/browser/elemHi... test/browser/elemHideEmulation.js:394: runTestPseudoClassHasSelectorWithHasAndWithSuffixSibling(test, "div:-abp-has(:-abp-has(> div.inside)) + div > div"); On 2017/08/10 13:39:47, Wladimir Palant wrote: > So we should test that "div:-abp-has(> div.inside) > + div > div" won't hide anything. Sorry, that should rather be "div:-abp-has(> body div.inside) + div > div"
Updated patch. This is based on master. https://codereview.adblockplus.org/29493648/diff/29493652/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29493648/diff/29493652/chrome/content/elem... chrome/content/elemHideEmulation.js:194: const relativeSelectorRegexp = /^[\s>+~]/; On 2017/08/10 13:42:17, Wladimir Palant wrote: > \s doesn't belong in here. Done. https://codereview.adblockplus.org/29493648/diff/29493652/chrome/content/elem... chrome/content/elemHideEmulation.js:232: selector = ":scope" + selector; On 2017/08/10 13:36:08, Wladimir Palant wrote: > Nice one, I didn't know about :scope yet. Browser support is problematic > however. Scoped stylesheets are being cited as the primary usage example, yet > this feature appears to be dead. Chrome deprecated it > (https://www.chromestatus.com/features/5374137958662144) and Edge isn't planning > to implement it > (https://developer.microsoft.com/en-us/microsoft-edge/platform/status/scopedst...). > > Now using :scoped with element.querySelector() is different and part of the DOM4 > spec, so I hope that this feature won't be removed from Firefox and Chrome. > Still questionable whether/when Microsoft chooses to implement it in Edge. > > I just checked with Sebastian and Dave on IRC about what they think about > missing Edge support here. It seems that not having this work on Edge is > acceptable. However, it shouldn't break things either, so you need to catch the > error element.querySelector() will throw there. Good point. Will catch the exception. I'll see if there is a better way to do it. https://codereview.adblockplus.org/29493648/diff/29493652/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29493648/diff/29493652/test/browser/elemHi... test/browser/elemHideEmulation.js:394: runTestPseudoClassHasSelectorWithHasAndWithSuffixSibling(test, "div:-abp-has(:-abp-has(> div.inside)) + div > div"); On 2017/08/10 13:42:17, Wladimir Palant wrote: > On 2017/08/10 13:39:47, Wladimir Palant wrote: > > So we should test that "div:-abp-has(> div.inside) > > + div > div" won't hide anything. > > Sorry, that should rather be "div:-abp-has(> body div.inside) + div > div" Done.
https://codereview.adblockplus.org/29493648/diff/29515588/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29493648/diff/29515588/chrome/content/elem... chrome/content/elemHideEmulation.js:229: // :scope isn't supported on Edge. It still is not standard. What do you mean by "not standard"? It's part of the DOM4 standard which is an official recommendation. The comment is also missing the conclusion, why is it here? It should be something like ":scope isn't supported on Edge, ignore errors caused by it." And it belongs into the catch block. https://codereview.adblockplus.org/29493648/diff/29515588/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29493648/diff/29515588/test/browser/elemHi... test/browser/elemHideEmulation.js:389: elems.toHide = document.getElementById("tohide"); Please assign a proper object literal: let elems = { parent: document.getElementById(...), ... }; https://codereview.adblockplus.org/29493648/diff/29515588/test/browser/elemHi... test/browser/elemHideEmulation.js:398: return expectation(elems); Handling the expectations here for two scenarios but outside for one more is inconsistent. I think that this function should generally get an `expected` parameter like: { parent: true, middle: true, inside: true, sibling: true, sibling2: true, toHide: false }
https://codereview.adblockplus.org/29493648/diff/29515588/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29493648/diff/29515588/chrome/content/elem... chrome/content/elemHideEmulation.js:229: // :scope isn't supported on Edge. It still is not standard. On 2017/08/16 08:17:15, Wladimir Palant wrote: > What do you mean by "not standard"? It's part of the DOM4 standard which is an > official recommendation. > > The comment is also missing the conclusion, why is it here? It should be > something like ":scope isn't supported on Edge, ignore errors caused by it." And > it belongs into the catch block. Done. https://codereview.adblockplus.org/29493648/diff/29515588/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29493648/diff/29515588/test/browser/elemHi... test/browser/elemHideEmulation.js:389: elems.toHide = document.getElementById("tohide"); On 2017/08/16 08:17:16, Wladimir Palant wrote: > Please assign a proper object literal: > > let elems = { > parent: document.getElementById(...), > ... > }; Done. https://codereview.adblockplus.org/29493648/diff/29515588/test/browser/elemHi... test/browser/elemHideEmulation.js:398: return expectation(elems); On 2017/08/16 08:17:15, Wladimir Palant wrote: > Handling the expectations here for two scenarios but outside for one more is > inconsistent. I think that this function should generally get an `expected` > parameter like: > > { > parent: true, > middle: true, > inside: true, > sibling: true, > sibling2: true, > toHide: false > } Done.
LGTM 
|
