Merge pull request #1208 from knockout/1208-ie8-options-selection-bug

KO 3.0.0 bug in IE8 - options/optionsCaption
knockout · Dec 11, 2013 · 63914ad · 63914ad
2 parents 262d8cc + 5efcd94
commit 63914ad
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 1 deletion.
diff --git a/spec/defaultBindings/optionsBehaviors.js b/spec/defaultBindings/optionsBehaviors.js
@@ -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>";

diff --git a/src/binding/defaultBindings/options.js b/src/binding/defaultBindings/options.js
@@ -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);
@@ -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.
@@ -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;