Empty error message when adding invalid filter

[greiner]

Environment

Adblock Plus 1.12.4.1722
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.

Background

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/global.properties 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 global.properties 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/metadata.chrome 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

[kzar]

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.

[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.

[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.

[kzar]

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

[kzar]

Argh...

[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"
  }
}

[kzar]

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

[kzar]

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

[trev]

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

[kzar]

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

[kzar]

Whoops, just realised he filed the issue...

[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.

[sebastian]

A commit referencing this issue has landed:
​Issue 4275, 4829 - Removed adblockplus dependency, added missing translations

[Ross]

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

ABP 2.99.0.1903beta
Firefox 57 / Windows 10

ABP 1.13.4.1903
Chrome 62 / Windows 10
Opera 48 / Windows 10

[sebastian]

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