Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#6618 closed defect (rejected)

Element hiding emulation uses inline styles for plain selectors

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

https://codereview.adblockplus.org/29760631/

Description (last modified by mjethani)

Environment

ABP 3.0.3 on Chrome 66

How to reproduce

  1. Add the filter example.com#?#p
  2. Visit https://example.com/
  3. Open the DevTools inspector to examine the DOM

Observed behaviour

The <p> elements are hidden by setting the style="..." attribute

Expected behaviour

The <p> elements should be hidden using a CSS selector

Notes

The reason this matters is that plain selectors are not dependent on DOM mutations, so we don't process these (#6437) when there's a mutation. If we use CSS selectors in the first place there's no need to process these selectors each time there's a mutation.

Change History (8)

comment:1 Changed 20 months ago by mjethani

  • Blocking 6437 added
  • Cc kzar hfiguiere added
  • Description modified (diff)

comment:2 Changed 20 months ago by hfiguiere

No. That's the correct behaviour for a #?# filter. Simple element hiding with style sheets should be a filter of type ##.

comment:3 Changed 20 months ago by mjethani

What is the rationale for this? It seems like a no-brainer to use a CSS selector in this case instead of calling querySelectorAll each time there's a mutation.

One more thing to note is that a CSS selector would also apply to shadow DOMs whereas querySelectorAll only works on the light DOM.

comment:4 Changed 20 months ago by mjethani

Also by the way since we have switched to user style sheets this is kind of moot, we are already using CSS selectors here if tabs.removeCSS is available.

comment:5 Changed 20 months ago by hfiguiere

This was issue #5314

This was before user stylesheets in Chrome.

comment:6 Changed 20 months ago by mjethani

OK, this makes sense. Anyway if we make the change for #6610 then it's going to go the other way. I'll close this.

comment:7 Changed 20 months ago by mjethani

  • Resolution set to rejected
  • Status changed from new to closed

comment:8 Changed 20 months ago by mjethani

  • Review URL(s) modified (diff)
Note: See TracTickets for help on using tickets.