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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 28 additions & 0 deletions spec/defaultBindings/optionsBehaviors.js
Expand Up @@ -96,6 +96,34 @@ describe('Binding: Options', function() {
expect(testNode.childNodes[0]).toHaveSelectedValues(["B"]);
});

it('Should select first option when removing the selected option and the original first option', function () {
// This test failed in IE<=8 without changes made in #1208
testNode.innerHTML = '<select data-bind="options: filterValues, optionsText: \'x\', optionsValue: \'x\'">';
var viewModel = {
filterValues: ko.observableArray([{x:1},{x:2},{x:3}])
};
ko.applyBindings(viewModel, testNode);
testNode.childNodes[0].options[1].selected = true;
expect(testNode.childNodes[0]).toHaveSelectedValues([2]);

viewModel.filterValues.splice(0, 2, {x:4});
expect(testNode.childNodes[0]).toHaveSelectedValues([4]);
});

it('Should select caption by default and retain selection when adding multiple items', function () {
// This test failed in IE<=8 without changes made in #1208
testNode.innerHTML = '<select data-bind="options: filterValues, optionsCaption: \'foo\'">';
var viewModel = {
filterValues: ko.observableArray()
};
ko.applyBindings(viewModel, testNode);
expect(testNode.childNodes[0]).toHaveSelectedValues([undefined]);

viewModel.filterValues.push("1");
viewModel.filterValues.push("2");
expect(testNode.childNodes[0]).toHaveSelectedValues([undefined]);
});

it('Should trigger a change event when the options selection is populated or changed by modifying the options data (single select)', function() {
var observable = new ko.observableArray(["A", "B", "C"]), changeHandlerFireCount = 0;
testNode.innerHTML = "<select data-bind='options:myValues'></select>";
Expand Down
13 changes: 12 additions & 1 deletion src/binding/defaultBindings/options.js
Expand Up @@ -21,8 +21,10 @@ ko.bindingHandlers['options'] = {

var unwrappedArray = ko.utils.unwrapObservable(valueAccessor());
var includeDestroyed = allBindings.get('optionsIncludeDestroyed');
var arrayToDomNodeChildrenOptions = {};
var captionPlaceholder = {};
var captionValue;

var previousSelectedValues;
if (element.multiple) {
previousSelectedValues = ko.utils.arrayMap(selectedOptions(), ko.selectExtensions.readValue);
Expand Down Expand Up @@ -88,6 +90,15 @@ ko.bindingHandlers['options'] = {
return [option];
}

// By using a beforeRemove callback, we delay the removal until after new items are added. This fixes a selection
// problem in IE<=8. See https://github.com/knockout/knockout/issues/1208
if (ko.utils.ieVersion <= 8) {
arrayToDomNodeChildrenOptions['beforeRemove'] =
function (option) {
element.removeChild(option);
};
}

function setSelectionCallback(arrayEntry, newOptions) {
// IE6 doesn't like us to assign selection to OPTION nodes before they're added to the document.
// That's why we first added them without selection. Now it's time to set the selection.
Expand All @@ -109,7 +120,7 @@ ko.bindingHandlers['options'] = {
}
}

ko.utils.setDomNodeChildrenFromArrayMapping(element, filteredArray, optionForArrayItem, null, callback);
ko.utils.setDomNodeChildrenFromArrayMapping(element, filteredArray, optionForArrayItem, arrayToDomNodeChildrenOptions, callback);

// Determine if the selection has changed as a result of updating the options list
var selectionChanged;
Expand Down