Opened 11 months ago

Last modified 3 weeks ago

#4829 reopened defect

Empty error message when adding invalid filter

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

https://codereview.adblockplus.org/29372899/

Description (last modified by kzar)

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

Change History (26)

comment:1 Changed 11 months ago by mapx

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

comment:2 Changed 11 months 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 11 months 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 11 months 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 11 months 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 11 months ago by kzar

  • Blocked By 4833 added
  • Blocking 4833 removed

Argh...

comment:7 Changed 11 months ago by trev

  • Blocked By 4833 removed

comment:8 Changed 11 months ago by trev

  • Blocked By 4833 added

comment:9 Changed 11 months ago by trev

  • Blocked By 4833 removed

comment:10 Changed 11 months ago by trev

  • Blocked By 4833 added

comment:11 Changed 11 months ago by kzar

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

comment:12 Changed 11 months 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 9 months ago by kzar

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

comment:14 Changed 7 months 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 4 months ago by trev

  • Blocked By 4833 removed

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

comment:16 Changed 3 months 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 months ago by kzar

  • Cc greiner removed
  • Description modified (diff)

Whoops, just realised he filed the issue...

comment:18 Changed 3 months ago by kzar

  • Description modified (diff)

comment:19 Changed 3 months ago by kzar

  • Description modified (diff)

comment:20 Changed 3 months ago by kzar

  • Blocking 4275 added; 5751 removed

comment:21 Changed 3 months ago by kzar

  • Blocking

comment:22 Changed 3 months 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 months ago by sebastian

  • Owner set to sebastian

comment:24 Changed 3 months 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 7 weeks ago by 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

comment:26 Changed 3 weeks 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
Note: See TracTickets for help on using tickets.