Opened 4 years ago

Closed 13 months ago

#4829 closed defect (rejected)

Empty error message when adding invalid filter

Reported by: greiner Assignee: sebastian
Priority: P2 Milestone:
Module: Platform Keywords: closed-in-favor-of-gitlab
Cc: kzar, sebastian, Ross, greiner Blocked By:
Blocking: #4275, #4275 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by kzar)


Adblock Plus
Chrome 55.0.2883.87
Ubuntu 16.04

How to reproduce

  1. Open the options page and navigate to "Add custom filters" tab.
  2. Attempt to add the filter ,##foo.

Observed behaviour

Empty modal dialog appears.

Expected behaviour

Modal dialog appears and contains further information about the error.


The filter validation that produces the error "filter_invalid_domain" was introduced in #4450 but without an error text associated with it. Therefore adding a translatable string with the ID "filter_invalid_domain" to adblockplus/chrome/locale/en-US/ should be sufficient to fix this issue.

The problem is the adblockplus repository is going away, we hardly want to add more strings there. So instead let's move the filter_* messages over into adblockplusui while we're at it.

What to change

  • Move the filter_* strings from in adblockplus to new-options.json in adblockplusui, synchronise the updated adblockplusui project with Crowdin. (No need to remove the strings from adblockplus nor update that project in Crowdin, since it's going away.)
  • Add the filter_invalid_domain string with the message of "Invalid (or empty) domain specified" .

Integration notes

You'll have to adjust adblockpluschrome/ when updating the adblockplusui dependency in order to take the filter_* strings from adblockplusui instead of adblockplus.

Hints for testers

Open the options page and trigger the various errors, make sure they are displayed properly:

  • filter_elemhide_duplicate_id
  • filter_elemhideemulation_nodomain
  • filter_elemhide_nocriteria
  • filter_invalid_domain
  • filter_invalid_regexp
  • filter_regexp_tooltip
  • filter_unknown_option

Change History (28)

comment:1 Changed 4 years ago by mapx

  • Component changed from Adblock-Plus-for-Firefox to Platform

comment:2 Changed 4 years ago by kzar

  • Cc sebastian trev added

Can reproduce as described, but this raises the question where these strings should live. Since we're working towards removing the adblockplus dependency from adblockpluschrome it seems wrong to add the string there.

Should we instead move all these strings over to adblockplusui and add filter_invalid_domain whilst at it? Should we instead start adding strings to adblockpluscore instead?

Either way I guess this issue will eventually change module, either become UI or core and we'll need further issues for the dependency updates.

comment:3 Changed 4 years ago by trev

These strings definitely belong in adblockpluscore, this is where they are used. However, moving them from adblockplus to adblockpluscore isn't going to be entirely straightforward so we might want to add the missing string to adblockplus for now and address the migration independently.

comment:4 Changed 4 years ago by kzar

OK, well I volunteer to open an issue for that and to do the work if you can create the adblockpluscore project for me on CrowdIn and give me access.

As for whether we should add the string to adblockplus first, I'd rather just fix this properly personally but I don't mind too much either way.

comment:5 Changed 4 years ago by kzar

  • Blocking 4833 added

(I've opened #4833 for that, I'll set it as blocking for now but I don't mind if you undo.)

comment:6 Changed 4 years ago by kzar

  • Blocked By 4833 added
  • Blocking 4833 removed


comment:7 Changed 4 years ago by trev

  • Blocked By 4833 removed

comment:8 Changed 4 years ago by trev

  • Blocked By 4833 added

comment:9 Changed 4 years ago by trev

  • Blocked By 4833 removed

comment:10 Changed 4 years ago by trev

  • Blocked By 4833 added

comment:11 Changed 4 years ago by kzar

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

comment:12 Changed 4 years ago by kzar

I prepared everything for these strings to go in adblockpluscore, but a discussion came up on IRC about weather they should really belong in adblockplusui.

Until that's decided I can't continue with this. Both sound reasonable to me so I don't mind if the strings end up in core or ui. Thomas & Wladimir what do you think?

For reference the strings in question are as follows, but note the text for filter_invalid_domain might still change:

  "filter_elemhide_duplicate_id": {
    "message": "Only one ID of the element to be hidden can be specified"
  "filter_elemhideemulation_nodomain": {
    "message": "No active domain specified for extended element hiding filter"
  "filter_elemhide_nocriteria": {
    "message": "No criteria specified to recognize the element to be hidden"
  "filter_invalid_domain": {
    "message": "Invalid (or empty) domain specified"
  "filter_invalid_regexp": {
    "message": "Invalid regular expression"
  "filter_regexp_tooltip": {
    "message": "This filter is either a regular expression or too short to be optimized. Too many of these filters might slow down your browsing."
  "filter_unknown_option": {
    "message": "Unknown filter option"

comment:13 Changed 4 years ago by kzar

Sorry to nag you guys but should I put these strings in adblockplusui or adblockpluscore?

comment:14 Changed 3 years ago by kzar

greiner, trev: So this is still blocked, any decision about if filter_invalid_domain and similar texts should live in adblockplusui or adblockpluscore?

comment:15 Changed 3 years ago by trev

  • Blocked By 4833 removed

I just closed #4833, these strings should go in adblockplusui.

comment:16 Changed 3 years ago by kzar

  • Blocking 5751 added
  • Cc greiner added
  • Component changed from Platform to User-Interface
  • Description modified (diff)
  • Platform changed from Chrome to Unknown / Cross platform
  • Status changed from reviewing to reopened

(Copying in Thomas since this is now an adblockplusui change.)

comment:17 Changed 3 years ago by kzar

  • Cc greiner removed
  • Description modified (diff)

Whoops, just realised he filed the issue...

comment:18 Changed 3 years ago by kzar

  • Description modified (diff)

comment:19 Changed 3 years ago by kzar

  • Description modified (diff)

comment:20 Changed 3 years ago by kzar

  • Blocking 4275 added; 5751 removed

comment:21 Changed 3 years ago by kzar

  • Blocking

comment:22 Changed 3 years ago by sebastian

If those strings aren't directly used in adblockplusui, they should rather go into adblockpluschrome, also see ticket:4275#comment:4. The reason we initially planned to move all shared strings to adblockplusui (regardless whether they are used there) was merely a suboptimal compromise, since they were also used by adblockplus which is deprecated now.

comment:23 Changed 3 years ago by sebastian

  • Owner set to sebastian

comment:24 Changed 3 years ago by sebastian

  • Component changed from User-Interface to Platform
  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Resolution set to fixed
  • Status changed from reopened to closed

comment:25 Changed 3 years ago by Ross

Filter error messages are still blank/empty. (Firefox contains "Line 1")

Firefox 57 / Windows 10

Chrome 62 / Windows 10
Opera 48 / Windows 10

comment:26 Changed 3 years ago by kzar

  • Cc Ross greiner added
  • Milestone Adblock-Plus-3.0-for-Chrome-Opera-Firefox deleted
  • Priority changed from Unknown to P2
  • Ready set
  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:27 Changed 3 years ago by fhd

  • Cc trev removed

comment:28 Changed 13 months ago by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from reopened to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the new issue tracker.

Note: See TracTickets for help on using tickets.