Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix for #13514.Set selectedIdx to 0 if a 'wrong' value is set as dropdown-list value #1191

Closed
wants to merge 12 commits into from
Closed

Fix for #13514.Set selectedIdx to 0 if a 'wrong' value is set as dropdown-list value #1191

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
13860db
Set selectedIdx to 0 if a 'wrong' value is set as dropdown-list value…
ruado1987 Mar 3, 2013
63174e0
Merge remote-tracking branch 'upstream/master' into bug_13514
ruado1987 Mar 4, 2013
df7daa3
Merge branch 'master', remote-tracking branch 'upstream' into bug_13514
ruado1987 Mar 7, 2013
8805f64
Preserve the behavior for select-multiple while fixing this issue
ruado1987 Mar 9, 2013
646b498
Merge remote-tracking branch 'upstream/master' into bug_13514
ruado1987 Mar 12, 2013
8fd6479
force IE9 to behave like other browsers when a non-matching value is …
ruado1987 Mar 13, 2013
1d7d830
force IE9 to behave like other browsers when a non-matching value is …
ruado1987 Mar 13, 2013
bb3eb30
Merge branch 'bug_13514' of github.com:ruado1987/jquery into bug_13514
ruado1987 Mar 13, 2013
c293a60
force IE9 to behave like other browsers when a non-matching value is …
ruado1987 Mar 13, 2013
a2f7906
Merge remote-tracking branch 'upstream/master' into bug_13514
ruado1987 Mar 13, 2013
72ede71
Merge branch 'bug_13514' of github.com:ruado1987/jquery into bug_13514
ruado1987 Mar 13, 2013
30e2e4b
Set selectedIdx to -1 when non-matching value is set on select-one an…
ruado1987 Mar 22, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/attributes.js
View file
Open in desktop
Expand Up @@ -280,6 +280,12 @@ jQuery.extend({

if ( !values.length ) {
elem.selectedIndex = -1;
} else {
// IE 9 doesn't select a dropdown-list's first option
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be Support: IE9. is IE7/8/10 also affected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I haven't had a chance to check the issue on these ie versions. Anyway, I will try to check it out today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, you also meant the comment needs to be changed to Suport: IE9 ..., right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staabm: this issue doesnt affect IE7/8, I havent had a chance to test it on IE10 though.

// when value doesnt match with one of its options' value
if ( elem.selectedIndex < 0 ) {
elem.selectedIndex = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work correctly when elem is a select-multiple. Also, I'm concerned about this edge case fix requiring too much additional code, but that could be mitigated somewhat by templating off the get method. I'd try starting with

var optionSet,
    values = jQuery.makeArray( value ),
    options = elem.options,
    index = elem.selectedIndex,
    one = elem.type === "select-one" || index < 0;

and proceed from there, making sure to add thorough unit tests for both select-one and select-multiple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is not occurring to select-multiple. Do you mean that the code has to be changed so that calling val() with a 'wrong' option value(s) on select-multiple will also select the first option? Currently no option will be selected when passing a 'wrong' value to val() in case of select-multiple.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observe the change in behavior from introducing this code: http://jsfiddle.net/wuM8f/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My latest changes: http://jsfiddle.net/aVjyH/1/ to preserve the current behavior when elem is a select-multiple. The new code changes elem.selectedIndex only if elem.type is "select-multiple" and elem.selectedIndex is less than 0. The approach used in get doesn't work well when applying to set because one = elem.type === "select-one" || index < 0 can be true even when elem is a select-multiple and none of its options is selected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right of course; we slip it in there because a select-multiple with nothing selected can follow the single-get null case. Regardless, http://jsfiddle.net/aVjyH/1/ is too much code, and will compress poorly. Seriously, follow along these lines:

var optionSet,
    values = jQuery.makeArray( value ),
    options = elem.options,
    index = elem.selectedIndex,
    // Keep as a variable only if overall gzip size is smaller
    // than using elem.type !== "select-one" directly in an if condition
    one = elem.type === "select-one";

and try using the loop body instead of selectedIndex to track if post-iteration fixup is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following your advice, I've come up with this change. However, I don't use the index variable because the variable is not useful in determining if post-iteration fixup is necessary. I also use elem.type === "select-one" directly in the if condition since it results in a bit smaller file than introducing the one variable.

}
}
return values;
}
Expand Down
5 changes: 4 additions & 1 deletion test/unit/attributes.js
View file
Open in desktop
Expand Up @@ -768,7 +768,7 @@ test( "removeProp(String)", function() {
});

test( "val()", function() {
expect( 21 + ( jQuery.fn.serialize ? 6 : 0 ) );
expect( 22 + ( jQuery.fn.serialize ? 6 : 0 ) );

document.getElementById("text1").value = "bla";
equal( jQuery("#text1").val(), "bla", "Check for modified value of input element" );
Expand Down Expand Up @@ -844,6 +844,9 @@ test( "val()", function() {
equal( $button.val("baz").html(), "text", "Setting the value does not change innerHTML" );

equal( jQuery("<option/>").val("test").attr("value"), "test", "Setting value sets the value attribute" );

jQuery("#select5").val( "" );
equal( jQuery("#select5").val(), "3", "IE9 should select the first option if empty string is set as its dropdown-list's value" );
});

if ( "value" in document.createElement("meter") &&
Expand Down