Opened 8 months ago

Closed 4 months ago

Last modified 2 months ago

#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: Platform: Firefox
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29410607/

Description (last modified by mjethani)

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.

Change History (29)

comment:1 Changed 8 months ago 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 8 months ago by trev

  • Cc sebastian added

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

comment:3 Changed 8 months ago 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 8 months ago 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 8 months ago by greiner

  • Cc greiner added

comment:6 Changed 8 months ago 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 8 months ago by sebastian

  • Description modified (diff)

comment:8 Changed 8 months ago by mjethani

  • Owner set to mjethani

comment:9 Changed 8 months ago by trev

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

comment:10 Changed 8 months ago by mjethani

  • Review URL(s) modified (diff)

comment:11 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:12 Changed 6 months ago 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 4 months ago by abpbot

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

comment:14 Changed 4 months ago by mjethani

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

comment:15 Changed 4 months ago 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 4 months ago 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,
                             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.

comment:17 Changed 4 months ago 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 4 months ago 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 4 months ago by 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?

comment:20 Changed 4 months ago by mjethani

Filed #5429.

comment:21 Changed 4 months ago by abpbot

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

comment:22 Changed 3 months ago by sebastian

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

comment:23 Changed 2 months ago 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.

ABP 2.99.0.1838beta
Firefox 55 / Windows 7

comment:24 Changed 2 months ago 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 2 months ago 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 2 months ago 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 2 months ago 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.

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

comment:28 in reply to: ↑ 27 Changed 2 months ago 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 2 months ago by sebastian

Yes, this would be the to be expected result.

Note: See TracTickets for help on using tickets.