Bug 63709

Summary:

Disabled or read-only input type=search still allows clicking [x] to clear contents

Product:

WebKit

Reporter:

Kentaro Hara <haraken>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

haraken, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Kentaro Hara

Reported 2011-06-30 06:23:58 PDT

See Chromium bug 85552: http://code.google.com/p/chromium/issues/detail?id=85552 Given the following code: <input type="search" value="Test" disabled> Chrome displays a text field with an [X] button on the right to clear the contents. Clicking the [X] button works even when the control is disabled. Safari, which also shows an [X], does not allow clicking it to clear the contents of the field when it's disabled.

Attachments
Patch (9.55 KB, patch) 2011-06-30 06:57 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Patch (7.77 KB, patch) 2011-07-01 03:09 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Patch (7.77 KB, patch) 2011-07-01 03:10 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Patch (11.03 KB, patch) 2011-07-01 05:40 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Patch (8.72 KB, patch) 2011-07-01 06:04 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Kentaro Hara

Comment 1 2011-06-30 06:57:00 PDT

Created attachment 99295 [details] Patch

Kent Tamura

Comment 2 2011-06-30 07:16:27 PDT

Comment on attachment 99295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99295&action=review BTW, Does NSSearchField show the [X] button when it is disabled/readonly? > LayoutTests/fast/forms/search-cancel-button-mouseup.html:2 > <html> This file is for 'cancel-button-mouseup'. So merging a test case of this bug is not suitable. We have disabled-search-input.html. It's a better place to add the test case. > LayoutTests/fast/forms/search-cancel-button-mouseup.html:13 > +<input id="search2" type="search" value="value2"><br /> > +<input id="search3" type="search" value="value3" disabled="disabled"><br /> > +<input id="search4" type="search" value="value4" disabled="disabled"><br /> > +<input id="search5" type="search" value="value5" disabled="disabled"><br /> You fixes 'readonly' too. So you should make test cases for 'readonly' too. > LayoutTests/fast/forms/search-cancel-button-mouseup.html:22 > + var input = document.getElementById("search0"); nit: You can use getElementsByTagName('input') and index-access. Also, resources/common.js has $(). > LayoutTests/fast/forms/search-cancel-button-mouseup.html:43 > + eventSender.mouseMoveTo(middleX, y); > + eventSender.mouseDown(); > + eventSender.mouseMoveTo(buttonX, y); > + eventSender.mouseUp(); You had better have click(x, y) function. > Source/WebCore/ChangeLog:5 > + Disallow clicking an [X] button in 'search' input forms when 'disabled' attribute is set. Please mention 'readonly' too.

Alexey Proskuryakov

Comment 3 2011-06-30 12:26:12 PDT

It's partially a regression - in Safari 5.0.5, the [x] button accepted clicks, but it didn't clear the content in disabled element.

Kentaro Hara

Comment 4 2011-07-01 03:09:31 PDT

Created attachment 99451 [details] Patch

Kentaro Hara

Comment 5 2011-07-01 03:10:29 PDT

Created attachment 99452 [details] Patch

Kentaro Hara

Comment 6 2011-07-01 03:11:46 PDT

Comment on attachment 99295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99295&action=review >> LayoutTests/fast/forms/search-cancel-button-mouseup.html:2 >> <html> > > This file is for 'cancel-button-mouseup'. So merging a test case of this bug is not suitable. > We have disabled-search-input.html. It's a better place to add the test case. Done. Reverted search-cancel-button-mouseup.html to the original. >> LayoutTests/fast/forms/search-cancel-button-mouseup.html:13 >> +<input id="search5" type="search" value="value5" disabled="disabled"><br /> > > You fixes 'readonly' too. So you should make test cases for 'readonly' too. Added the test cases. >> LayoutTests/fast/forms/search-cancel-button-mouseup.html:22 >> + var input = document.getElementById("search0"); > > nit: You can use getElementsByTagName('input') and index-access. Also, resources/common.js has $(). I removed multiple 'input's and instead used one 'input' in the latest patch. >> LayoutTests/fast/forms/search-cancel-button-mouseup.html:43 >> + eventSender.mouseUp(); > > You had better have click(x, y) function. Done. >> Source/WebCore/ChangeLog:5 >> + Disallow clicking an [X] button in 'search' input forms when 'disabled' attribute is set. > > Please mention 'readonly' too. Done.

Kentaro Hara

Comment 7 2011-07-01 03:19:04 PDT

> It's partially a regression - in Safari 5.0.5, the [x] button accepted clicks, but it didn't clear the content in disabled element. Thank you very much for the review. I confirmed the followings: - In released Safari 5.0.5 (6533.21.1, with WebKit r81529), the [X] button accepts clicks. The [X] button does not clear the content if 'disabled' is set, but clears the content if 'readonly' is set. - In Safari 5.0.5 (6533.21.1, with WebKit r89995), the [X] button accepts clicks. The [X] button clears the content if 'disabled' or 'readonly' is set. - In Safari 5.0.5 (with my WebKit patch), the [X] button accepts clicks. The [X] button does not clear the content if 'disabled' or 'readonly' is set.

Kent Tamura

Comment 8 2011-07-01 04:04:15 PDT

Comment on attachment 99452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99452&action=review > LayoutTests/fast/forms/disabled-search-input-expected.txt:11 > +PASS Value is "". > +PASS Value is "b". > +PASS Value is "foo". > +PASS Value is "foo". > +PASS Value is "foo". > +PASS Value is "foo". > +PASS Value is "foo". > +PASS Value is "foo". Test result readability is not good. Please show each of test conditions by debug(); > LayoutTests/fast/forms/disabled-search-input.html:8 > +<input id="search_input" type="search" /> The purpose of the existing test is to check behavior in a case that a disabled search field gets enabled. You must add 'disabled' here. > LayoutTests/fast/forms/disabled-search-input.html:15 > +function click(button) { The argument name 'button' is confusing. It's a position of something. > LayoutTests/fast/forms/disabled-search-input.html:37 > +function checkResult(input, expected) { > + if (input.value == expected) > + testPassed('Value is "' + input.value + '".'); > + else > + testFailed('Value is "' + input.value + '", but it should be "' + expected + '".'); > +} This function is not needed. You can write it as: shouldBe('input.value', '"foo"'); > LayoutTests/fast/forms/disabled-search-input.html:39 > +function setInputAttribute(input, value, disabled, readonly) { Attribute -> Attributes? > LayoutTests/fast/forms/disabled-search-input.html:45 > + if (readonly) > + input.setAttribute("readonly", readonly); > + else if (input.hasAttribute("readonly")) > + input.removeAttribute("readonly"); Why don't you use input.readOnly = readonly ? > LayoutTests/fast/forms/disabled-search-input.html:50 > + var cancelButton = searchCancelButtonPosition(input); 'cancelButton' is not a good name. It's a position, not button.

Kent Tamura

Comment 9 2011-07-01 04:08:45 PDT

Comment on attachment 99452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99452&action=review >> LayoutTests/fast/forms/disabled-search-input.html:8 >> +<input id="search_input" type="search" /> > > The purpose of the existing test is to check behavior in a case that a disabled search field gets enabled. You must add 'disabled' here. Also, the existing test clicks the center of the search field in order to get focus, not the cancel button.

Kentaro Hara

Comment 10 2011-07-01 05:40:43 PDT

Created attachment 99463 [details] Patch

Kentaro Hara

Comment 11 2011-07-01 05:41:06 PDT

Comment on attachment 99452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99452&action=review >> LayoutTests/fast/forms/disabled-search-input-expected.txt:11 >> +PASS Value is "foo". > > Test result readability is not good. > Please show each of test conditions by debug(); Done. > LayoutTests/fast/forms/disabled-search-input.html:1 > +<!DOCTYPE html> I renamed the file to search-disabled-readonly.html. >>> LayoutTests/fast/forms/disabled-search-input.html:8 >>> +<input id="search_input" type="search" /> >> >> The purpose of the existing test is to check behavior in a case that a disabled search field gets enabled. You must add 'disabled' here. > > Also, the existing test clicks the center of the search field in order to get focus, not the cancel button. Done. I added the test cases for both, i.e. (1) click the center of the form and (2) click the cancel button. >> LayoutTests/fast/forms/disabled-search-input.html:15 >> +function click(button) { > > The argument name 'button' is confusing. It's a position of something. Done. >> LayoutTests/fast/forms/disabled-search-input.html:37 >> +} > > This function is not needed. > You can write it as: > shouldBe('input.value', '"foo"'); Done. >> LayoutTests/fast/forms/disabled-search-input.html:39 >> +function setInputAttribute(input, value, disabled, readonly) { > > Attribute -> Attributes? Done. >> LayoutTests/fast/forms/disabled-search-input.html:45 >> + input.removeAttribute("readonly"); > > Why don't you use input.readOnly = readonly ? Done. >> LayoutTests/fast/forms/disabled-search-input.html:50 >> + var cancelButton = searchCancelButtonPosition(input); > > 'cancelButton' is not a good name. It's a position, not button. Done.

Kent Tamura

Comment 12 2011-07-01 05:48:52 PDT

Comment on attachment 99463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99463&action=review > LayoutTests/fast/forms/search-disabled-readonly-expected.txt:8 > +Test on the input form with disabled=false and readonly=false > +Click the cancel button: > +PASS input.value is "" > +... and then input one character: > +PASS input.value is "b" This doesn't cover the original scenario of disabled-search-input.html. The scenario was: 1. layout with <input type=search disabled> 2. Click the center of it 3. Type a key 4. Expect that the character is inserted. But, you don't need to cover the scenario if you don't remove disabled-search-input.html.

Kent Tamura

Comment 13 2011-07-01 05:49:47 PDT

(In reply to comment #12) > This doesn't cover the original scenario of disabled-search-input.html. > The scenario was: > 1. layout with <input type=search disabled> 1.5 disabled = false > 2. Click the center of it > 3. Type a key > 4. Expect that the character is inserted. > > But, you don't need to cover the scenario if you don't remove disabled-search-input.html.

Kentaro Hara

Comment 14 2011-07-01 06:04:12 PDT

Created attachment 99466 [details] Patch

Kentaro Hara

Comment 15 2011-07-01 06:06:21 PDT

Comment on attachment 99463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99463&action=review >> LayoutTests/fast/forms/search-disabled-readonly-expected.txt:8 >> +PASS input.value is "b" > > This doesn't cover the original scenario of disabled-search-input.html. > The scenario was: > 1. layout with <input type=search disabled> > 2. Click the center of it > 3. Type a key > 4. Expect that the character is inserted. > > But, you don't need to cover the scenario if you don't remove disabled-search-input.html. Ah...sorry, I got it. In conclusion, I left disabled-search-input.html as it is, and added search-disabled-readonly.html.

Kent Tamura

Comment 16 2011-07-01 06:06:23 PDT

Comment on attachment 99466 [details] Patch ok, great!

WebKit Review Bot

Comment 17 2011-07-01 06:58:31 PDT

Comment on attachment 99466 [details] Patch Clearing flags on attachment: 99466 Committed r90225: <http://trac.webkit.org/changeset/90225>

WebKit Review Bot

Comment 18 2011-07-01 06:58:35 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.