Opened 10 months ago

Closed 10 months ago

Last modified 7 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 10 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 10 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 10 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 10 months ago by greiner

  • Description modified (diff)

Added link to GitLab issue.

comment:5 Changed 10 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 10 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 10 months ago by kzar

Gotya OK, thanks.

comment:8 in reply to: ↑ description ; follow-up: Changed 10 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 10 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 10 months ago by kzar

  • Owner set to kzar

comment:11 Changed 10 months ago by greiner

  • Blocking 7431 added

comment:12 Changed 10 months ago by kzar

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

comment:13 Changed 10 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 10 months ago by kzar

  • Description modified (diff)

comment:15 Changed 10 months ago by greiner

  • Description modified (diff)

Added hints for translators.

comment:16 Changed 10 months ago by kzar

  • Description modified (diff)

comment:17 Changed 10 months ago by kzar

  • Description modified (diff)

comment:18 Changed 10 months ago by kzar

  • Description modified (diff)

comment:19 Changed 10 months ago by greiner

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

comment:20 Changed 9 months ago by abpbot

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

comment:21 Changed 7 months ago by ukacar

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