Issue 29339527: Issue 3892 - Disable button if input is invalid when adding subscriptions or filters

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Issue 29339527: Issue 3892 - Disable button if input is invalid when adding subscriptions or filters (Closed)

Created:
April 6, 2016, 10:22 p.m. by Sebastian Noack

Modified:
May 29, 2017, 8:50 a.m.

Reviewers:
Thomas Greiner

Visibility:
Public.

Description

Issue 3892 - Disable button if input is invalid when adding subscriptions or filters

Patch Set 1 : #

Total comments: 4

Created: April 6, 2016, 10:43 p.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+47 lines, -17 lines)			Patch
M	options.html	View	2 chunks	+2 lines, -2 lines	1 comment	Download
M	options.js	View	5 chunks	+23 lines, -9 lines	1 comment	Download
M	skin/options.css	View	5 chunks	+22 lines, -6 lines	2 comments	Download
M	skin/options-sprite.png	View	Binary file		0 comments	Download

Messages

Total messages: 2

Expand All Messages | Collapse All Messages

Sebastian Noack

https://codereview.adblockplus.org/29339527/diff/29339533/options.html File options.html (right): https://codereview.adblockplus.org/29339527/diff/29339533/options.html#newcode406 options.html:406: <input id="blockingList-textbox" type="url" required="true" pattern="https?://.+" class="default-focus" placeholder="https://example.com/blockinglist.txt" /> Two ...

April 6, 2016, 10:46 p.m. (2016-04-06 22:46:02 UTC) #1

https://codereview.adblockplus.org/29339527/diff/29339533/options.html
File options.html (right):

https://codereview.adblockplus.org/29339527/diff/29339533/options.html#newcod...
options.html:406: <input id="blockingList-textbox" type="url" required="true"
pattern="https?://.+" class="default-focus"
placeholder="https://example.com/blockinglist.txt" />
Two notes here:

1. type=url only checks whether there is a protocol delimiter with at least on
non-whitespace character before and after it. Therefore the additional pattern
which is consistent with the behavior on the old options page.
2. The placeholder previously indicated an URL with omitted protocol part.
Regardless of the new validation, this simply doesn't work with the current
implementation.

https://codereview.adblockplus.org/29339527/diff/29339533/options.js
File options.js (right):

https://codereview.adblockplus.org/29339527/diff/29339533/options.js#newcode847
options.js:847: E("blockingList-textbox").value = "";
This is kinda unrelated, but it seems to have been another bug that when you add
a subscription (or cancelled the dialog), and reopen the dialog to add another
subscription, that the previous URL is still showing.

https://codereview.adblockplus.org/29339527/diff/29339533/skin/options.css
File skin/options.css (left):

https://codereview.adblockplus.org/29339527/diff/29339533/skin/options.css#ol...
skin/options.css:484: cursor: pointer;
This was redundant, as we already set "cursor" to "pointer" on the element
itself. But if we'd keep it redundant here, we would also have to override it
redundantly when the button is disabled. Same below.

https://codereview.adblockplus.org/29339527/diff/29339533/skin/options.css#ol...
skin/options.css:882: background-position: -28px -32px;
This position was wrong. The icon is actually just 8px wide, however the pixel
left of it as well as two pixels right of it are empty. So with the previous
position the icon was one pixel too far left. But who cares? However, now where
we switch between the gray (disabled) and blue (active) variant of the icon, the
icon would jump by one pixel if we don't fix this.

Sebastian Noack

May 22, 2017, 1:28 p.m. (2017-05-22 13:28:46 UTC) #2

Closing this issue unmerged, as this change is no longer relevant with the new
design. See the related discussion in the issue tracker.

Expand All Messages | Collapse All Messages