| 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: | P2 | Keywords: | InRadar | ||||||||
| Version: | 312.x | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows Vista | ||||||||||
| Attachments: |
|
||||||||||
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? (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. (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... I figure this is related...
<!-- single selection -->
<select name="user" size="20">
....
alert(select.getAttribute('multiple')) // null
<!-- multiple selection -->
<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.
Created attachment 124049 [details]
Patch
Looking at other browsers it seems it only happens when the size attribute is > 1.
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. 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'? Created attachment 124577 [details]
Fixes from review comments.
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. 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/ Created attachment 124642 [details]
Fixed changelogs
Comment on attachment 124642 [details]
Fixed changelogs
ok
Comment on attachment 124642 [details] Fixed changelogs Clearing flags on attachment: 124642 Committed r106318: <http://trac.webkit.org/changeset/106318> All reviewed patches have been landed. Closing bug. |
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>