|Summary:||Setting value on a select element to a non existing option value should clear selection|
|Product:||WebKit||Reporter:||Erik Arvidsson <arv>|
|Component:||Forms||Assignee:||Jon Lee <jonlee>|
|Severity:||Normal||CC:||ap, ian, jonlee, rafaelw, webkit-bug-importer, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Erik Arvidsson 2011-08-30 14:37:28 PDT
Given: <select> <option value="a">A</option> <option value="b">B</option> </select> <script> var sel = document.querySelector('select'); sel.value = 'x' assert(sel.selectedIndex === -1) </script> The spec says: "On setting, the value attribute must set the selectedness of all the option elements in the list of options to false, and then the first option element in the list of options, in tree order, whose value is equal to the given new value, if any, must have its selectedness set to true." This means that we should unselect all and then select first option that matches the value.
Comment 1 Alexey Proskuryakov 2011-08-31 16:32:29 PDT
This fails in WebKit and in Firefox, so sounds like a mistake in the spec.
Comment 2 Rafael Weinstein 2011-08-31 17:32:27 PDT
IE9 follows the spec. FWIW (probably not much), the spec seems like the right (desirable) behavior to me.
Comment 4 Alexey Proskuryakov 2011-08-31 23:16:15 PDT
WebKit/Gecko behavior doesn't seem unnatural or confusing to me. Why base the spec on a minority engine behavior? Is there known Web compatibility impact?
Comment 6 WebKit Review Bot 2012-01-03 13:17:01 PST
Attachment 120984 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource Index mismatch: 3a7674dadd24ecd6a1b023eba203ecf2ed62fc59 != edb861612940a3244431d239dacdbc36317b627c rereading e1e2538cff7d320c2b7cc22c0b75c9369ce9cdb2 A LayoutTests/svg/custom/webkit-transform-crash.html A LayoutTests/svg/custom/webkit-transform-crash-expected.txt M LayoutTests/ChangeLog M Source/WebCore/ChangeLog M Source/WebCore/svg/SVGStyledTransformableElement.cpp 103950 = 8282a1d85ad097ce4064a5896912bdb211b115e6 already exists! Why are we refetching it? at /usr/lib/git-core/git-svn line 5210 Died at Tools/Scripts/update-webkit line 158. If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Alexey Proskuryakov 2012-01-03 13:33:28 PST
I'm still not sure why we want to change this.
Comment 8 Darin Adler 2012-01-10 11:44:24 PST
Comment on attachment 120984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120984&action=review > Source/WebCore/html/HTMLSelectElement.cpp:257 > + // Setting the value clears the selectedness of all options. > + setSelectedIndex(-1); Does calling setSelectedIndex twice have any additional effect? For example, do we get additional change events? To word this another way, why isn’t this call in the return statement and then again at the end of the function?
Comment 9 Jon Lee 2012-01-10 13:52:12 PST
Comment on attachment 120984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120984&action=review >> Source/WebCore/html/HTMLSelectElement.cpp:257 >> + setSelectedIndex(-1); > > Does calling setSelectedIndex twice have any additional effect? For example, do we get additional change events? > > To word this another way, why isn’t this call in the return statement and then again at the end of the function? In this case there should be no extra change event calls, but I like your approach, since that guarantees just one setSelectedIndex() call for any given case. I will do this instead.
Comment 10 Jon Lee 2012-01-12 14:42:52 PST
Without this patch there'd be inconsistency between value and selectedIndex, since both are supposed to clear the selectedness of all options first. Also, when <select> has no selection, value returns ''. I'd expect that setting value to '' would remove any existing selection. Moreover, Alexey and I did some additional testing and found that IE7 and IE8 seem to also follow the spec. So, any sites in which WK behavior would has different from IE's probably will have implemented a separate code path, further preventing any potential breaks in existing web sites. It seems low risk to make this behavior follow the spec, so I will land the patch with Darin's suggested changes.