Opened 9 months ago

Last modified 7 weeks ago

#6504 reviewing change

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.

Change History (14)

comment:1 Changed 9 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 9 months ago by mjethani

  • Cc kzar hfiguiere sergz sebastian added

comment:3 Changed 9 months ago by mjethani

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

comment:4 Changed 9 months ago by mjethani

  • Status changed from new to reviewing

comment:5 Changed 9 months ago by abpbot

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

comment:6 Changed 9 months ago by mjethani

  • Review URL(s) modified (diff)

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

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

comment:9 Changed 4 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 4 months ago by mjethani

  • Review URL(s) modified (diff)

comment:11 Changed 4 months ago by abpbot

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

comment:12 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:13 Changed 2 months ago by abpbot

comment:14 Changed 7 weeks 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

Note: See TracTickets for help on using tickets.