RESOLVED FIXED Bug 52436
Setting "selected" attribute to false should have no effect in single line <select> (affects jQuery) 
https://bugs.webkit.org/show_bug.cgi?id=52436
Summary Setting "selected" attribute to false should have no effect in single line <s...
Christian
Reported 2011-01-14 01:56:59 PST
Hi (I am using Ubuntu 10.04) I got a <select> element with three <option> inside (my page ist XHTML 1.0 Transitional), looks like this: <select> <option value="0">0</option> <option value="1">1</option> <option value="2">2</option> </select> None of those is selected manually (i.e. has the "selected='selected'" tag) but on first loading the page, the option "0" ist shown by default. When I use jQuery to delete all "selected" tags in all <option>-element (which are not there), suddenly no option is selected anymore. You can check it yourself with following (w3c validated) little code: <!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" xml:lang="de" lang="de"> <head> <title>test</title> <meta http-equiv="Content-Type" content="text/html;charset=utf-8"/> <script type="text/javascript" src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.4/jquery.min.js"></script> </head> <body> <select id="s" size="1"> <option value="0">0</option> <option value="1">1</option> <option value="2">2</option> </select> <input id="i" type="button" value="Remove 'selected'"/> <script type="text/javascript"> $(document).ready(function() { $("#i").click(function() { $("#s").find("option").removeAttr("selected"); }); }); </script> </body> </html> This disappearing of a selected value happens in Google Chrome 8.0.552.237, Chromium 10.0.639.0 (71411) as well as Safari 5.0.3 (7533.19.4). You see, pretty new browsers. According to webkit.org the webkit version of Chromium is 534.16 but the newest webkit version is r75294. So I can't really tell if I got the newest one, it's a confusing name scheme. This behaviour, however, does not appear in Firefox 3.6.13, Opera 11.00 or Internet Explorer 8. I think that the way Firefox, Opera and IE handle this is the correct way. Could you please check if this is a bug in WebKit? Best regards, Christian
Attachments
reduced test case (507 bytes, text/html)
2011-01-14 10:54 PST, Alexey Proskuryakov 		no flags
Details
Patch (6.40 KB, patch)
2011-01-28 14:36 PST, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (6.62 KB, patch)
2011-01-28 15:41 PST, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2011-01-14 10:54:17 PST
Created attachment 78962 [details] reduced test case
Alexey Proskuryakov
Comment 2 2011-01-14 11:02:22 PST
This is not really about removing the "selected" attribute - behind the scenes, jQuery first sets it to false. I'm not sure if WebKit is actually broken here, but we should investigate this as a difference with Firefox. Please consider also filing a bug against jQuery, as there is no apparent reason for removeAttr() to behave this way. removeAttr: function( name, fn ) { return this.each(function(){ jQuery.attr( this, name, "" ); // <-- this sets .selected to false in your test case if ( this.nodeType === 1 ) { this.removeAttribute( name ); } }); },
Christian
Comment 3 2011-01-15 00:21:57 PST
I filed this bug at jQuery today: http://bugs.jquery.com/ticket/7981
Emil A Eklund
Comment 4 2011-01-28 14:36:30 PST
Created attachment 80498 [details] Patch While the original specification doesn't specify a behavior for this case the HTML5 [1] draft does. This patch changes the behavior to match the HTML5 draft specification and to match the Mozilla behavior. IE selects the last selectable element rather than the first but apart from that exhibits the same behavior. 1: http://dev.w3.org/html5/spec/Overview.html#the-select-element
Alexey Proskuryakov
Comment 5 2011-01-28 15:06:19 PST
Could you please quote the relevant bits from HTML5?
Darin Adler
Comment 6 2011-01-28 15:17:37 PST
Comment on attachment 80498 [details] Patch I’d like to see a test that covers trying to set the selected index to -2. For future reference, you can sometimes make better tests by putting more of the test into the shouldBe function: shouldBe("selectElement.selectedIndex = 0; selectElement.selectedIndex", "0");
Emil A Eklund
Comment 7 2011-01-28 15:20:09 PST
"If the multiple attribute is absent and the element's display size is 1, then whenever there are no option elements in the select element's list of options that have their selectedness set to true, the user agent must set the selectedness of the first option element in the list of options in tree order that is not disabled, if any, to true."
Emil A Eklund
Comment 8 2011-01-28 15:41:14 PST
Created attachment 80513 [details] Patch Thanks Darin. Added a test for -2 and used shouldBe as suggested.
Darin Adler
Comment 9 2011-01-31 14:48:49 PST
Comment on attachment 80513 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80513&action=review > Source/WebCore/dom/SelectElement.cpp:325 > - const Vector<Element*>& items = data.listItems(element); > - int listIndex = optionToListIndex(data, element, optionIndex); > + if (optionIndex == -1 && !deselect && !data.multiple()) > + optionIndex = nextSelectableListIndex(data, element, -1); > if (!data.multiple()) > deselect = true; > > + const Vector<Element*>& items = data.listItems(element); > + int listIndex = optionToListIndex(data, element, optionIndex); > + I didn’t criticize this the first time around, but I realize now that the setSelectedIndex function is a kind of low level place to put this logic. Normally I’d expect to see the logic in the callers. Is there a higher level place in the class we can put this? I also think the new block of code needs a “Why” comment; it’s a bit mysterious without one. Maybe we could follow up this patch with a patch that adds that comment. Also, I don’t know why you chose to reorder the existing code. If you hadn’t reordered it, this patch would only show the new code you are adding. But because you chose to put the deselect = true code above the optionToListIndex code, there is a larger change here. With no rationale for the change. > LayoutTests/ChangeLog:8 > + Add test for changing the selection in a one-line select element using\ Stray backslash here. > LayoutTests/fast/dom/HTMLSelectElement/selected-false.html:30 > + optionElements[2].selected = true; > + optionElements[1].selected = true; > + shouldBe("selectElement.selectedIndex", "1"); These could be inside the shouldBe too just as the single statement lines are. There’s no reason to have this part of the test be hidden in the test output.
Eric Seidel (no email)
Comment 10 2011-01-31 16:06:58 PST
Comment on attachment 80498 [details] Patch Cleared Darin Adler's review+ from obsolete attachment 80498 [details] so that this bug does not appear in http://webkit.org/pending-commit.
WebKit Commit Bot
Comment 11 2011-01-31 19:45:57 PST
Comment on attachment 80513 [details] Patch Clearing flags on attachment: 80513 Committed r77206: <http://trac.webkit.org/changeset/77206>
Note You need to log in before you can comment on or make changes to this bug.