Skip to content

Commit

Permalink
Don't have .val() return selected-but-disabled options, or selected o…
Browse files Browse the repository at this point in the history 
…ptions inside a disabled optgroup. Doesn't change the .val() returned for a disabled select. Fixes #3240, adapted from Nathan Hammond's patch there.
  • Loading branch information
@dmethvin @jeresig
dmethvin authored and jeresig committed Sep 24, 2010
1 parent 700ff05 commit 2c4b208
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 12 deletions.
6 changes: 4 additions & 2 deletions src/attributes.js
View file
Expand Up @@ -163,8 +163,10 @@ jQuery.fn.extend({
for ( var i = one ? index : 0, max = one ? index + 1 : options.length; i < max; i++ ) { for ( var i = one ? index : 0, max = one ? index + 1 : options.length; i < max; i++ ) {
var option = options[ i ]; var option = options[ i ];


if ( option.selected ) { // Don't return options that are disabled or in a disabled optgroup
// Get the specifc value for the option if ( option.selected && !option.disabled &&
(!option.parentNode.disabled || !jQuery.nodeName( option.parentNode, "optgroup" )) ) {
// Get the specific value for the option
value = jQuery(option).val(); value = jQuery(option).val();


// We don't need an array for one selects // We don't need an array for one selects
Expand Down
9 changes: 9 additions & 0 deletions test/index.html
View file
Expand Up @@ -114,6 +114,15 @@ <h2 id="qunit-userAgent"></h2>
<option id="option3d" value="3">3</option> <option id="option3d" value="3">3</option>
<option id="option3e">no value</option> <option id="option3e">no value</option>
</select> </select>
<select name="select4" id="select4" multiple="multiple">
<optgroup disabled="disabled">
<option id="option4a" class="emptyopt" value="">Nothing</option>
<option id="option4b" disabled="disabled" selected="selected" value="1">1</option>
<option id="option4c" selected="selected" value="2">2</option>
</optgroup>
<option selected="selected" disabled="disabled" id="option4d" value="3">3</option>
<option id="option4e">no value</option>
</select>


<object id="object1" codebase="stupid"> <object id="object1" codebase="stupid">
<param name="p1" value="x1" /> <param name="p1" value="x1" />
Expand Down
10 changes: 9 additions & 1 deletion test/unit/attributes.js
View file
Expand Up @@ -302,7 +302,7 @@ test("removeAttr(String)", function() {
}); });


test("val()", function() { test("val()", function() {
expect(17); expect(20);


document.getElementById('text1').value = "bla"; document.getElementById('text1').value = "bla";
equals( jQuery("#text1").val(), "bla", "Check for modified value of input element" ); equals( jQuery("#text1").val(), "bla", "Check for modified value of input element" );
Expand All @@ -329,6 +329,14 @@ test("val()", function() {
jQuery('#select3').val(""); jQuery('#select3').val("");
same( jQuery('#select3').val(), [''], 'Call val() on a multiple="multiple" select' ); same( jQuery('#select3').val(), [''], 'Call val() on a multiple="multiple" select' );


same( jQuery('#select4').val(), [], 'Call val() on multiple="multiple" select with all disabled options' );

jQuery('#select4 optgroup').add('#select4 > [disabled]').attr('disabled', false);
same( jQuery('#select4').val(), ['2', '3'], 'Call val() on multiple="multiple" select with some disabled options' );

jQuery('#select4').attr('disabled', true);
same( jQuery('#select4').val(), ['2', '3'], 'Call val() on disabled multiple="multiple" select' );

This comment has been minimized.

Sign in to view

Copy link
@jitter

jitter Sep 27, 2010

Contributor

This test fails for all Webkit browsers listed on the testswarm http://swarm.jquery.org/job/225/. The cause seems to be that Webkit (different from all other browsers) returns true for the disabled attribute of all options which are immediate childrens of a disabled select. Even when the options itself hasn't the disabled attribute set explicitly

This is the reason why the option with id option4d is missing from the actual result. The only workaround I could find is to use the getAttribute() function in Safari (didn't test if this also works in the other Webkit based browsers). Which returns null if the disabled attribute isn't set or false and an empty string if it is set to true / disabled. While option.disabled always returns true if the parent select is disabled not matter if the option itself is explicitly disabled or not.

So we would basically need to check if we are using a Webkit based browser or find other means to determine if we can rely on option.disabled in attribute.js val() and I don't know if that's acceptable.

This comment has been minimized.

Sign in to view

Copy link
@jeresig

jeresig Sep 27, 2010

Member

Thanks for the catch! I did some additional testing on the technique that you proposed and it seems to work fine everywhere that I can test so I'm going to push ahead with that. Landed in 157a383

var checks = jQuery("<input type='checkbox' name='test' value='1'/><input type='checkbox' name='test' value='2'/><input type='checkbox' name='test' value=''/><input type='checkbox' name='test'/>").appendTo("#form"); var checks = jQuery("<input type='checkbox' name='test' value='1'/><input type='checkbox' name='test' value='2'/><input type='checkbox' name='test' value=''/><input type='checkbox' name='test'/>").appendTo("#form");


same( checks.serialize(), "", "Get unchecked values." ); same( checks.serialize(), "", "Get unchecked values." );
Expand Down
2 changes: 1 addition & 1 deletion test/unit/event.js
View file
Expand Up @@ -255,7 +255,7 @@ test("bind(), iframes", function() {
}); });


test("bind(), trigger change on select", function() { test("bind(), trigger change on select", function() {
expect(3); expect(4);
var counter = 0; var counter = 0;
function selectOnChange(event) { function selectOnChange(event) {
equals( event.data, counter++, "Event.data is not a global event object" ); equals( event.data, counter++, "Event.data is not a global event object" );
Expand Down
12 changes: 6 additions & 6 deletions test/unit/selector.js
View file
Expand Up @@ -21,7 +21,7 @@ test("element", function() {
same( jQuery("p", jQuery("div")).get(), q("firstp","ap","sndp","en","sap","first"), "Finding elements with a context." ); same( jQuery("p", jQuery("div")).get(), q("firstp","ap","sndp","en","sap","first"), "Finding elements with a context." );
same( jQuery("div").find("p").get(), q("firstp","ap","sndp","en","sap","first"), "Finding elements with a context." ); same( jQuery("div").find("p").get(), q("firstp","ap","sndp","en","sap","first"), "Finding elements with a context." );


same( jQuery("#form").find("select").get(), q("select1","select2","select3"), "Finding selects with a context." ); same( jQuery("#form").find("select").get(), q("select1","select2","select3","select4"), "Finding selects with a context." );


ok( jQuery("#length").length, '&lt;input name="length"&gt; cannot be found under IE, see #945' ); ok( jQuery("#length").length, '&lt;input name="length"&gt; cannot be found under IE, see #945' );
ok( jQuery("#lengthtest input").length, '&lt;input name="length"&gt; cannot be found under IE, see #945' ); ok( jQuery("#lengthtest input").length, '&lt;input name="length"&gt; cannot be found under IE, see #945' );
Expand Down Expand Up @@ -338,7 +338,7 @@ test("pseudo - :not", function() {
expect(24); expect(24);
t( "Not", "a.blog:not(.link)", ["mark"] ); t( "Not", "a.blog:not(.link)", ["mark"] );


t( "Not - multiple", "#form option:not(:contains(Nothing),#option1b,:selected)", ["option1c", "option1d", "option2b", "option2c", "option3d", "option3e"] ); t( "Not - multiple", "#form option:not(:contains(Nothing),#option1b,:selected)", ["option1c", "option1d", "option2b", "option2c", "option3d", "option3e", "option4e"] );
t( "Not - recursive", "#form option:not(:not(:selected))[id^='option3']", [ "option3b", "option3c"] ); t( "Not - recursive", "#form option:not(:not(:selected))[id^='option3']", [ "option3b", "option3c"] );


t( ":not() failing interior", "p:not(.foo)", ["firstp","ap","sndp","en","sap","first"] ); t( ":not() failing interior", "p:not(.foo)", ["firstp","ap","sndp","en","sap","first"] );
Expand All @@ -360,8 +360,8 @@ test("pseudo - :not", function() {
t( "No element not selector", ".container div:not(.excluded) div", [] ); t( "No element not selector", ".container div:not(.excluded) div", [] );


t( ":not() Existing attribute", "#form select:not([multiple])", ["select1", "select2"]); t( ":not() Existing attribute", "#form select:not([multiple])", ["select1", "select2"]);
t( ":not() Equals attribute", "#form select:not([name=select1])", ["select2", "select3"]); t( ":not() Equals attribute", "#form select:not([name=select1])", ["select2", "select3", "select4"]);
t( ":not() Equals quoted attribute", "#form select:not([name='select1'])", ["select2", "select3"]); t( ":not() Equals quoted attribute", "#form select:not([name='select1'])", ["select2", "select3", "select4"]);


t( ":not() Multiple Class", "#foo a:not(.blog)", ["yahoo","anchor2"] ); t( ":not() Multiple Class", "#foo a:not(.blog)", ["yahoo","anchor2"] );
t( ":not() Multiple Class", "#foo a:not(.link)", ["yahoo","anchor2"] ); t( ":not() Multiple Class", "#foo a:not(.link)", ["yahoo","anchor2"] );
Expand Down Expand Up @@ -427,13 +427,13 @@ test("pseudo - visibility", function() {
test("pseudo - form", function() { test("pseudo - form", function() {
expect(8); expect(8);


t( "Form element :input", "#form :input", ["text1", "text2", "radio1", "radio2", "check1", "check2", "hidden1", "hidden2", "name", "search", "button", "area1", "select1", "select2", "select3"] ); t( "Form element :input", "#form :input", ["text1", "text2", "radio1", "radio2", "check1", "check2", "hidden1", "hidden2", "name", "search", "button", "area1", "select1", "select2", "select3", "select4"] );
t( "Form element :radio", "#form :radio", ["radio1", "radio2"] ); t( "Form element :radio", "#form :radio", ["radio1", "radio2"] );
t( "Form element :checkbox", "#form :checkbox", ["check1", "check2"] ); t( "Form element :checkbox", "#form :checkbox", ["check1", "check2"] );
t( "Form element :text", "#form :text:not(#search)", ["text1", "text2", "hidden2", "name"] ); t( "Form element :text", "#form :text:not(#search)", ["text1", "text2", "hidden2", "name"] );
t( "Form element :radio:checked", "#form :radio:checked", ["radio2"] ); t( "Form element :radio:checked", "#form :radio:checked", ["radio2"] );
t( "Form element :checkbox:checked", "#form :checkbox:checked", ["check1"] ); t( "Form element :checkbox:checked", "#form :checkbox:checked", ["check1"] );
t( "Form element :radio:checked, :checkbox:checked", "#form :radio:checked, #form :checkbox:checked", ["radio2", "check1"] ); t( "Form element :radio:checked, :checkbox:checked", "#form :radio:checked, #form :checkbox:checked", ["radio2", "check1"] );


t( "Selected Option Element", "#form option:selected", ["option1a","option2d","option3b","option3c"] ); t( "Selected Option Element", "#form option:selected", ["option1a","option2d","option3b","option3c","option4b","option4c","option4d"] );
}); });
4 changes: 2 additions & 2 deletions test/unit/traversing.js
View file
Expand Up @@ -152,7 +152,7 @@ test("not(Selector)", function() {
equals( jQuery("#main > p#ap > a").not("#google").length, 2, "not('selector')" ); equals( jQuery("#main > p#ap > a").not("#google").length, 2, "not('selector')" );
same( jQuery("p").not(".result").get(), q("firstp", "ap", "sndp", "en", "sap", "first"), "not('.class')" ); same( jQuery("p").not(".result").get(), q("firstp", "ap", "sndp", "en", "sap", "first"), "not('.class')" );
same( jQuery("p").not("#ap, #sndp, .result").get(), q("firstp", "en", "sap", "first"), "not('selector, selector')" ); same( jQuery("p").not("#ap, #sndp, .result").get(), q("firstp", "en", "sap", "first"), "not('selector, selector')" );
same( jQuery("#form option").not("option.emptyopt:contains('Nothing'),[selected],[value='1']").get(), q("option1c", "option1d", "option2c", "option3d", "option3e" ), "not('complex selector')"); same( jQuery("#form option").not("option.emptyopt:contains('Nothing'),[selected],[value='1']").get(), q("option1c", "option1d", "option2c", "option3d", "option3e", "option4e" ), "not('complex selector')");


same( jQuery('#ap *').not('code').get(), q("google", "groups", "anchor1", "mark"), "not('tag selector')" ); same( jQuery('#ap *').not('code').get(), q("google", "groups", "anchor1", "mark"), "not('tag selector')" );
same( jQuery('#ap *').not('code, #mark').get(), q("google", "groups", "anchor1"), "not('tag, ID selector')" ); same( jQuery('#ap *').not('code, #mark').get(), q("google", "groups", "anchor1"), "not('tag, ID selector')" );
Expand All @@ -163,7 +163,7 @@ test("not(Element)", function() {
expect(1); expect(1);


var selects = jQuery("#form select"); var selects = jQuery("#form select");
same( selects.not( selects[1] ).get(), q("select1", "select3"), "filter out DOM element"); same( selects.not( selects[1] ).get(), q("select1", "select3", "select4"), "filter out DOM element");
}); });


test("not(Function)", function() { test("not(Function)", function() {
Expand Down

0 comments on commit 2c4b208

Please sign in to comment.