Bug 76389

Summary:

Form select option not de-selecting

Product:

WebKit

Reporter:

buy12

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Minor

CC:

ap, jonlee, pf, tkent, webkit-bug-importer, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

312.x

Hardware:

OS:

Windows Vista

Attachments:

Description	Flags
Patch	none
Fixes from review comments.	none
Fixed changelogs	none

buy12

Reported 2012-01-16 09:43:30 PST

Hi, I have a form select object that has size = 20 and no "multiple" attribute defined and has nothing selected. This results in the first option being selected, which is wrong: ----------------------------------------------------------------------------------- alert(select.getAttribute('multiple') // null select.options[1].selected = true; select.options[1].selected = false; alert(select.options[0].selected); // true in chrome (16) and safari (5.1) ----------------------------------------------------------------------------------- But, setting attribute 'multiple' to '', makes everything ok: ----------------------------------------------------------------------------------- select.setAttribute('multiple',''); alert(select.getAttribute('multiple') // '' select.options[1].selected = true; select.options[1].selected = false; alert(select.options[0].selected); // false, which is correct ----------------------------------------------------------------------------------- <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> <html xmlns="http://www.w3.org/1999/xhtml"><head><title>Test</title> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" /> </head> <body onload="test()"> <form name="form1" action=""> <select name="user" size="20"> <option value="1">one</option> <option value="2">two</option> </select> </form> <script type="text/javascript"> function test () { var select = document.form1.user; select.options[1].selected = true; select.options[1].selected = false; } </script> </body></html>

Attachments
Patch (6.80 KB, patch) 2012-01-25 18:16 PST, Pablo Flouret	no flags	Details Formatted Diff Diff
Fixes from review comments. (9.02 KB, patch) 2012-01-30 12:04 PST, Pablo Flouret	no flags	Details Formatted Diff Diff
Fixed changelogs (9.02 KB, patch) 2012-01-30 17:46 PST, Pablo Flouret	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Jon Lee

Comment 1 2012-01-16 10:44:43 PST

I can confirm this, setting status to new. When the option gets deselected, the select has no selection. Somewhere in the select code it doesn't know to set the selected index to -1. Instead, it defaults to 0. This can be confirmed by having the second option be set to be the default selection. While this is definitely a bug, I also find this pattern strange. Why not just say select.selectedIndex = -1?

Radar WebKit Bug Importer

Comment 2 2012-01-16 10:45:00 PST

<rdar://problem/10700830>

buy12

Comment 3 2012-01-16 10:58:24 PST

(In reply to comment #1) > I can confirm this, setting status to new. > > When the option gets deselected, the select has no selection. Somewhere in the select code it doesn't know to set the selected index to -1. Instead, it defaults to 0. This can be confirmed by having the second option be set to be the default selection. > > While this is definitely a bug, I also find this pattern strange. Why not just say select.selectedIndex = -1? Better user experience. Have an ordered list of names that I'm allowing users to search through. Say you're looking for "Smith" and typing the letters in a text box, as you type each letter, the next name in the list is highlighted. Then you mess up and type an "x" instead of "h", there would be no match, so if you did a select.selectedIndex = -1, it would move the focus back to the top of the list. Not a big deal by any stretch, I just think it's better not to do that.

buy12

Comment 4 2012-01-16 11:28:47 PST

(In reply to comment #3) > (In reply to comment #1) > > I can confirm this, setting status to new. > > > > When the option gets deselected, the select has no selection. Somewhere in the select code it doesn't know to set the selected index to -1. Instead, it defaults to 0. This can be confirmed by having the second option be set to be the default selection. > > > > While this is definitely a bug, I also find this pattern strange. Why not just say select.selectedIndex = -1? > > Better user experience. Have an ordered list of names that I'm allowing users to search through. Say you're looking for "Smith" and typing the letters in a text box, as you type each letter, the next name in the list is highlighted. Then you mess up and type an "x" instead of "h", there would be no match, so if you did a select.selectedIndex = -1, it would move the focus back to the top of the list. Not a big deal by any stretch, I just think it's better not to do that. Wanted to follow up on this statement, this behavior does not occur with chrome/safari. I'm testing on multiple browsers...

buy12

Comment 5 2012-01-16 13:14:29 PST

I figure this is related...  <select name="user" size="20"> .... alert(select.getAttribute('multiple')) // null  <select name="user" size="20" multiple> .... alert(select.getAttribute('multiple')) // '' There doesn't appear to be a way of using 'blank' and null to switch the select element from behaving like a multiple select element to a single select element. I would think that, select.setAttribute('multiple',null) would change the multiple select list to a single select list, but that's not working for me.

Pablo Flouret

Comment 6 2012-01-25 18:16:21 PST

Created attachment 124049 [details] Patch Looking at other browsers it seems it only happens when the size attribute is > 1.

Jon Lee

Comment 7 2012-01-29 21:47:59 PST

Comment on attachment 124049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124049&action=review > Source/WebCore/html/HTMLSelectElement.cpp:771 > + selectOption(m_multiple || size() > 1 ? -1 : nextSelectableListIndex(-1)); I think for clarity we tend to parenthesize the whole conditional even though rules of operator priority makes this correct anyway. However, given the more complex expression, I would instead opt to just break this up into an "else if" and "else" clause, for readability and parallelism. > LayoutTests/ChangeLog:9 > + * fast/forms/select-option-selecting.html: Added. I've never quite understood where to place these tests-- I assume to keep the folder manageable we should be putting new tests in fast/forms/select/ instead? It looks like newer tests are being added there.

Kent Tamura

Comment 8 2012-01-30 02:27:24 PST

Comment on attachment 124049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124049&action=review > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + Test: fast/forms/select-option-selecting.html You had better mention rationale of the change. e.g. specification says so, other browsers work so. >> Source/WebCore/html/HTMLSelectElement.cpp:771 >> + selectOption(m_multiple || size() > 1 ? -1 : nextSelectableListIndex(-1)); > > I think for clarity we tend to parenthesize the whole conditional even though rules of operator priority makes this correct anyway. > > However, given the more complex expression, I would instead opt to just break this up into an "else if" and "else" clause, for readability and parallelism. Should this be !usesMenuList()? >> LayoutTests/ChangeLog:9 >> + * fast/forms/select-option-selecting.html: Added. > > I've never quite understood where to place these tests-- I assume to keep the folder manageable we should be putting new tests in fast/forms/select/ instead? It looks like newer tests are being added there. Jon is right. We had better put tests for <select> into fast/forms/select/. Bug 65915 > LayoutTests/fast/forms/select-option-selecting.html:33 > +var normal = document.forms[0].elements.normal; 'normal' is not a helpful name. It should be menuListWithoutSize or something. > LayoutTests/fast/forms/select-option-selecting.html:37 > +normal.options[2].selected = true; > +normal.options[2].selected = false; Please add debug('Set and reset normal.options[2].selected'); to improve readability of test result. > LayoutTests/fast/forms/select-option-selecting.html:42 > +var defaultSizedNormal = document.forms[0].elements.defaultSizedNormal; 'defaultSize' is confusing with having no size. Should be 'menuListWIthSize1' or something? > LayoutTests/fast/forms/select-option-selecting.html:51 > +var sizedNormal = document.forms[0].elements.sizedNormal; 'sizedNormal' is not a good name. 'singleListBox'? > LayoutTests/fast/forms/select-option-selecting.html:60 > +sizedNormal.selectedIndex = 1; > +shouldBe("sizedNormal.selectedIndex", "1"); The test result is not readable. This should be: shouldBe("sizedNormal.selectedIndex = 1; sizedNormal.selectedIndex", "1"); or debug("Setting 1 to selectedIndex"); sizedNormal.selectedIndex = 1; shouldBe(...) > LayoutTests/fast/forms/select-option-selecting.html:64 > +sizedNormal.selectedIndex = -1; > +shouldBe("sizedNormal.selectedIndex", "-1"); ditto. > LayoutTests/fast/forms/select-option-selecting.html:69 > +var multiple = document.forms[0].elements.multiple; How about changing the name to 'multipleListBox'?

Pablo Flouret

Comment 9 2012-01-30 12:04:55 PST

Created attachment 124577 [details] Fixes from review comments.

Pablo Flouret

Comment 10 2012-01-30 12:10:41 PST

Comment on attachment 124049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124049&action=review >>> Source/WebCore/html/HTMLSelectElement.cpp:771 >>> + selectOption(m_multiple || size() > 1 ? -1 : nextSelectableListIndex(-1)); >> >> I think for clarity we tend to parenthesize the whole conditional even though rules of operator priority makes this correct anyway. >> >> However, given the more complex expression, I would instead opt to just break this up into an "else if" and "else" clause, for readability and parallelism. > > Should this be !usesMenuList()? I think it makes sense, yeah.

Kent Tamura

Comment 11 2012-01-30 17:37:44 PST

Comment on attachment 124577 [details] Fixes from review comments. View in context: https://bugs.webkit.org/attachment.cgi?id=124577&action=review > LayoutTests/ChangeLog:11 > + * fast/forms/select-option-selecting-expected.txt: Added. > + * fast/forms/select-option-selecting.html: Added. Please update the ChangeLog. fast/forms/ -> fast/forms/select/

Pablo Flouret

Comment 12 2012-01-30 17:46:02 PST

Created attachment 124642 [details] Fixed changelogs

Kent Tamura

Comment 13 2012-01-30 17:47:17 PST

Comment on attachment 124642 [details] Fixed changelogs ok

WebKit Review Bot

Comment 14 2012-01-30 18:55:22 PST

Comment on attachment 124642 [details] Fixed changelogs Clearing flags on attachment: 124642 Committed r106318: <http://trac.webkit.org/changeset/106318>

WebKit Review Bot

Comment 15 2012-01-30 18:55:26 PST

All reviewed patches have been landed. Closing bug.

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