Summary: | Setting "selected" attribute to false should have no effect in single line <select> (affects jQuery) | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Christian <christian.lange.81> | ||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, darin, eae | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Christian
2011-01-14 01:56:59 PST
Created attachment 78962 [details]
reduced test case
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 ); } }); }, I filed this bug at jQuery today: http://bugs.jquery.com/ticket/7981 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 Could you please quote the relevant bits from HTML5? 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");
"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." Created attachment 80513 [details]
Patch
Thanks Darin. Added a test for -2 and used shouldBe as suggested.
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. 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. Comment on attachment 80513 [details] Patch Clearing flags on attachment: 80513 Committed r77206: <http://trac.webkit.org/changeset/77206> |