#5773 closed defect (fixed)

Element hiding emulation filters do not work in old Chrome/Opera versions

Reported by: Ross Assignee: hfiguiere
Priority: Unknown Milestone: Adblock-Plus-3.0-for-Firefox
Module: Core Keywords:
Cc: sebastian, hfiguiere, kzar, trev Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29555834
https://codereview.adblockplus.org/29557738/
https://codereview.adblockplus.org/29557741/

Description (last modified by Ross)

Environment

ABP 1.13.3.1838
Chrome 49 / 50 / Windows 7
Opera 36 / 37 / Windows 7
Does not occur on Chrome 51+
Does not occur on Opera 38+

How to reproduce

  1. Visit https://adblockplus.org
  2. Add the filter to ABP: adblockplus.org#?#:-abp-properties(width: 76px)
  3. Refresh the page.

Observed behaviour

Filter is not applied (ABP logo is not hidden) in the versions noted above.

Expected behaviour

Filters to work on min supported versions or min supported version number to be raised?

Change History (23)

comment:1 Changed 13 months ago by Ross

  • Description modified (diff)

comment:2 Changed 13 months ago by hfiguiere

Any error in the console?

comment:3 Changed 13 months ago by hfiguiere

  • Cc hfiguiere added

comment:4 Changed 13 months ago by Ross

No errors in the console.

comment:5 Changed 13 months ago by hfiguiere

  • Owner set to hfiguiere

comment:6 Changed 13 months ago by hfiguiere

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

comment:7 Changed 13 months ago by abpbot

comment:8 Changed 13 months ago by hfiguiere

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

comment:9 Changed 13 months ago by sebastian

  • Summary changed from Element hiding filters do not work in min supported Chrome/Opera versions. to Element hiding emulation filters do not work in old Chrome/Opera versions

comment:10 follow-up: Changed 13 months ago by kzar

  • Cc kzar trev added

Wouldn't it have made more sense to add another workaround to give CSSRuleList iterator support adblockpluschrome/ext/common.js like we did for HTMLCollection and NodeList?

Also could you copy me in on adblockpluscore issues + reviews? This hasn't even been triaged yet but the change already got pushed :/.

comment:11 Changed 13 months ago by trev

Seeing that I'm running Chrome 61 already, I'd rather just drop support for Chrome versions before 51. This should make this work-around and the other ones unnecessary.

comment:12 follow-up: Changed 13 months ago by kzar

Sounds good to me, according to Wikipedia version 51 is already over a year old.

comment:13 in reply to: ↑ 10 Changed 13 months ago by hfiguiere

Replying to kzar:

Wouldn't it have made more sense to add another workaround to give CSSRuleList iterator support adblockpluschrome/ext/common.js like we did for HTMLCollection and NodeList?

Also could you copy me in on adblockpluscore issues + reviews? This hasn't even been triaged yet but the change already got pushed :/.

See issue #5381

I was unaware of a workaround in a different module for these.

comment:14 in reply to: ↑ 12 Changed 13 months ago by hfiguiere

Replying to kzar:

Sounds good to me, according to Wikipedia version 51 is already over a year old.

Given how impossible Google made to download these old versions, I'd be in favor too.

comment:15 Changed 13 months ago by kzar

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:16 Changed 13 months ago by kzar

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

Marking this as rejected since we're instead going to drop support for older versions of Chrome #5795. Hubert please could you revert your earlier fix?

comment:17 Changed 13 months ago by kzar

  • Resolution rejected deleted
  • Status changed from closed to reopened

Reopening for now since Sebastian questioned whether we should drop older version of Chrome after all. I still think that commit should be reverted though, since IMO it would make more sense to add the workaround to adblockpluschrome/ext/common.js if it's needed.

comment:18 Changed 13 months ago by sebastian

Yeah, if we keep supporting Chrome 49+, and until we made a decision in #5795, we do, it seems to make sense (if just for consistency) to remove these workarounds from adblockbpluscore and add another polyfill here instead.

comment:19 Changed 13 months ago by hfiguiere

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

comment:20 Changed 13 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:21 Changed 13 months ago by abpbot

A commit referencing this issue has landed:
Issue 5773 - use ES6 for stylesheets and cssRules.

comment:22 Changed 13 months ago by abpbot

A commit referencing this issue has landed:
Issue 5773 - Polyfill iterator for CSSRuleList and StyleSheetList

comment:23 Changed 13 months ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.