Bug 8455

Summary:

selectedIndex for a select after a Form reset() has wrong value

Product:

WebKit

Reporter:

Christos Zisopoulos <czisopoulos>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

czisopoulos

Priority:

Version:

417.x

Hardware:

Mac

OS:

OS X 10.4

Attachments:

Description	Flags
a simple demonstration of the bug	none
Fix bug in testcase	none
First attempt	ggaren: review-
Light version	ggaren: review+

Christos Zisopoulos

Reported 2006-04-18 06:44:37 PDT

In a simple for with a <select/> two <option/> tags, when a page loads selectedIndex is 0 (the first option). After calling document.forms[0].reset(), selectedIndex is -1 Only by explicitly specifying an option as selected, will you get the expected behaviour after a reset. <html> <head> <title>test</title> <script type="text/javascript" language="javascript" charset="utf-8"> function selectBug() { alert("Before Form.reset() selectedIndex is " + document.forms[0].elements[0].selectedIndex); document.forms[0].reset(); alert("AFTER Form.reset() selectedIndex is " + document.forms[0].elements[0].selectedIndex); } </script> </head> <body onload="selectBug()"> <form action="test_submit" method="post"> <select id="select_1" name="event[event_type_id]"> <option value="1" >Value 1</option> <option value="2" selected>Value 2</option> </select> <input name="commit" type="submit" value="Submit" /> </form> </body> </html>

Attachments
a simple demonstration of the bug (1.16 KB, text/html) 2006-04-18 06:49 PDT, Christos Zisopoulos	no flags	Details
Fix bug in testcase (1.16 KB, text/html) 2006-05-23 12:30 PDT, Rob Buis	no flags	Details
First attempt (1.56 KB, patch) 2006-05-23 12:48 PDT, Rob Buis	ggaren: review-	Details Formatted Diff Diff
Light version (3.57 KB, patch) 2006-05-24 02:05 PDT, Rob Buis	ggaren: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Christos Zisopoulos

Comment 1 2006-04-18 06:49:33 PDT

Created attachment 7800 [details] a simple demonstration of the bug

Christos Zisopoulos

Comment 2 2006-04-18 06:50:52 PDT

Possible duplicate of #6282?

Alexey Proskuryakov

Comment 3 2006-04-18 13:12:23 PDT

Confirmed with stock 10.4.5 Safari. ToT fails with an assertion failure, thus upgrading to P2. ASSERTION FAILED: firstOption == listItems.size() || found (/Users/ap/WebKit/WebCore/rendering/render_form.cpp:904 void WebCore::RenderSelect::updateSelection())

Rob Buis

Comment 4 2006-05-23 12:30:14 PDT

Created attachment 8490 [details] Fix bug in testcase I am pretty sure the testcase wanted to use forms[1] reset, otherwise testing for selectedIndex of forms[1] is pointless. Cheers, Rob.

Rob Buis

Comment 5 2006-05-23 12:48:11 PDT

Created attachment 8491 [details] First attempt First stab at fixing this. It may be too "heavy", in which case I'd like to hear how to make it "lighter" :) Ofcourse if the code is okayed, I'll provide a testcase... Cheers, Rob.

Geoffrey Garen

Comment 6 2006-05-23 13:38:35 PDT

Comment on attachment 8491 [details] First attempt It should be sufficient to keep a flag tracking whether any of the option elements had the 'selected' attribute set, and select option 0 if none did. That would avoid the call to recalcListItems(). I'm going to mark this r-, in case you want to submit the 'lighter' solution.

Rob Buis

Comment 7 2006-05-24 02:05:19 PDT

Created attachment 8510 [details] Light version Hi Geoff, Here is my "light" version including testcase. Cheers, Rob.

Geoffrey Garen

Comment 8 2006-05-24 07:26:06 PDT

Comment on attachment 8510 [details] Light version firstOption is unnecessary. You can just use items[0]. That's a nitpick, though. Nice work.

Rob Buis

Comment 9 2006-05-24 14:16:21 PDT

Hi, (In reply to comment #8) > (From update of attachment 8510 [details] [edit]) > firstOption is unnecessary. You can just use items[0]. That's a nitpick, > though. Nice work. > As discussed on irc, this is mostly a preference thing. If always item[0] was the correct one, I'd agree, but it doesnt have to be, so we'd need a while loop. I do not think either solution is perfect, though I slightly prefer the current one. Cheers, Rob.

Note You need to log in before you can comment on or make changes to this bug.