Issue 6193 - Allow element hiding emulation exceptions

lib/filterValidation.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /** @module filterValidation */
 
 "use strict";
 
-const {Filter, InvalidFilter, ElemHideBase, ElemHideEmulationFilter} =
-      require("filterClasses");
+const {Filter, InvalidFilter, ElemHideBase, ElemHideException,
+       ElemHideEmulationFilter} = require("filterClasses");
 const {Utils} = require("utils");
 
 /**
  * An error returned by
  * {@link module:filterValidation.parseFilter parseFilter()} or
  * {@link module:filterValidation.parseFilters parseFilters()}
  * indicating that a given filter cannot be parsed,
  * contains an invalid CSS selector or is a filter list header.
  *
  * @param {string} type See documentation in the constructor below.
@@ skipping 70 matching lines @@
     document.querySelector(selector);
     sheet.insertRule(selector + "{}", 0);
   }
   catch (e)
   {
     return false;
   }
   return true;
 }
 
+function isValidFilterSelector(filter)
+{
+  // Only ElemHideBase has selectors

[kzar, 2018/04/05 15:00:27]

Perhaps split these up a bit? Something like this (untested)?

if (!(filter instanceof ElemHideBase))
  return true;

// We don't yet check to see if element hiding emulation filters
// have valid selectors yet. See issue xxx.
if (filter instanceof ElemHideEmulationFilter)
  return true;

// Since the #@# syntax is used for exception to both element hiding
// and element hiding emulation filters we now need to make sure only
// the former are checked.
if ((filter instanceof ElemHideException) && filter.selector.includes(":-abp-"))
  return true;

return isValidCSSSelector(filter.selector);

[hub, 2018/04/05 17:04:41]

Done.

+  // We don't check the syntax of ElemHideEmulationFilter yet
+  if (!(filter instanceof ElemHideBase)
+      || (filter instanceof ElemHideEmulationFilter))

[kzar, 2018/04/05 15:00:27]

Nit: Parenthesis around the second clause seem redundant.
Nit: We would normally put the `||` at the end of the previous line. (Did this
pass linting?)
Nit: Since the clause spans multiple lines please use braces for the body. (Same
below.)

[hub, 2018/04/05 17:04:41]

Done.

+    return true;

[kzar, 2018/04/05 15:00:27]

Nit: I think a newline between these cases would look cleaner.

[hub, 2018/04/05 17:04:41]

Done.

+  // If it ElemHideException, don't check if it is
+  // for an EmulationFilter
+  if ((filter instanceof ElemHideException) &&
+      (filter.selector.indexOf(":-abp-") != -1))

[kzar, 2018/04/05 15:00:27]

Is this check good enough now that we support :has?

[hub, 2018/04/05 17:04:41]

This idea is that like above if we have one of the ":-abp-" pseudo selector we
don't check if this is a valid CSS selector, and assume it is fine. I don't even
know how I could let the comment above. I'll rephrase it properly.

In the future we should have a proper syntax checking all the way, but right
now, this is just to prevent the filter from being rejected when the user add
it.

[kzar, 2018/04/06 10:05:44]

Acknowledged.

+    return true;
+
+  return isValidCSSSelector(filter.selector);
+}
+
 /**
  * @typedef ParsedFilter
  * @property {?Filter} [filter]
  *   The parsed filter if it is valid. Or null if the given string is empty.
  * @property {FilterParsingError} [error]
  *   See {@link module:filterValidation~FilterParsingError FilterParsingError}
  */
 
 let parseFilter =
 /**
@@ skipping 12 matching lines @@
     if (text[0] == "[")
       return {error: new FilterParsingError("unexpected-filter-list-header")};
 
     filter = Filter.fromText(text);
 
     if (filter instanceof InvalidFilter)
     {
       return {error: new FilterParsingError("invalid-filter",
                                             {reason: filter.reason})};
     }
-    if (filter instanceof ElemHideBase &&
-        !(filter instanceof ElemHideEmulationFilter) &&
-        !isValidCSSSelector(filter.selector))
+    if (!isValidFilterSelector(filter))
     {
       return {error: new FilterParsingError("invalid-css-selector",
                                             {selector: filter.selector})};
     }
   }
 
   return {filter};
 };
 
 /**
@@ skipping 25 matching lines @@
 
     if (error)
     {
       error.lineno = i + 1;
       errors.push(error);
     }
   }
 
   return {filters, errors};
 };