Opened on 04/03/2017 at 10:26:22 AM

Closed on 07/17/2017 at 03:34:11 PM

Last modified on 03/20/2018 at 04:14:23 PM

#5090 closed change (fixed)

Switch to chrome.tabs.insertCSS() on Firefox

Reported by: trev Assignee: mjethani
Priority: P2 Milestone: Adblock-Plus-1.13.4-for-Chrome-Opera
Module: Platform Keywords:
Cc: sebastian, greiner, Ross Blocked By:
Blocking: #6507 Platform: Firefox
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

Description (last modified by mjethani)


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.

Attachments (0)

Change History (30)

comment:1 Changed on 04/03/2017 at 10:46:14 AM by 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

comment:2 Changed on 04/03/2017 at 10:49:31 AM by trev

  • Cc sebastian added

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

comment:3 Changed on 04/03/2017 at 11:03:23 AM by 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?

comment:4 Changed on 04/03/2017 at 11:42:40 AM by 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.

comment:5 Changed on 04/03/2017 at 12:16:52 PM by greiner

  • Cc greiner added

comment:6 Changed on 04/03/2017 at 12:28:01 PM by sebastian

  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set

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.)

comment:7 Changed on 04/03/2017 at 12:31:37 PM by sebastian

  • Description modified (diff)

comment:8 Changed on 04/06/2017 at 04:46:19 PM by mjethani

  • Owner set to mjethani

comment:9 Changed on 04/12/2017 at 10:14:37 AM by trev

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:10 Changed on 04/12/2017 at 11:05:15 AM by mjethani

  • Review URL(s) modified (diff)

comment:11 Changed on 04/21/2017 at 01:23:10 PM by mjethani

  • Description modified (diff)

comment:12 Changed on 05/18/2017 at 02:41:21 AM by mjethani

  • Description modified (diff)

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

comment:13 Changed on 07/17/2017 at 03:32:34 PM by abpbot

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

comment:14 Changed on 07/17/2017 at 03:34:11 PM by mjethani

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:15 Changed on 07/19/2017 at 08:26:59 AM by kzar

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

comment:16 Changed on 07/19/2017 at 08:51:14 AM by 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,

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.

comment:17 Changed on 07/19/2017 at 09:32:56 AM by 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.

comment:18 Changed on 07/19/2017 at 11:35:43 AM by 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").

comment:19 Changed on 07/19/2017 at 11:40:12 AM by sebastian

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

  a, b
  c, d


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?

comment:20 Changed on 07/19/2017 at 12:59:00 PM by mjethani

Filed #5429.

comment:21 Changed on 07/24/2017 at 09:18:50 AM by abpbot

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

comment:22 Changed on 09/05/2017 at 10:18:35 PM by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next

comment:23 Changed on 09/11/2017 at 05:36:17 PM by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

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

Firefox 55 / Windows 7

comment:24 Changed on 09/11/2017 at 07:33:44 PM by sebastian

  • Cc Ross added

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.

comment:25 Changed on 09/12/2017 at 01:07:07 PM by 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.

comment:26 Changed on 09/14/2017 at 07:51:46 AM by philll

  • Verified working unset

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

comment:27 follow-up: Changed on 09/14/2017 at 11:16:41 AM by Ross

  • Verified working set

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.

Chrome 52 / 61 / Windows 7
Firefox 50/52/53/55 / Windows 7

comment:28 in reply to: ↑ 27 Changed on 09/15/2017 at 01:39:53 PM by 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).

comment:29 Changed on 09/15/2017 at 07:17:00 PM by sebastian

Yes, this would be the to be expected result.

comment:30 Changed on 03/20/2018 at 04:14:23 PM by mjethani

  • Blocking 6507 added

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from mjethani.
Note: See TracTickets for help on using tickets.