[Webkit-unassigned] [Bug 63709] Disabled input type=search still allows clicking [x] to clear contents

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=63709


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #99452|review?                     |review-
               Flag|                            |




--- Comment #8 from Kent Tamura <tkent at chromium.org>  2011-07-01 04:04:15 PST ---
(From update of attachment 99452)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.