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.
Created attachment 99295 [details] Patch
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.
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.
Created attachment 99451 [details] Patch
Created attachment 99452 [details] Patch
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.
> 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 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 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.
Created attachment 99463 [details] Patch
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 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.
(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.
Created attachment 99466 [details] Patch
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 on attachment 99466 [details] Patch ok, great!
Comment on attachment 99466 [details] Patch Clearing flags on attachment: 99466 Committed r90225: <http://trac.webkit.org/changeset/90225>
All reviewed patches have been landed. Closing bug.