Bug 27942 – Form Reset does not work per w3c standard when single selection SELECT element has more than one option marked as selected.

• Component: Forms
• Status: RESOLVED FIXED

[Carol Szabo]

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.

[Carol Szabo]

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.

[Carol Szabo]

Created attachment 33982 [details]
Fixed a code style guideline issue

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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);

[Kenneth Rohde Christiansen]

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.

[Carol Szabo]

Created attachment 34007 [details]
Proposed patch improved and test added

[Darin Adler]

Why did you chose Firefox’s behavior over IE’s since they don’t agree?

Does HTML 5 have anything to say about this?

[Ian 'Hixie' Hickson]

"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.

[Carol Szabo]

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.

[Darin Adler]

(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.

[Darin Adler]

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!

[Carol Szabo]

Created attachment 34082 [details]
Removed the executable flag from the html file and added newline at the end of that file

[Carol Szabo]

(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.

[Carol Szabo]

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.

[Darin Adler]

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.

[Carol Szabo]

Created attachment 34088 [details]
Implemented Darin's suggestion in comment #15

[Adam Barth]

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

[Carol Szabo]

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.

[Darin Adler]

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.

[Adam Barth]

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

[Adam Barth]

All reviewed patches have been landed.  Closing bug.