Bug 63709 - Disabled or read-only input type=search still allows clicking [x] to clear contents
Summary: Disabled or read-only input type=search still allows clicking [x] to clear co...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-30 06:23 PDT by Kentaro Hara
Modified: 2011-07-01 06:58 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 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.
Comment 1 Kentaro Hara 2011-06-30 06:57:00 PDT
Created attachment 99295 [details]
Patch
Comment 2 Kent Tamura 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.
Comment 3 Alexey Proskuryakov 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.
Comment 4 Kentaro Hara 2011-07-01 03:09:31 PDT
Created attachment 99451 [details]
Patch
Comment 5 Kentaro Hara 2011-07-01 03:10:29 PDT
Created attachment 99452 [details]
Patch
Comment 6 Kentaro Hara 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.
Comment 7 Kentaro Hara 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.
Comment 8 Kent Tamura 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.
Comment 9 Kent Tamura 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.
Comment 10 Kentaro Hara 2011-07-01 05:40:43 PDT
Created attachment 99463 [details]
Patch
Comment 11 Kentaro Hara 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.
Comment 12 Kent Tamura 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.
Comment 13 Kent Tamura 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.
Comment 14 Kentaro Hara 2011-07-01 06:04:12 PDT
Created attachment 99466 [details]
Patch
Comment 15 Kentaro Hara 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.
Comment 16 Kent Tamura 2011-07-01 06:06:23 PDT
Comment on attachment 99466 [details]
Patch

ok, great!
Comment 17 WebKit Review Bot 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>
Comment 18 WebKit Review Bot 2011-07-01 06:58:35 PDT
All reviewed patches have been landed.  Closing bug.