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.

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

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

Conversation

Projects
None yet
6 participants
@ruado1987
Copy link
Contributor

commented Mar 3, 2013

This commit fixes the issue described in the bug ticket when a dropdown-list is set to a value which doesnt match with any of its options' value.

@@ -280,6 +280,12 @@ jQuery.extend({

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

This comment has been minimized.

Copy link
@staabm

staabm Mar 3, 2013

Contributor

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

This comment has been minimized.

Copy link
@ruado1987

ruado1987 Mar 4, 2013

Author Contributor

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

This comment has been minimized.

Copy link
@ruado1987

ruado1987 Mar 4, 2013

Author Contributor

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

This comment has been minimized.

Copy link
@staabm

staabm Mar 4, 2013

Contributor

Yes

This comment has been minimized.

Copy link
@ruado1987

ruado1987 Mar 4, 2013

Author Contributor

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

@ruado1987

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2013

@dmethvin @gibson042 : Please review this fix and give away your advice. Thanks.

// IE 9 doesn't select a dropdown-list's first option
// when value doesnt match with one of its options' value
if ( elem.selectedIndex < 0 ) {
elem.selectedIndex = 0;

This comment has been minimized.

Copy link
@gibson042

gibson042 Mar 6, 2013

Member

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.

This comment has been minimized.

Copy link
@ruado1987

ruado1987 Mar 7, 2013

Author Contributor

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.

This comment has been minimized.

Copy link
@gibson042

gibson042 Mar 7, 2013

Member

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

This comment has been minimized.

Copy link
@ruado1987

ruado1987 Mar 8, 2013

Author Contributor

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.

This comment has been minimized.

Copy link
@gibson042

gibson042 Mar 8, 2013

Member

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.

This comment has been minimized.

Copy link
@ruado1987

ruado1987 Mar 9, 2013

Author Contributor

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.

});

if ( !values.length ) {
elem.selectedIndex = -1;
} else {
// fix IE9 bug when non-matching value is set
if ( elem.type === "select-one" && !optionSet ) {

This comment has been minimized.

Copy link
@gibson042

gibson042 Mar 11, 2013

Member

What file are you using to verify size changes? Our standard metric is jquery.min.js.gz, and you'll be able to see it directly from grunt if you rebase onto 91d5764. I'm certain, for instance, that you'll achieve better compression by declaring options third rather than first and skipping the initial assignment to optionSet. There are even greater savings to be had by abandoning jQuery.each in favor of native iteration.

Also, looking at this with fresh eyes, I'm pretty sure you can combine the two tests here and just set elem.selectedIndex = -1 whenever optionSet is falsy.

This comment has been minimized.

Copy link
@ruado1987

ruado1987 Mar 12, 2013

Author Contributor

I had verified size changes using the generated minified file before you pushed the latest changes regarding to the grunt-compare-size. Just pulled the latest changes and started using this plugin for my verification. You are also right when saying native iteration will result in better performance than jQuery.each. Actually the former was what I wanted to use, but I thought it would be better to follow the old code to use jQuery's own utilities. Anyway, I will change the code to use native iteration. As for the last comment, I don't really get what you mean. Are you saying that it is better to remove elem.type === "select-one" and keep only !optionSet in the condition tests?

This comment has been minimized.

Copy link
@gibson042

gibson042 Mar 12, 2013

Member

Yes; assuming the following works (and if it doesn't then $selectMultiple.val([]) etc. are likely broken), I would consider it better than the if/else:

if ( !optionSet ) {
    elem.selectedIndex = -1;
}

The remainder is then mostly optimizing for compression; e.g.:

var optionSet, option,
    options = elem.options,
    i = options.length,
    values = jQuery.makeArray( value );

while ( i-- ) {
    option = options[ i ];
    if ( (option.selected = jQuery.inArray( jQuery(option).val(), values ) >= 0) ) {
        optionSet = true;
    }
}
@ruado1987

This comment has been minimized.

Copy link
Owner Author

commented on src/attributes.js in 8fd6479 Mar 13, 2013

@gibson042: Moving the assignment of i to the for loop results in a smaller gz file (29476 vs. 29480).

@ruado1987

This comment has been minimized.

Copy link
Owner Author

commented on src/attributes.js in 8fd6479 Mar 13, 2013

@gibson042: This issue wont be fixed if the code is just elem.selectedIndex = -1. It is still required to set elem.selectedIndex to 0 if the element is a select one.

@gibson042

This comment has been minimized.

Copy link
Member

commented Mar 13, 2013

Really? I observe consistent behavior on all browsers: http://jsbin.com/ikovev/1/

At this point, it's a question of what consistency is most appropriate. Nonexistent values are not documented as valid input, so this is an opportunity to define behavior. Implementing "value not found" as selectedIndex = -1 (and subsequent null .val()) seems to me like an improvement over selectedIndex = 0 (and subsequent non-null .val()), but this is now sufficiently vague that I'll put out the @dmethvin​-shaped signal light and we'll try to establish a consensus.

@gibson042

This comment has been minimized.

Copy link
Member

commented Mar 13, 2013

HTML5 language:

On setting, the selectedIndex attribute must set the selectedness of all the option elements in the list of options to false, and then the option element in the list of options whose index is the given new value, if any, must have its selectedness set to true.
This can result in no element having a selectedness set to true even in the case of the select element having no multiple attribute and a display size of 1.

@ruado1987

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2013

@gibson042 By "won't be fixed", I mean IE9's behavior still remains the same as described in ticket #13514. According to the HTML5 spec on this regard, I think IE9 behavior is the most appropriate though. Anyway, let's wait for @dmethvin decision.

@ruado1987

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2013

@gibson042 Is there any update on this issue? It has been idle for a week.

@gibson042

This comment has been minimized.

Copy link
Member

commented Mar 20, 2013

@dmethvin ping.

I strongly prefer the first, but as stated above, this is undefined behavior for jQuery right now:

  1. "Value not found" sets selectedIndex = -1. Subsequent .val() returns null.
  2. "Value not found" sets selectedIndex = 0 (more technically, to the lowest selectedIndex identifying a non-disabled <option>). Subsequent .val() returns the corresponding value.

(HTML5 link)

@dmethvin

This comment has been minimized.

Copy link
Member

commented Mar 20, 2013

I remember looking at this and my head started hurting. Same effect this week. Us defining behavior is always dangerous.

If a select has no initial option selected, that's undefined behavior according to HTML4 but nearly all browsers handle that case by choosing the first option I think. I couldn't see the similar language in the HTML5 case.

So option 1 provides functionality that isn't possible through the normal declarative markup. How cross-browser is it? In HTML5 if no option has its selectness, does that imply the spec is preventing the browser from displaying one as a default? If so this seems to contradict the HTML parsing scenario.

@gibson042

This comment has been minimized.

Copy link
Member

commented Mar 21, 2013

You're right that HTML4 leaves it undefined, but HTML5 specifies option 1, at least programatically:

Note: [setting selectedIndex] can result in no element having a selectedness set to true even in the case of the select element having no multiple attribute and a display size of 1.

It's clear that setting selectedIndex to -1 should result in all options having false selectedness, and that getting selectedIndex in such a state should return -1.

The display is unspecified, but all browsers that I tested blank out the select— including Android, iOS, and IE6+. We've previously taken a firm stance that rendering issues are not our problem, and I don't see any reason to break that now.

@dmethvin

This comment has been minimized.

Copy link
Member

commented Mar 21, 2013

Okay, I'm good with option 1 then. Let's do it.

@ruado1987

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2013

@gibson042 @dmethvin : Done. Please review the latest changes.

@gibson042 gibson042 closed this in c9ca9bf Mar 22, 2013

gibson042 added a commit that referenced this pull request Mar 22, 2013

@gibson042

This comment has been minimized.

Copy link
Member

commented Mar 22, 2013

Thanks very much!

@billynoah

This comment has been minimized.

Copy link

commented Jan 19, 2017

Applications everywhere used $('select').val(''); to unset the selected option and reset a form. This now results in a blank option appearing and has introduced bugs across dozens of projects when I upgraded to jQuery 1.12. It's not that this new behavior is wrong, but many applications are now broken because of this change and it was not even mentioned in the changelog.

@mgol

This comment has been minimized.

Copy link
Member

commented Jan 19, 2017

@billynoah Unfortunately, it's too late now to change it; we're no longer maintaining jQuery 1.x/2.x and even if we were, changing that back would be a breaking change now and could break people using jQuery 1.12 and expecting the new behavior.

@billynoah

This comment has been minimized.

Copy link

commented Jan 19, 2017

@mgol - Yes I get that, just taking time out to gripe :-) I don't think this change was a good one, but I agree it's too late.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.