Skip to content

Attributes: fix setting selected on an option in IE<=11 #2840

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
Closed
wants to merge 5 commits into from
Closed

Attributes: fix setting selected on an option in IE<=11 #2840

wants to merge 5 commits into from

Conversation

timmywil
Copy link
Member

  • Same as the getter fix. All that was required was accessing the selectedIndex property.
  • Also added support for optgroups

Fixes gh-2732

mgol
mgol reviewed Jan 15, 2016
View reviewed changes
src/attributes/prop.js
@@ -79,6 +79,12 @@ jQuery.extend( {
}
} );

// Support: IE<=11
Copy link
Member

Choose a reason for hiding this comment

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

What about Edge? I'd love all new IE-specific stuff to be tested on Edge and if the bug is not present there to write IE <= 11 only.

Adding spaces around <= would be good as well, that's where the conventions seem to go currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Edge is ok. Updated comment, but the syntax says there's no space after the <=, so it's

IE <=11 only.

@gibson042
Copy link
Member

Can the get and set bodies be made even more similar? gzip will love you...

@dmethvin
Copy link
Member

Going even further, can get and set use the same method?

@timmywil
Copy link
Member Author

Going even further, can get and set use the same method?

I suppose they can, but notice that the getter doesn't access selectedIndex on the select unless there's an optgroup. That said, if we're okay with it, I'll just make the getter and setter the same function. I don't think it'll hurt anything to go back to accessing selectedIndex twice in the getter.

@dmethvin
Copy link
Member

Yeah, I doubt an extra access will make that much difference. Since it's already guarded by a support test, only IE will pay that penalty anyway.

@timmywil
Copy link
Member Author

Actually, they can't be the same function. The getter needs to return null and the setter needs to return undefined. I tried a factory to create similar functions, but that was +15 bytes. Even using the same function (even though it wouldn't work right) was +4 bytes. Just for kicks, I tried making the getter identical to the setter – with the extra return statement, and it was exactly the same. I think what was there is still the best option.

@timmywil timmywil closed this in 780cac8 Jan 19, 2016
@timmywil timmywil deleted the selected branch January 19, 2016 16:37
timmywil added a commit that referenced this pull request Mar 7, 2016
@timmywil
Attributes: fix setting selected on an option in IE<=11
67d4aeb 
Fixes gh-2732
Close gh-2840
timmywil added a commit that referenced this pull request Mar 7, 2016
@timmywil
Attributes: fix setting selected on an option in IE<=11
b188947 
Fixes gh-2732
Close gh-2840
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

.prop("selected", true) does not work as expected in IE11
5 participants
@timmywil @gibson042 @dmethvin @mgol @jquerybot