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

KO 3.0.0 bug in IE8 - options/optionsCaption #1208

Merged
merged 1 commit into from Dec 11, 2013

Conversation

mbest
Copy link
Member

@mbest mbest commented Nov 22, 2013

Looking at upgrading our app to 3.0.0 but it is breaking IE8 for us.
I have a reproduction here http://jsbin.com/orEwOLAW/3/edit

What happens is that when I have a select element with both options and optionsCaption then I push more than one item in to the array, instead of the caption being chosen the first option is.

In my jsbin, if you change the KO version to 2.3.0, then the bug doesn't appear. Or if you comment out the second filterValues.push() the bug doesn't appear.

@crissdev
Copy link

@csainty if you change the code to push all the values at once then it works as expected.

viewModel.filterValues.push.apply(viewModel.filterValues, ["1", "2"]);

http://jsbin.com/orEwOLAW/4/edit

@csainty
Copy link
Author

csainty commented Nov 15, 2013

@crissdev An interesting observation. I haven't had time to try debug this yet but I am sure that will help pin-point it.
It doesn't help my usecase though unfortunately, the data is coming in async and needs to be added one at a time.

@mbest
Copy link
Member

mbest commented Nov 16, 2013

The problem appears to be related to the fact that the caption option is always replaced when the options binding updates. We have another issue open for that: #1081.

The IE-specific issue is that when the selected option is removed, whatever is the first option becomes selected immediately. And IE ignores later code that sets the selection to a different option. The IE-specific problem can be demonstrated without using the caption:

<select data-bind="options: filterValues, optionsText: 'x', optionsValue: 'x'"></select>
var viewModel = {
  filterValues: ko.observableArray([{x:1},{x:2}])
};

ko.applyBindings(viewModel);

viewModel.filterValues.splice(0, 1, {x:1});

http://jsbin.com/orEwOLAW/7/edit

@hipertracker
Copy link

Yes, pushing all data at once solves the problem (I am not sure about ko 3.0, but in ko 2.x it also improves the speed)

viewModel.filterValues(["1", "2"]);

http://jsbin.com/orEwOLAW/8/edit

@mbest
Copy link
Member

mbest commented Nov 27, 2013

My first thought to fix this was to deal with the selection aspect by using selectedIndex to select the right option. But that proved tricky to implement.

Then I realized that the problem stemmed from removing options. Whenever you remove the currently selected option, another one has to be selected. The bug in IE<=8 is that the option that's selected when removing options sticks even when options are added right away. By reversing the steps (adding, then removing), this bug doesn't cause any problems.

I'm using the beforeRemove callback to fix the problem because it's called after new items are added. If this ever changes, the new tests will catch the error. I'm also just using element.removeChild instead of ko.removeNode because the nodes will already have been cleaned by ko.cleanNode.

@SteveSanderson
Copy link
Contributor

Great catch and nice solution. Thanks!

SteveSanderson added a commit that referenced this pull request Dec 11, 2013
KO 3.0.0 bug in IE8 - options/optionsCaption
@SteveSanderson SteveSanderson merged commit 63914ad into master Dec 11, 2013
@mbest mbest deleted the 1208-ie8-options-selection-bug branch December 11, 2013 20:05
@mbest
Copy link
Member

mbest commented Jan 18, 2014

I just ran the test suite in Firefox, and the new test, "Binding: Options Should select first option when removing the selected option and the original first option." fails. I was able to get the test to pass by making the 'beforeRemove' version run on Firefox as well.

@mbest mbest mentioned this pull request Feb 11, 2014
@rniemeyer rniemeyer mentioned this pull request Feb 11, 2014
7 tasks
mbest added a commit that referenced this pull request Feb 12, 2014
…m with selection when deleting options first.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants