Opened 21 months ago

Closed 9 months ago

#6504 closed change (fixed)

Implement ElemHideEmulation.useInlineStyles property

Reported by: mjethani Assignee: mjethani
Priority: P3 Milestone:
Module: Core Keywords:
Cc: kzar, hfiguiere, sergz, sebastian Blocked By: #6437
Blocking: #6422 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29713583/
https://codereview.adblockplus.org/29728690/
https://codereview.adblockplus.org/29847558/

Description (last modified by mjethani)

Background

See description in #6422.

What to change

  1. In lib/content/elemHideEmulation.js, add a useInlineStyles flag, which if set to false should cause ElemHideEmulation to always use a selector rather than the style="..." property on the element
  2. Make necessary optimizations (see #6437)

Hints for testers

Only on Firefox 53+ (see #5090), element hiding emulation should use user style sheets rather than setting the style attribute on the element to be hidden. To check whether this is happening or not, open up the Inspector tab in Developer Tools, click on the element in question, and see if the style attribute has been set to display: none !important (it should not be); also see that in the styles column on the right there is a CSS rule for the element that contains display: none !important, hover over the grey text to see that it starts with data:text/css.

Make sure issues like #6458, #6619, and #6680 don't exist and that element hiding emulation works correctly in general as well as with dynamic updates to the DOM and dynamic addition of style sheets.

The change here has been reverted. As of the latest update, element hiding emulation should use the style="display: none !important" technique to hide elements, even on Firefox. Please make sure that this is the case.

Resolution

We made changes to use style sheets for element hiding emulation, and then we backtracked after realizing that tabs.removeCSS is unlikely to make it to Chromium anytime soon (Chromium bug #608854) for reasons unknown. We realize that it is not necessary to use style sheets for element hiding emulation, at least for now. We might explore this idea in the future should Chromium include support for style sheet removal.

Change History (16)

comment:1 Changed 21 months ago by mjethani

In addition to all of the general optimizations I am proposing for #6437, there is one specific optimization for user style sheets here. Once we have the list of DOM mutations, we can efficiently recompute the CSS selectors for the hidden elements without having to traverse the DOM again.

comment:2 Changed 21 months ago by mjethani

  • Cc kzar hfiguiere sergz sebastian added

comment:3 Changed 21 months ago by mjethani

  • Owner set to mjethani
  • Review URL(s) modified (diff)

comment:4 Changed 21 months ago by mjethani

  • Status changed from new to reviewing

comment:5 Changed 21 months ago by abpbot

A commit referencing this issue has landed:
Issue 6504 - Add useInlineStyles flag to ElemHideEmulation

comment:6 Changed 21 months ago by mjethani

  • Review URL(s) modified (diff)

comment:7 Changed 18 months ago by mjethani

A gentle reminder to all involved that the first patch is no good without the second patch; if we don't land the second one, we'll just have to roll back the first one when someone reports an issue with the wrong elements being hidden.

@hfiguiere has asked me to make it asynchronous so I'm going to give this one more shot.

comment:8 Changed 16 months ago by mjethani

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

comment:9 Changed 16 months ago by mjethani

There are two reasons why we still cannot use user style sheets for element hiding emulation without compromising on performance. The first one is #6446.

The second one is this comment in lib/content/elemHideEmulation.js:

// Ignore mutation targets when using style sheets, because we may have
// to update all the CSS selectors.
if (!this.useInlineStyles)
  evaluationTargets = null;

We are skipping the optimization introduced in #6437 when user style sheets are being used. Patch #29728690 would fix this, but it will take a while to land it and we have to focus on other things right now.

For now, I am going to make a change to lib/content/elemHideEmulation.js so that we never use user style sheets.

comment:10 Changed 16 months ago by mjethani

  • Review URL(s) modified (diff)

comment:11 Changed 16 months ago by abpbot

A commit referencing this issue has landed:
Issue 6504 - Remove useInlineStyles for now

comment:12 Changed 16 months ago by mjethani

  • Description modified (diff)

comment:13 Changed 15 months ago by abpbot

comment:14 Changed 14 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Element hiding emulation filters set style="display: none !important" as expected.

ABP 3.3.2.2175
Firefox 62 / 51 / Windows 10
Chrome 69 / 49 / Windows 10
Opera 56 / 36 / Windows 10

comment:15 Changed 10 months ago by abpbot

comment:16 Changed 9 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.