Issue 5090 - Use user stylesheets for element hiding if possible

background.js

Index: background.js
diff --git a/background.js b/background.js
index cb7a861be117f6a390b74ffa1370bb9de50ba33f..0d67a098747746acd0ca6c7d6da2441fdd36635c 100644
--- a/background.js
+++ b/background.js
@@ -21,9 +21,12 @@ const {RegExpFilter} = require("filterClasses");
 const {ElemHide} = require("elemHide");
 const {checkWhitelisted} = require("whitelisting");
 const {extractHostFromFrame} = require("url");
+const {hideElements} = require("css");
 const {port} = require("messaging");
 const devtools = require("devtools");
 
+let tryInsertCSS = typeof browser != "undefined";
+
 port.on("get-selectors", (msg, sender) =>
 {
   let selectors;
@@ -45,7 +48,35 @@ port.on("get-selectors", (msg, sender) =>
     selectors = [];
   }
 
-  return {selectors, trace};
+  if (!tryInsertCSS)

[Sebastian Noack, 2017/04/12 12:21:18]

Chrome and Firefox should use the same code path. The hard-coded browser check
is unnecessary. Just call chrome.tabs.insertCSS({..., cssOrigin: "user"}) and if
it fails send the selectors as response (as outlined in the issue description).

[Manish Jethani, 2017/04/12 12:32:30]

We have to remember whether it failed so we don't try again (as per the issue),
hence the extra variable tryInsertCSS.

Unfortunately this API is available in Firefox only via browser.tabs. It doesn't
work via chrome.tabs. Since we're not trying to do this on Chrome anyway, I
figured we'd check for the existence of the browser object.

Should we try to target Chrome as well so it works automatically in a future
version?

[Sebastian Noack, 2017/04/12 12:54:43]

Chrome is also going to add support for user stylesheets to
chrome.tabs.insertCSS(), though most likely with different semantics (they don't
want to add the cssOrigin parameter therefore).

Either way, as less diverge we have as better. Does using the browser namespace
requires using promises? If not, perhaps we could first implement
https://issues.adblockplus.org/ticket/5028, in order to minimize the diverge
here, and potentially in other places (in the future).

[Wladimir Palant, 2017/04/12 13:37:57]

Are you certain? I am not aware of Firefox exposing different functionality via
browser.* than chrome.*. I also checked the schema and the implementation in the
Firefox source code, I cannot see anything special about that parameter, nothing
that would disallow it via chrome.*.

[Manish Jethani, 2017/04/12 14:27:01]

My bad, it is implemented but it takes a callback instead of returning a
promise. I'll update the code so it uses chrome.tabs.insertCSS instead.

+    return {selectors, trace, inject: true};
+
+  return new Promise(resolve =>
+  {
+    hideElements(sender.page.id, sender.frame.id, selectors, error =>
+    {
+      if (error && /\bError processing cssOrigin\b/.test(error.message) != -1)
+        tryInsertCSS = false;
+
+      let response = {trace, inject: !!error};
+
+      if (trace || error)
+        response.selectors = selectors;
+
+      resolve(response);
+    });
+  });
+});
+
+port.on("hide-elements", (msg, sender) =>

[Sebastian Noack, 2017/04/12 12:21:18]

This is out of scope of what is defined in the issue. I'm not sure if the
addional complexity (and messaging overhead) is worth it, just to have
css-property filters injected via user stylesheets (where possible).

[Wladimir Palant, 2017/04/12 12:25:56]

Yes, this is an aspect we missed when describing the issue. Manish asked me
about that, and I think that we should definitely stay away from the DOM if we
can - also for the CSS property filters. This should mean slightly more complex
code but no runtime penalty otherwise - the content script already knows from
the get-selectors response whether it should message the background page.

[Sebastian Noack, 2017/04/12 12:54:43]

Keep, in mind that with -abp-selector/:has there will be no way around 
modifying the DOM, as it doesn't generate a new selector but must hide elements
directly. Will property filters be handled completely different from the new
syntax, so that we keep having a selector to inject?

+{
+  return new Promise(resolve =>
+  {
+    hideElements(sender.page.id, sender.frame.id, msg.selectors, error =>
+    {
+      resolve({success: !error});
+    });
+  });
 });
 
 port.on("forward", (msg, sender) =>