Opened on 03/29/2019 at 10:43:45 AM
Closed on 04/05/2019 at 05:11:08 PM
Last modified on 07/10/2019 at 12:46:16 PM
#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.
Attachments (0)
Change History (21)
comment:1 Changed on 03/29/2019 at 12:24:36 PM by greiner
comment:2 Changed on 03/29/2019 at 01:31:11 PM by kzar
Sounds good to me, since it seems like messageResponder.js is the only consumer of the filterValidation module.
comment:3 Changed on 03/29/2019 at 01:33:18 PM 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 on 03/29/2019 at 01:34:08 PM by greiner
- Description modified (diff)
Added link to GitLab issue.
comment:5 Changed on 03/29/2019 at 01:49:49 PM 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 on 03/29/2019 at 01:56:11 PM 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 on 03/29/2019 at 05:30:05 PM by kzar
Gotya OK, thanks.
comment:8 in reply to: ↑ description ; follow-up: ↓ 9 Changed on 03/29/2019 at 10:41:47 PM 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 on 04/01/2019 at 02:36:36 PM 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 on 04/02/2019 at 09:34:55 AM by kzar
- Owner set to kzar
comment:11 Changed on 04/02/2019 at 02:16:30 PM by greiner
- Blocking 7431 added
comment:12 Changed on 04/02/2019 at 02:37:14 PM by kzar
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:13 Changed on 04/02/2019 at 02:54:39 PM 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 on 04/02/2019 at 02:58:37 PM by kzar
- Description modified (diff)
comment:15 Changed on 04/02/2019 at 03:00:37 PM by greiner
- Description modified (diff)
Added hints for translators.
comment:16 Changed on 04/02/2019 at 03:05:42 PM by kzar
- Description modified (diff)
comment:17 Changed on 04/02/2019 at 03:06:12 PM by kzar
- Description modified (diff)
comment:18 Changed on 04/02/2019 at 03:35:07 PM by kzar
- Description modified (diff)
comment:19 Changed on 04/05/2019 at 05:11:08 PM by greiner
- Resolution set to fixed
- Status changed from reviewing to closed
comment:20 Changed on 04/26/2019 at 12:48:44 PM by abpbot
A commit referencing this issue has landed:
Issue 7423 - Stop relying on the filterValidation module
comment:21 Changed on 07/10/2019 at 12:46:16 PM by ukacar
- Verified working set
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.