Issue 4726 - Add tests for the element hiding emulation content script

test/browser/elemHideEmulation.js

Index: test/browser/elemHideEmulation.js
===================================================================
new file mode 100644
--- /dev/null
+++ b/test/browser/elemHideEmulation.js
@@ -0,0 +1,182 @@
+QUnit.module("Element hiding emulation",
+{
+  before: function()
+  {
+    // The URL object in PhantomJS 2.1.7 does not implement any properties, so
+    // we need a polyfill.
+    if (!URL || !("origin" in URL))
+    {
+      var doc = document.implementation.createHTMLDocument();
+      var anchor = doc.createElement("a");
+      doc.body.appendChild(anchor);
+      URL = function(url)
+      {
+        if (!url)
+          throw "Invalid URL";
+        anchor.href = url;
+        this.origin = anchor.origin;
+      };
+    }
+  },
+  afterEach: function()
+  {
+    var styleElements = document.head.getElementsByTagName("style");
+    for (var i = 0; i < styleElements.length; i++)

[kzar, 2017/01/09 08:28:43]

Can we not use more modern JavaScript features with PhantomJS? How about `for
(let styleElement of styleElements)`?

[Felix Dahlke, 2017/01/10 09:16:49]

Unfortunately not with version 2.1.7, that one still uses a version of WebKit
that doesn't support let (or any other ES6 features we would want to use I
think). The next release will presumably support this, then we can modernise
(probably also the element hiding emulation content script itself).

[kzar, 2017/01/10 11:05:33]

Damn, this could be a problem in the near future since we plan to use some of
these features since Chrome 49 is otherwise the lowest common denominator. I
guess we need to use babel or similar to transpile the scripts when they're run
from PhantomJS for now until the next version comes out.

[kzar, 2017/01/11 04:25:28]

You missed this one? (Some other smaller ones in the other files too.)

[Felix Dahlke, 2017/01/11 10:06:02]

No, it rather sounded like you thought this could become a problem in the future
- which it could. Right now, elemHideEmulation.js is still ES5, and will most
likely continue to be at least until :has support has landed. By that time,
PhantomJS should have made a release (they always seem to release in January). I
don't think being able to use some ES6 features a few weeks earlier is worth the
extra hassle of introducing babel here. We could create an issue for this, so we
won't forget about it. What do you think?

[kzar, 2017/01/11 11:19:18]

Please at least put a comment / note in relevant places warning that the file(s)
only supports ES5 due to PhantomJS support. (Mention the version of PhantomJS
needed for us to improve things.) We're looking to move code over to using
`let`, arrow functions etc so we need to avoid breaking these tests by mistake.

[Felix Dahlke, 2017/01/12 09:24:06]

Sure! This affects chrome/content/elemHideEmulation.js,
test/browser/elemHideEmulation.js and also lib/common.js. I will add a note to
all three files and refer to an issue I'll create about being unable to use ES5
features in some files because of the PhantomJS version we use. Only one new
file is being added here, the others were ES5 before. However, to my knowledge,
there is nothing holding us back from using selected ES6 features in these files
except for PhantomJS.

[kzar, 2017/01/12 09:42:13]

Cool, sounds good. (IMO an issue is overkill since there's a lot of other code
in adblockplus{chrome,core,ui} that I'm/we're planning to migrate over anyway,
but up to you.)

[Felix Dahlke, 2017/01/17 19:41:08]

Done.

+      document.head.removeChild(styleElements[0]);
+  }
+});
+
+QUnit.assert.hidden = function(element)

[kzar, 2017/01/09 08:28:43]

I wonder if the URL polyfill and these utility function should be ain a separate
helpers file? Sounds like they would be useful for a lot of tests.

[Felix Dahlke, 2017/01/10 09:16:49]

I personally prefer to share code once it needs to be shared, not before. So I
would rather keep them here until there is a second test that needs them.

[kzar, 2017/01/10 11:05:33]

Fair enough.

+{
+  this.equal(getComputedStyle(element).display, "none",
+             "The element's display property should be set to 'none'");
+};
+
+QUnit.assert.visible = function(element)
+{
+  this.notEqual(getComputedStyle(element).display, "none",
+                "The element's display property should not be set to 'none'");
+};
+
+function findUniqueId()
+{
+  var id = "elemHideEmulationTest-" + Math.floor(Math.random() * 10000);
+  if (!document.getElementById(id))
+    return id;
+  return findUniqueId();
+}
+
+function insertStyleRule(rule)
+{
+  var styleElement;

[kzar, 2017/01/09 08:28:43]

Nit: `let` rather than `var` throughout?

[Felix Dahlke, 2017/01/10 09:16:48]

See above.

+  var styleElements = document.head.getElementsByTagName("style");
+  if (styleElements.length)
+    styleElement = styleElements[0];

[kzar, 2017/01/09 08:28:43]

Nit: Since the else clause has braces the if clause should have them too.

[Felix Dahlke, 2017/01/10 09:16:48]

Why? I personally prefer it this way.

[kzar, 2017/01/10 11:05:33]

I've been told to do that on codereviews before by Wladimir IIRC. I kind of see
his point too, it's a little less clear otherwise. (If a bit more concise.)

[Felix Dahlke, 2017/01/10 13:27:32]

IIRC he actually prefers it that way, but since we don't have a rule about that
I would prefer to leave it as is. Doing things a certain way based on
speculation about other people's personal preferences is not productive IMO.

[kzar, 2017/01/11 04:25:29]

OK well how's this? I have been asked to use consistent braces in the past and
have come to prefer them too. Mind adding them here?

[Felix Dahlke, 2017/01/11 10:06:02]

What others have asked in other places is not particularly relevant: We have
deliberately not standarised braces around single statement bodies to leave it
up to the people working on a module/file/function to decide. We might want to
revisit that in the future if it leads to wasteful arguments, but right now all
that really counts is what YOU find more readable. Since you indicated you do
find it more readable I will add them :)

[kzar, 2017/01/11 11:19:19]

I think it is relevant what Wladimir has told me in the past since he's the
module owner and I'm just reviewing this for him since he's away.

[Felix Dahlke, 2017/01/17 19:41:07]

You're not reviewing it for him, you're reviewing it :) Anyway, done!

+  else
+  {
+    styleElement = document.createElement("style");
+    document.head.appendChild(styleElement);
+  }
+  styleElement.sheet.insertRule(rule, styleElement.sheet.cssRules.length);
+}
+
+function createElementWithStyle(styleBlock)
+{
+  var element = document.createElement("div");
+  element.id = findUniqueId();
+  document.body.appendChild(element);
+  insertStyleRule("#" + element.id + " " + styleBlock);
+  return element;
+}
+
+function applyElemHideEmulation(selectors, callback)
+{
+  var elemHideEmulation = new ElemHideEmulation(
+    window,
+    function(callback)

[kzar, 2017/01/09 08:28:43]

Nit: Mind using arrow functions here and for the other anonymous functions?

[Felix Dahlke, 2017/01/10 09:16:48]

See above.

+    {
+      var patterns = [];
+      selectors.forEach(function(selector)
+      {
+        patterns.push({selector: selector});
+      });
+      callback(patterns);
+    },
+    function(selectors)
+    {
+      if (!selectors.length)
+        return;
+      var selector = selectors.join(", ");
+      insertStyleRule(selector + "{display: none !important;}");
+    }
+  );
+
+  elemHideEmulation.load(function()
+  {
+    elemHideEmulation.apply();
+    callback();
+  });
+}
+
+QUnit.test("Verbatim property selector", function(assert)
+{
+  var done = assert.async();
+  var toHide = createElementWithStyle("{background-color: #000}");
+  applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"],

[kzar, 2017/01/09 08:28:43]

Nit: This indentation looks weird. How about something like this? (using arrow
functions whilst at it)

applyElemHideEmulation(
  "[-abp-properties='background-color: rgb(0, 0, 0)']",
  () =>
  {
    assert.hidden(toHide);
    done();
  }
);

[Felix Dahlke, 2017/01/10 09:16:49]

Yeah I know, I did some research in our code base and this seems to be the way
we normally do this. That way we don't need to indent the function body. I would
be fine with breaking the first parameter as well, so we end up with:

foo(
  [...],
  function()
{
});

I'm mostly concerned about consistency, so if we use a style like the following
elsewhere, I would be OK with going for that:

foo(
  [...],
  function()
  {
  });

Or even:

foo(
  [...],
  function()
  {
  }
);

Quite annoying edge case, really :D

[kzar, 2017/01/10 11:05:33]

Well I'm pretty sure we've done it the way I've suggested elsewhere, that's how
I would do it and how I would expect it to be done. (I don't much feel like
searching through the platform / core code for examples though.)

[Felix Dahlke, 2017/01/10 13:27:32]

Then I figure we should leave it as-is for the sake of progress. I think we
should just find a linter and configure it so that everybody is happy, then we
will have everything consistent.

[kzar, 2017/01/11 04:25:29]

Or alternatively you could just improve the indentation here for the sake of
progress? How it stands is ugly IMO and it sounds like you're not too keen on
how it looks either from your comment.

[Felix Dahlke, 2017/01/11 10:06:02]

I don't find it very beautiful, but it does look like the best option to me.
Back when I wrote this, I went to look for examples of how we generally do this.
IIRC I couldn't find any examples (much of the code in core doesn't comply with
the 80 column rule yet), so I looked at other JavaScript projects, and this
style (with the `{` on the same line of course) was pretty prominent.

Why I like it: This way the way the argument list is being wrapped does not
affect the indentation of the function body. In this case it's pretty trivial,
but if you look at the last test case below, the entire test case code is
indented by one level solely based on low long the test summary is.

I see two other options that keep the indentation of the function body stable:

1. We turn the anonymous function into a named function that is declared right
before it is being used.
2. We deliberately violate the 80 column rule by not breaking, which can be done
when adhering to it would hurt readability.

My preferred approach is what's up right now. If you would rather not have that,
I would prefer #1. If you don't like that either, #2. Indenting the function
body is something I'd like to avoid if possible - at least for the test cases
below, in this case I would be OK with it.

What do you think?

[kzar, 2017/01/11 11:19:18]

Well we "indent the function body" all the time, for example above in this very
file. By your logic shouldn't

  elemHideEmulation.load(function()
  {
    elemHideEmulation.apply();
    callback();
  });

become this?

  elemHideEmulation.load(function()
{
  elemHideEmulation.apply();
  callback();
});

I don't think I've ever see the opening bracket for a function definition
indented how you're suggesting, or at least I can't remember when I have.
Perhaps like you say it's a convention in other projects, I don't know.
Well I think this applyElemHideEmulation call should look like this:

applyElemHideEmulation(
  ["[-abp-properties='background-color: rgb(0, 0, 0)']"],
  function()
  {
    assert.hidden(toHide);
    done();
  })
);

and if you're worried about the indentation being consistent between all the
tests you can wrap the ones with shorter descriptions more eagerly than is
strictly required, no harm in that.

[Felix Dahlke, 2017/01/12 09:24:06]

No, I mean the opening brace of an anonymous function passed as an argument is
on the same indentation level as the function invocation.

My qualm with all this is this edge case:

QUnit.test("This and that works properly", function(assert)
{
   [long test body]
});

If we would just decide that the description is not elaborate enough, changing
just a single string will have us reindent the entire body:

QUnit.test("This and that works properly unless another things happens",
  function(assert)
  {
    [long test body]
  }
);

We do not have this issues with the three alternatives I offered:

A. We indent the opening brace of an anonymous function passed as an argument at
the same level as the function invocation (my preferred approach, seems most
popular in other JS projects that have a max column rule).

B. We create a named function that is then passed to the function invocation.

C. We deliberately violate the 80 column rule in these cases (that is how we
deal with this in the existing code base, since huge chunks of it still don't
comply with the 80 column rule).


 > Well I think this applyElemHideEmulation call should look like this:
For small functions such as this, I really wouldn't mind this approach. Just for
long functions do I find it problematic, it'll cause a high number of unrelated
changes if we change a single argument in a function invocation, mess up hg
blame etc.
So all test case bodies would always have to be indented one level further? That
would avoid the diff/blame issues I went into above, but I think it is a rule
that will be hard to follow consistently - it just doesn't make sense when none
of your QUnit.test invocations would actually violate the 80 column rule.

I'm fine with any of A, B or C here. I find A to be the most elegant solution.
Sure, it would be inconsistent with how we do it for small functions, but
sometimes consistency can be sacrificed for readability. What do you think?

As a last resort I could try to keep every test description below a certain
threshold to avoid this situation altogether. Not a fantastic solution, but it'd
do the trick...

[kzar, 2017/01/12 09:42:13]

I really think that you're overthinking this and should simply indent the
functions like I suggest. Diff tools can handle a bit of whitespace changing.

[Sebastian Noack, 2017/01/12 23:12:58]

I agree that putting the function header on a line with a different level of
indentation than the braces that encapsulate the function body looks quite
confusing. Neither do I recall having seen this in our code base before.

Moreover, do I also find it inconsistent and difficult to parse if some
arguments are given on the line which initiated the function call while some are
wrapped in a way that it doesn't align with the initial arguments.

As for consistency across our code base, here are two examples where we do it
like Dave and I are suggesting:
https://hg.adblockplus.org/adblockpluschrome/file/tip/chrome/devtools.js#l25
https://hg.adblockplus.org/adblockpluschrome/file/tip/composer.js#l41

I cannot see any code in this file where the addional indentation would cause
exceeding the 80 characters limit, and if there is this would be an indication
that you probably shouldn't use an anonymous function there in the first place.

[Felix Dahlke, 2017/01/17 19:41:07]

Thanks Sebastian, done.

(On a related note: I think it could be pretty helpful if we could work on some
example code that demonstrates our code wrapping style. Figuring out how to wrap
code can be a huge time sink, arguing about it even more. Ultimately it doesn't
really matter, so we might as well standarise it more clearly. Is either one of
you interested in setting that up? If not I might look into it one day.)

+    function()
+  {
+    assert.hidden(toHide);
+    done();
+  });
+});
+
+QUnit.test("Property selector with wildcard", function(assert)
+{
+  var done = assert.async();
+  var toHide = createElementWithStyle("{background-color: #000}");
+  applyElemHideEmulation(["[-abp-properties='*color: rgb(0, 0, 0)']"],
+    function()
+  {
+    assert.hidden(toHide);
+    done();
+  });
+});
+
+QUnit.test("Property selector with regular expression", function(assert)
+{
+  var done = assert.async();
+  var toHide = createElementWithStyle("{background-color: #000}");
+  applyElemHideEmulation(["[-abp-properties='/.*color: rgb\\(0, 0, 0\\)/']"],
+    function()
+  {
+    assert.hidden(toHide);
+    done();
+  });
+});
+
+QUnit.test("Property selector with regular expression containing escaped brace",
+  function(assert)
+{
+  var done = assert.async();
+  var toHide = createElementWithStyle("{background-color: #000}");
+  applyElemHideEmulation(
+    ["[-abp-properties='/background.\\x7B 0,6\\x7D : rgb\\(0, 0, 0\\)/']"],
+    function()
+  {
+    assert.hidden(toHide);
+    done();
+  });
+});
+
+QUnit.test("Property selector with regular expression containing improperly \
+escaped brace",
+  function(assert)
+{
+  var done = assert.async();
+  var toHide = createElementWithStyle("{background-color: #000}");
+  applyElemHideEmulation(
+    ["[-abp-properties='/background.\\x7B0,6\\x7D: rgb\\(0, 0, 0\\)/']"],
+    function()
+  {
+    assert.visible(toHide);
+    done();
+  });
+});
+
+QUnit.test("Property selector works for dynamically changed property",
+  function(assert)
+{
+  var done = assert.async();
+  var toHide = createElementWithStyle("{}");
+  applyElemHideEmulation(["[-abp-properties='background-color: rgb(0, 0, 0)']"],
+    function()
+  {
+    assert.visible(toHide);
+    insertStyleRule("#" + toHide.id + " {background-color: #000}");
+    setTimeout(function()
+    {
+      assert.hidden(toHide);
+      done();
+    });
+  });
+});