Switch to chrome.tabs.insertCSS() on Firefox

[trev]

Background

As of Firefox 53, chrome.tabs.insertCSS implementation on Firefox ​accepts a cssOrigin parameter which makes it usable for us. Unlike with Chrome, on Firefox we can now inject CSS code from the background process directly without messing with the DOM in the content script.

What to change

When a content script sends a get-selectors message, try injecting the stylesheet from the backround page using tabs.insertCSS({..., cssOrigin: "user}).

If the stylesheet was injected by insertCSS() and no devtools panel is active for the same tab, send following response:

{
  inject: false,
  trace: false
}

If the stylesheet was injected by insertCSS() and there is an active devtools panel for the same tab, send following response:

{
  inject: false,
  trace: true,
  selectors: [...]
}

If insertCSS() failed for any reason, set inject to true and always include the selectors, regardless whether the devtools panel is active:

{
  inject: true,
  trace: ...,
  selectors: [...]
}

If insertCSS() failed with the error message "Error processing cssOrigin", remember this and don't try to use insertCSS() again next time, instead behave exactly as if insertCSS() failed.

Adapt the content script, so that it doesn't attempt to inject a stylesheet if inject is false.

For CSS property filters, if inject is false, pass the selectors to the background page to inject the stylesheet using insertCSS(); otherwise inject the styles inline.

[sebastian]

In order to keep the devtools panel functional, we still have to return the selectors if there is an active devtools panel for this tab, but merely not inject them. Even if the devtools panel isn't supported on Firefox, Chrome is about to add support for user stylsheets to insertCSS() as well, and then we will use the same logic there (see #242). So I prefer to get this right from the start.

Currently, when there is an active devtools panel, get-selectors is responding like this:

{
  "trace": true,
  "selectors": [...]
}

So I suggest to add another key to indicate whether the selectors should be injected (regardless whether they are traced):

{
  "inject": false,
  "trace": true,
  "selectors": [...]
}

If the selectors neither need to be traced (i.e. if there is no active devtools panel) nor have to be injected by the content script (i.e. when insertCSS() is used), we can omit the the selectors from the response:

{
  "inject": false,
  "trace": false
}

[trev]

Fine with me for now, feel free to adjust the description accordingly.

[sebastian]

I will, but I have one more question. Is there also a way to detect whether insertCSS() supports user stylesheets, or do we have to check the user agent for Firefox >=53?

[trev]

I verified that calling chrome.tabs.insertCSS({code: "", dummy: "user}) will fail in both Firefox and Chrome - unknown parameters are being rejected. So I guess we could try calling chrome.tabs.insertCSS({..., cssOrigin: "user}) first and fall back to injecting selectors in the content script if it fails. It should also be possible to remember the result so that we don't keep trying - errors not related to parameter validation (such as missing permissions) are typically async.

[sebastian]

I guess, that should do for now. (We might have to revisit this though, once Chrome catches up, as they plan to use user stylesheet, without additional parameter, no matter what.)

[mjethani]

I've updated the description based on the discussion on Rietveld where we decided to use feature detection as originally planned.

[abpbot]

A commit referencing this issue has landed:
​Issue 5090 - Use user stylesheets for element hiding if possible

[kzar]

As you pointed out in IRC ESLint is now failing, I tracked it down to this change. Please could you fix?

[mjethani]

I'm getting 104 errors. I tried with the parent (changeset efc63b673869) and it still has 103 errors. It looks more like something changed in the ESList config.

They're all indentation errors.

This line in ext/background.js for example:

    updatePageFrameStructure(details.frameId, details.tabId, details.url,
                             details.parentFrameId);

Gives this error:

  279:1  error  Expected indentation of 6 spaces but found 29  indent

As per the style guide this type of indentation is totally cool, we should update the config accordingly.

[sebastian]

Yes, this kind of indentation is in compliance with our coding style. We didn't change our ESLint configuration in the past two months. So it seems that something changed in ESLint itself. According to the ​documentation we might have to use the CallExpression option now.

[mjethani]

It looks like there has been a ​major change in ​ESLint v4.0.0.

I set it to CallExpression: {arguments: "first"}} and now I'm getting 96 errors. Either we fix our indentation in a number of places (which I'm not sure is a good idea), or we set the value to "off").

[sebastian]

Yes, both of the following styles are in compliance with our coding practices:

foo(
  a, b
  c, d
);

foo(a,
    b,
    c);

And I'm not convinced that we should enforce either single one of them. So I guess setting CallExpression to off it is. Can you file a new issue for the change to ​eslint-config-eyeo?

[mjethani]

Filed #5429.

[abpbot]

A commit referencing this issue has landed:
​Issue 5090 - Adjust indentation to fix ESLint error

[Ross]

Done. I tested this by ensuring the DevTools panel works as expected and that blocking still works in general.

ABP 2.99.0.1838beta
Firefox 55 / Windows 7

[sebastian]

Please make sure to test all scenarios:

• Firefox <=52 (before cssOrigin was implemented)
• Firefox >=53 (after cssOrigin was implemented)
• Firefox Mobile >=55
• Chrome/Opera

On Chrome, Opera and before Firefox 53 this change should have no effect, still we have to make sure that this change doesn't break anything there.

Some notes on Firefox Mobile: There are some differences for extensions, so that it is important when testing a change on Firefox, to additionally test with Firefox on Android. However, note that while Adblock Plus is compatible with Firefox 50 for desktop, on Android at least Firefox 55 is required.

[mjethani]

One more tip for testing this: On Chrome, or Firefox before version 53, if you have the filter ###foo for example, when you open devtools and select the element on the page with the ID foo, you'll see the Adblock Plus CSS selectors in the style inspector on the right in the second section, right after the element's own inline styles, with possibly ::content added to the beginning of each selector (if Shadow DOM is supported). On Firefox 53+, you should see the Adblock Plus CSS selectors a little bit lower down in the style inspector in a data:text/css section, indicating that it is an injected style sheet. If you see this, it means it's working as intended. To be sure, you can add style="display: block !important" to the element: on Firefox 53+, it should have no effect, whereas on Chrome and others it'll cause the element to get displayed despite the Adblock Plus filter.

Also worth noting, when Chrome does eventually get support for user style sheets, the Adblock Plus style sheet is going to appear in the devtools style inspector as injected style sheet rather than data:text/css.

The only way to be sure that this feature is working as intended on browsers that support user style sheets is to add the inline style display: block !important to the element and check that it doesn't cause the element to get displayed despite an Adblock Plus filter that's supposed to hide the element, as explained in the last sentence of the first paragraph here.

[philll]

Unset verified working since there's no hint to the additionally requested test environments from comment 24 having been tested yet.

[Ross]

Thank you for the excellent explanation/testing notes! This works as described. In Firefox 53+ adding display: block important does not cause the element to re-display.

ABP 2.99.0.1838beta
Chrome 52 / 61 / Windows 7
Firefox 50/52/53/55 / Windows 7

[mjethani]

Replying to Ross:

Thank you for the excellent explanation/testing notes! This works as described. In Firefox 53+ adding display: block important does not cause the element to re-display.

Just to be sure, adding style="display: block !important" (note the exclamation mark) to the element should cause the element to get displayed in Firefox 52, should not in Firefox 53 and onwards (including Firefox on Android).

[sebastian]

Yes, this would be the to be expected result.