Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#3892 closed change (rejected)

Disable button if input is invalid when adding subscriptions or filters

Reported by: sebastian Assignee: sebastian
Priority: P3 Milestone:
Module: User-Interface Keywords:
Cc: greiner, wspee Blocked By:
Blocking: #2355 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29339527

Description

Background

The old options page rejects empty or invalid URLs when adding subscriptions by URL. Currently, the new options page however doesn't prevent you from entering and empty or invalid URL when adding a subscription, causing further issues.

We came to the conclusion that disabling the button to add the subscription until a a valid URL has been entered would be the best user experience. For consistency, we should also disable the button to add a custom filter until a filter was entered.

What to change

Disable the buttons to add subscription or custom filter until the user entered a URL or filter respectively.

Change History (10)

comment:1 Changed 4 years ago by sebastian

  • Component changed from Unknown to User-Interface

comment:2 Changed 4 years ago by sebastian

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

comment:3 Changed 4 years ago by greiner

  • Priority changed from Unknown to P3
  • Ready set

comment:4 Changed 4 years ago by greiner

  • Blocking 2355 added

comment:5 Changed 3 years ago by saroyanm

This behavior specified already in the specification:
Inactive as long as Custom subscription popup subscription title input and Custom subscription popup subscription url input don't validate

So this should be taken care as part of #5158

comment:6 Changed 3 years ago by saroyanm

  • Resolution set to rejected
  • Status changed from reviewing to closed

See comment above

comment:7 follow-up: Changed 3 years ago by sebastian

  • Cc wspee added

The part of the specification you are quoting is about adding subscriptions, this issue was also about custom filters. I don't see anything about validating them in the specification, or did I miss it?

(Validation of custom filters is crucial, because an invalid element hiding filter results into breaking element hiding for other filters as well.)

Last edited 3 years ago by sebastian (previous) (diff)

comment:8 in reply to: ↑ 7 Changed 3 years ago by saroyanm

Replying to sebastian:

The part of the specification you are quoting is about adding subscriptions, this issue was also about custom filters. I don't see anything about validating them in the specification, or did I miss it?

I can only find validation for empty state(raw text mode), I don't see option to add single Custom Filter though.
are we going to have an option to add single Custom Filter ?

Version 0, edited 3 years ago by saroyanm (next)

comment:9 Changed 3 years ago by sebastian

Custom filters are currently validated, in both scenarios, when adding a single filter, and in raw edit mode, using parseFilter() and parseFilters() respectively. We definitely, have to retain this logic, not only to avoid unexpected results caused by user mistake, but more importantly because some invalid filters have a quite fatal impact (see the edit in my previous comment).

Moreover, the solution that has been specified in this issue wasn't great, as the validation error was discarded but merely the button to add the filter should have been disabled, leaving the user clueless what their mistake was. I hope with the new design we manage to do better, showing the validation error.

Last edited 3 years ago by sebastian (previous) (diff)

comment:10 Changed 3 years ago by wspee

The buttons won't be implemented so this remains closed but I created an issue against te specification that reflects the missing invalid filter validation:

https://bitbucket.org/adblockplus/spec/issues/5/handle-invalid-custom-filter

Note: See TracTickets for help on using tickets.