Opened 11 months ago

Closed 10 months ago

Last modified 10 months ago

#6845 closed change (duplicate)

Remove translatable strings from adblockpluschrome

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

Description (last modified by kzar)

Background

We plan to move the translatable strings over from adblockpluschrome to adblockplusui. With #6633 we're adding the files to adblockplusui, so now we need to remove them from adblockpluschrome and update the metadata.

What to change

  • Update the adblockplusui dependency to git:FIXME hg:FIXME.
  • Remove all the locale files.
  • Update the [import_locales] section of metadata.chrome to import those files from adblockplusui.

Hints for testers

  • Check that all the moved translatable strings still work (aren't blank). For a complete list of the strings see Thomas' summary.
  • The following functionality uses those strings:
    • "Block element" tool
    • issue reporter
    • Adblock Plus popup (when you click the ABP icon)
    • notifications

The invalid filter error messages have also been moved, some example filters:

String nameTriggering filterNotes
filter_elemhideemulation_nodomain #?#foo
filter_snippet_nodomain #$#foo This will not work until the string has been added, see #6797 and #6846.
filter_invalid_csp adblockplus.org$csp=base-uri
filter_invalid_domain ,##foo
filter_invalid_regexp /[/
filter_unknown_option $foo
invalid_css_selector ##..
unexpected_filter_list_header [ This will not work from the filter composer, will need to be tested in a subscription.

Change History (14)

comment:1 Changed 11 months ago by kzar

Thomas can you help fill in the blanks in the table? I'm trying to figure out examples of filters which the testers can use for the various error messages.

comment:2 Changed 11 months ago by kzar

  • Description modified (diff)

comment:3 Changed 11 months ago by kzar

  • Description modified (diff)

comment:4 Changed 11 months ago by kzar

  • Description modified (diff)

comment:5 Changed 11 months ago by greiner

  • Description modified (diff)

Replying to kzar:

Thomas can you help fill in the blanks in the table?

Sure, no problem. I added examples for the remaining filter validation errors to the ticket description.

comment:6 Changed 11 months ago by kzar

Thanks, those all work for me except [ which is silently stripped instead of displaying the unexpected_filter_list_header message.

comment:7 Changed 11 months ago by greiner

Strange, I'll have to look into that. Because parseFilter() does return the correct error message for me so it's likely a UI bug.

comment:8 Changed 11 months ago by kzar

  • Description modified (diff)

comment:9 Changed 11 months ago by greiner

I looked into the issue and turns out it's by design according to https://codereview.adblockplus.org/29321198/diff/29321199/messageResponder.js#newcode221.

comment:10 Changed 11 months ago by kzar

  • Description modified (diff)

Gotya, OK. I've added some notes to the table.

comment:11 Changed 11 months ago by kzar

  • Description modified (diff)

comment:12 Changed 10 months ago by greiner

  • Resolution set to duplicate
  • Status changed from new to closed

This change has landed as part of #6892 so I'm closing this ticket as duplicate.

comment:13 Changed 10 months ago by sebastian

I'm a little late to the party, but regarding unexpected_filter_list_header, originally we showed that error to users when they attempt to add a single filter that looked like a filter list header (on the old options page). However, since the introduction of the new options page, it seems this error is never shown at all, as custom filters can only be edited in bulk where we silently strip such filters (and unlike implied in the issue description here, this validation is only applied to custom filters, not to subscriptions). So we should remove the string unexpected_filter_list_header from adblockplusui, and adapt the logic in adblockpluschrome/lib/filterValidation.js accordingly.

comment:14 Changed 10 months ago by greiner

Well spotted.

Note, however, that the text area was only meant as a temporary solution and that we are going to reintroduce the ability to add single filters as part of the custom filter list overhaul in ui#15.

Note: See TracTickets for help on using tickets.