Created attachment 33978 [details] Test page that shows the bug HTML 4.01 which is honored by XHTML 1.0 and 1.1 specifies that when a select element does not have the multiple attribute set a browser should never select more than one option element even when the source of the page has multiple options marked as selected as in this case (see sections 17.6.1 and 17.2.1 at http://www.w3.org/TR/html401/interact/forms.html) WebKit selects multiple options after the reset button is clicked, if these options are selected in the source of the page even for a single selection select element.
Created attachment 33981 [details] Proposed patch Here is a proposed fix. This fix makes WebKit emulated Firefox (select the last option marked as selected), which is behavior compliant with the standard. IE 7, which is also compliant with the standard selects the first item marked as selected in this case.
Created attachment 33982 [details] Fixed a code style guideline issue
Comment on attachment 33978 [details] Test page that shows the bug I don't think you meant to mark this for review. See http://webkit.org/coding/contributing.html
Comment on attachment 33982 [details] Fixed a code style guideline issue Please see http://webkit.org/coding/contributing.html. You need to add your test and expected results as part of this patch. If this change is untestable, you need to explain why. See http://webkit.org/coding/coding-style.html which documents how we use full english word/phrases for variable names. Thus: + OptionElement* selectedEl = 0; is not an OK variable name. The indent is also wrong here: + selectedEl->setSelectedState(false);
There is a script in WebKitTools/Scripts called check-webkit-style that will *help* you to conform to the coding style guideline, though the script doesn't check everything such as variable names.
Created attachment 34007 [details] Proposed patch improved and test added
Why did you chose Firefox’s behavior over IE’s since they don’t agree? Does HTML 5 have anything to say about this?
"The reset algorithm for select elements is to go through all the option elements in the element's list of options, and set their selectedness to true if the option element has a selected attribute, and false otherwise." "If the multiple attribute is absent, whenever an option element in the select element's list of options has its selectedness set to true, and whenever an option element with its selectedness set to true is added to the select element's list of options, the user agent must set the selectedness of all the other option element in its list of options to false." So per spec in the original test case after hitting reset you'd end up with just "3" being selected, just like initially.
I chose Mozilla's behavior because it would yield the same item selected after reset as it was when the form first loaded and it was conforming to the spec. If I were to choose IE's behavior I would have either caused a different option to be selected after reset than after load or I would have had to change the behavior at page load also. As Ian 'Hixie' commented, my choice seems to match the behavior specified for load, assuming that options are viewed as added in textual order to the select element. In my humble opinion the case my patch is trying to address is an unusual and unimportant case anyway. It deals with a web page that is coded incorrectly. The biggest problem the bug caused is confusion on the user that the selection box may allow multiple selection. Choosing any single value after reset would have solved that.
(In reply to comment #9) > I chose Mozilla's behavior because it would yield the same item selected after > reset as it was when the form first loaded and it was conforming to the spec. Since Mozilla's behavior matches both what HTML5 specifies and what value we would have when the page was first loaded, then it seems like the right thing to do, even though it does not match IE.
Comment on attachment 34007 [details] Proposed patch improved and test added This patch looks wrong. As written all it does is rename the boolean to "isOptionSelected" and add a new, never-used "selectedOption". > + selectedOption=optionElement; Missing spaces around the "=" here. > +</form></body></html> > \ No newline at end of file Please add the newline. > Property changes on: LayoutTests/fast/forms/select-reset-multiple-selections-4-single-selection.html > ___________________________________________________________________ > Added: svn:executable > + * Please don't check in executable .html files. You can fix this by using chmod -x before adding the file to Subversion, or svn pd svn:executable after adding the file to Subversion. review- because the code change as posted doesn't seem to do anything!
Created attachment 34082 [details] Removed the executable flag from the html file and added newline at the end of that file
(In reply to comment #11) > (From update of attachment 34007 [details]) > This patch looks wrong. As written all it does is rename the boolean to > "isOptionSelected" and add a new, never-used "selectedOption". > > > + selectedOption=optionElement; > Darin, I think that you missed these lines: if (!items[i]->getAttribute(HTMLNames::selectedAttr).isNull()) { + if (selectedOption && !data.multiple()) + selectedOption->setSelectedState(false); optionElement->setSelectedState(true); - optionSelected = true; + selectedOption = optionElement; + isOptionSelected = true; They are intended to unselect the previously selected options if the select box is not a multiple selection box.
Created attachment 34083 [details] Fixed coding style issue. I apologize for the repeated coding style offenses. The check-webkit-coding-style script does not run on my system so I have to manually check my style and apparently I am not very good at it. I promise to make a better effort next time.
Comment on attachment 34083 [details] Fixed coding style issue. > + selectedOption = optionElement; > + isOptionSelected = true; I suggest eliminating the isOptionSelected boolean. It's the same as a null check on selectedOption now. > - if (!optionSelected && firstOption && data.usesMenuList()) > + if (!isOptionSelected && firstOption && data.usesMenuList()) Here you would just say !selectedOption.
Created attachment 34088 [details] Implemented Darin's suggestion in comment #15
Comment on attachment 34088 [details] Implemented Darin's suggestion in comment #15 I don't know what this error means, but the commit queue can't handle this patch: patching file WebCore/dom/SelectElement.cpp patch: **** malformed patch at line 15: @@ -502,8 +503,11 @@ void SelectElement::reset(SelectElementD
Created attachment 34138 [details] Regenerated patch should yield the same result as previous. This patch only differs from previous version in some of the control information for patching the only modified source file. Hopefully this goes through the patching process. I used svn-apply to test this patch and apparently it works.
Comment on attachment 34138 [details] Regenerated patch should yield the same result as previous. > Property changes on: LayoutTests/fast/forms/select-reset-multiple-selections-4-single-selection-expected.txt > ___________________________________________________________________ > Added: svn:executable > + * New text files should not be marked executable! I'll mark this r=me because that's minor and should be harmless.
Comment on attachment 34138 [details] Regenerated patch should yield the same result as previous. Clearing review flag on attachment: 34138 Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/fast/forms/select-reset-multiple-selections-4-single-selection-expected.txt A LayoutTests/fast/forms/select-reset-multiple-selections-4-single-selection.html M WebCore/ChangeLog M WebCore/dom/SelectElement.cpp Committed r46806 M WebKitTools/ChangeLog M WebKitTools/Scripts/modules/committers.py r46805 = 40ace582a5cfa5bb9bcd358a3747e3323f3c4a43 (trunk) M WebCore/dom/SelectElement.cpp M WebCore/ChangeLog M LayoutTests/ChangeLog A LayoutTests/fast/forms/select-reset-multiple-selections-4-single-selection-expected.txt A LayoutTests/fast/forms/select-reset-multiple-selections-4-single-selection.html r46806 = 8a73293fc161d96e48f967b087cf3131c3b11f62 (trunk) First, rewinding head to replay your work on top of it... Nothing to do. http://trac.webkit.org/changeset/46806
All reviewed patches have been landed. Closing bug.