Opened 9 months ago

Closed 8 months ago

Last modified 5 months ago

#7423 closed change (fixed)

Stop using the filterValidation module

Reported by: kzar Assignee: kzar
Priority: P2 Milestone:
Module: User-Interface Keywords: manifestv3
Cc: greiner, sebastian Blocked By:
Blocking: #7308, #7338 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/merge_requests/254

Description (last modified by kzar)

Background

With #7338 we're going to remove the filterValidation module, since background scripts (e.g. messageResponder.js) no longer will have access to the DOM APIs which we rely on to validate CSS selectors.

What to change

  • Move adblockpluschrome/lib/filterValidation.js code to adblockplusui
  • Remove CSS validation logic from filter validation code

Notes

See also ui#383

Hints for testers

  • Test that custom filters can be added using the "Block element" tool and the options page. Also test that websites can be whitelisted properly. Test that when invalid filters are entered by the user the proper messages are displayed - see this table for some example filters. Note: We're no longer validating CSS selectors, so the "... is not a valid CSS selector" message is no longer expected to be displayed.

Hints for translators

Removed "invalid_css_selector" string.

Integration notes

  • Delete adblockpluschrome/lib/filterValidation.js and adblockpluschrome/qunit/tests/filterValidation.js, see #7338 for more context.

Change History (21)

comment:1 Changed 9 months ago by greiner

That module does more than just validate CSS selectors so it sounds like we should move the entire module to adblockplusui and then split off the parts that are dependent on the DOM so that those are run in the context of the various UI pages.

comment:2 Changed 9 months ago by kzar

Sounds good to me, since it seems like messageResponder.js is the only consumer of the filterValidation module.

comment:3 Changed 9 months ago by greiner

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

Thanks, I updated the ticket description to reflect that.

comment:4 Changed 9 months ago by greiner

  • Description modified (diff)

Added link to GitLab issue.

comment:5 Changed 9 months ago by kzar

Would you mind upping the priority of this issue? I ask since it blocks #7338 which is required for cluster 0.

comment:6 Changed 9 months ago by greiner

  • Priority changed from P3 to P2

Done. It's mostly a formality for us anyway. The way we in reality prioritize things is by assigning them to a UI release. For now I've assigned it to release-2018-5 whose dependency update is scheduled for the time when we decide to make the next major extension release.

comment:7 Changed 9 months ago by kzar

Gotya OK, thanks.

comment:8 in reply to: ↑ description ; follow-up: Changed 9 months ago by sebastian

  • Move adblockpluschrome/lib/filterValidation.js to adblockplus/ui/lib/filterValidation.js

With the CSS validation code removed, all is left of the filterValidation module is a trivial wrapper around Fitler.fromText() and instance of InvalidFilter. Furthermore, the functions parseFilter() and parseFilters() are only called from one code path in messageResponder.js. So why not just inline the code there?

comment:9 in reply to: ↑ 8 Changed 8 months ago by greiner

  • Description modified (diff)

Replying to sebastian:

So why not just inline the code there?

Makes sense. I updated the ticket description to leave it up to whoever implements it to assess how to best integrate it.

comment:10 Changed 8 months ago by kzar

  • Owner set to kzar

comment:11 Changed 8 months ago by greiner

  • Blocking 7431 added

comment:12 Changed 8 months ago by kzar

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

comment:13 Changed 8 months ago by greiner

  • Blocking 7308 added; 7431 removed

As requested, I've changed the corresponding dependency update to #7308 (i.e. release-2018-5.-4).

comment:14 Changed 8 months ago by kzar

  • Description modified (diff)

comment:15 Changed 8 months ago by greiner

  • Description modified (diff)

Added hints for translators.

comment:16 Changed 8 months ago by kzar

  • Description modified (diff)

comment:17 Changed 8 months ago by kzar

  • Description modified (diff)

comment:18 Changed 8 months ago by kzar

  • Description modified (diff)

comment:19 Changed 8 months ago by greiner

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

comment:20 Changed 8 months ago by abpbot

A commit referencing this issue has landed:
Issue 7423 - Stop relying on the filterValidation module

comment:21 Changed 5 months ago by ukacar

  • Verified working set
Note: See TracTickets for help on using tickets.