Merge pull request knockout#652 from SteveSanderson/601-fix-binding-d…

…ependencies-2

fix binding dependencies

spec/defaultBindings/foreachBehaviors.js

@@ -175,6 +175,116 @@ describe('Binding: Foreach', {
         value_of(testNode.childNodes[0]).should_contain_html('<span data-bind="text: childprop">first child</span><span data-bind="text: childprop">added child</span>');
     },
 
+    'Should call an afterRender callback function and not cause updates if an observable accessed in the callback is changed': function () {
+        testNode.innerHTML = "<div data-bind='foreach: { data: someItems, afterRender: callback }'><span data-bind='text: childprop'></span></div>";
+        var callbackObservable = ko.observable(1),
+            someItems = ko.observableArray([{ childprop: 'first child' }]),
+            callbacks = 0;
+        ko.applyBindings({ someItems: someItems, callback: function() { callbackObservable(); callbacks++; } }, testNode);
+        value_of(callbacks).should_be(1);
+
+        // Change the array, but don't update the observableArray so that the foreach binding isn't updated
+        someItems().push({ childprop: 'hidden child'});
+        value_of(testNode.childNodes[0]).should_contain_text('first child');
+        // Update callback observable and check that the binding wasn't updated
+        callbackObservable(2);
+        value_of(testNode.childNodes[0]).should_contain_text('first child');
+        // Update the observableArray and verify that the binding is now updated
+        someItems.valueHasMutated();
+        value_of(testNode.childNodes[0]).should_contain_text('first childhidden child');
+    },
+
+    'Should call an afterAdd callback function and not cause updates if an observable accessed in the callback is changed': function () {
+        testNode.innerHTML = "<div data-bind='foreach: { data: someItems, afterAdd: callback }'><span data-bind='text: childprop'></span></div>";
+        var callbackObservable = ko.observable(1),
+            someItems = ko.observableArray([]),
+            callbacks = 0;
+        ko.applyBindings({ someItems: someItems, callback: function() { callbackObservable(); callbacks++; } }, testNode);
+        someItems.push({ childprop: 'added child'});
+        value_of(callbacks).should_be(1);
+
+        // Change the array, but don't update the observableArray so that the foreach binding isn't updated
+        someItems().push({ childprop: 'hidden child'});
+        value_of(testNode.childNodes[0]).should_contain_text('added child');
+        // Update callback observable and check that the binding wasn't updated
+        callbackObservable(2);
+        value_of(testNode.childNodes[0]).should_contain_text('added child');
+        // Update the observableArray and verify that the binding is now updated
+        someItems.valueHasMutated();
+        value_of(testNode.childNodes[0]).should_contain_text('added childhidden child');
+    },
+
+    'Should call an beforeRemove callback function and not cause updates if an observable accessed in the callback is changed': function () {
+        testNode.innerHTML = "<div data-bind='foreach: { data: someItems, beforeRemove: callback }'><span data-bind='text: childprop'></span></div>";
+        var callbackObservable = ko.observable(1),
+            someItems = ko.observableArray([{ childprop: 'first child' }, { childprop: 'second child' }]),
+            callbacks = 0;
+        ko.applyBindings({ someItems: someItems, callback: function(elem) { callbackObservable(); callbacks++; ko.removeNode(elem); } }, testNode);
+        someItems.pop();
+        value_of(callbacks).should_be(1);
+
+        // Change the array, but don't update the observableArray so that the foreach binding isn't updated
+        someItems().push({ childprop: 'hidden child'});
+        value_of(testNode.childNodes[0]).should_contain_text('first child');
+        // Update callback observable and check that the binding wasn't updated
+        callbackObservable(2);
+        value_of(testNode.childNodes[0]).should_contain_text('first child');
+        // Update the observableArray and verify that the binding is now updated
+        someItems.valueHasMutated();
+        value_of(testNode.childNodes[0]).should_contain_text('first childhidden child');
+    },
+
+    'Should call an afterMove callback function and not cause updates if an observable accessed in the callback is changed': function () {
+        testNode.innerHTML = "<div data-bind='foreach: { data: someItems, afterMove: callback }'><span data-bind='text: childprop'></span></div>";
+        var callbackObservable = ko.observable(1),
+            someItems = ko.observableArray([{ childprop: 'first child' }]),
+            callbacks = 0;
+        ko.applyBindings({ someItems: someItems, callback: function() { callbackObservable(); callbacks++; } }, testNode);
+        someItems.splice(0, 0, { childprop: 'added child'});
+        value_of(callbacks).should_be(1);
+
+        // Change the array, but don't update the observableArray so that the foreach binding isn't updated
+        someItems().push({ childprop: 'hidden child'});
+        value_of(testNode.childNodes[0]).should_contain_text('added childfirst child');
+        // Update callback observable and check that the binding wasn't updated
+        callbackObservable(2);
+        value_of(testNode.childNodes[0]).should_contain_text('added childfirst child');
+        // Update the observableArray and verify that the binding is now updated
+        someItems.valueHasMutated();
+        value_of(testNode.childNodes[0]).should_contain_text('added childfirst childhidden child');
+    },
+
+    'Should call an beforeMove callback function and not cause updates if an observable accessed in the callback is changed': function () {
+        testNode.innerHTML = "<div data-bind='foreach: { data: someItems, beforeMove: callback }'><span data-bind='text: childprop'></span></div>";
+        var callbackObservable = ko.observable(1),
+            someItems = ko.observableArray([{ childprop: 'first child' }]),
+            callbacks = 0;
+        ko.applyBindings({ someItems: someItems, callback: function() { callbackObservable(); callbacks++; } }, testNode);
+        someItems.splice(0, 0, { childprop: 'added child'});
+        value_of(callbacks).should_be(1);
+
+        // Change the array, but don't update the observableArray so that the foreach binding isn't updated
+        someItems().push({ childprop: 'hidden child'});
+        value_of(testNode.childNodes[0]).should_contain_text('added childfirst child');
+        // Update callback observable and check that the binding wasn't updated
+        callbackObservable(2);
+        value_of(testNode.childNodes[0]).should_contain_text('added childfirst child');
+        // Update the observableArray and verify that the binding is now updated
+        someItems.valueHasMutated();
+        value_of(testNode.childNodes[0]).should_contain_text('added childfirst childhidden child');
+    },
+
+    'Should not double-unwrap the given value': function() {
+        // Previously an observable value was doubly-unwrapped: in the foreach handler and then in the template handler.
+        // This is now fixed so that the value is unwrapped just in the template handler and only peeked at in the foreach handler.
+        // See https://github.com/SteveSanderson/knockout/issues/523
+        testNode.innerHTML = "<div data-bind='foreach: myArray'><span data-bind='text: $data'></span></div>";
+        var myArrayWrapped = ko.observable(ko.observableArray(['data value']));
+        ko.applyBindings({ myArray: myArrayWrapped }, testNode);
+        // Because the unwrapped value isn't an array, nothing gets rendered.
+        value_of(testNode.childNodes[0]).should_contain_text('');
+    },
+
     'Should be able to nest foreaches and access binding contexts both during and after binding': function() {
         testNode.innerHTML = "<div data-bind='foreach: items'>"
                                 + "<div data-bind='foreach: children'>"

spec/editDetectionBehaviors.js

@@ -238,17 +238,30 @@ describe('Array to DOM node children mapping', {
         value_of(mappingInvocations).should_be([]);
         value_of(countCallbackInvocations).should_be(mappingInvocations.length);
 
+        // Check that observable items can be added and unwrapped in the mapping function and will update the DOM.
+        // Also check that observables accessed in the callback function do not update the DOM.
         mappingInvocations = [], countCallbackInvocations = 0;
-        var observable = ko.observable(1);
-        ko.utils.setDomNodeChildrenFromArrayMapping(testNode, [observable, null, "B"], mapping, null, callback); // Add to beginning; delete from end
+        var observable = ko.observable(1), callbackObservable = ko.observable(1);
+        var callback2 = function(arrayItem, nodes) {
+            callbackObservable();
+            callback(arrayItem, nodes);
+        };
+        ko.utils.setDomNodeChildrenFromArrayMapping(testNode, [observable, null, "B"], mapping, null, callback2); // Add to beginning; delete from end
         value_of(ko.utils.arrayMap(testNode.childNodes, function (x) { return x.innerHTML })).should_be(["1", "null", "B"]);
         value_of(mappingInvocations).should_be([observable, null]);
         value_of(countCallbackInvocations).should_be(mappingInvocations.length);
 
+        // Change the value of the mapped observable and verify that the DOM is updated
         mappingInvocations = [], countCallbackInvocations = 0;
-        observable(2);      // Change the value of the observable
+        observable(2);
         value_of(ko.utils.arrayMap(testNode.childNodes, function (x) { return x.innerHTML })).should_be(["2", "null", "B"]);
         value_of(mappingInvocations).should_be([observable]);
         value_of(countCallbackInvocations).should_be(mappingInvocations.length);
+
+        // Change the value of the callback observable and verify that the DOM wasn't updated
+        mappingInvocations = [], countCallbackInvocations = 0;
+        callbackObservable(2);
+        value_of(mappingInvocations.length).should_be(0);
+        value_of(countCallbackInvocations).should_be(0);
     }
 });

spec/templatingBehaviors.js

@@ -154,6 +154,24 @@ describe('Templating', {
         value_of(testNode.innerHTML).should_be("Value = B");
     },
 
+    'Should not rerender DOM element if observable accessed in \'afterRender\' callaback is changed': function () {
+        var observable = new ko.observable("A"), count = 0;
+        var myCallback = function(elementsArray, dataItem) {
+            observable();   // access observable in callback
+        };
+        var myTemplate = function() {
+            return "Value = " + (++count);
+        };
+        ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: myTemplate }));
+        ko.renderTemplate("someTemplate", {}, { afterRender: myCallback }, testNode);
+        value_of(testNode.childNodes.length).should_be(1);
+        value_of(testNode.innerHTML).should_be("Value = 1");
+
+        observable("B");
+        value_of(testNode.childNodes.length).should_be(1);
+        value_of(testNode.innerHTML).should_be("Value = 1");
+    },
+
     'If the supplied data item is observable, evaluates it and has subscription on it': function () {
         var observable = new ko.observable("A");
         ko.setTemplateEngine(new dummyTemplateEngine({ someTemplate: function (data) {

src/binding/defaultBindings/checked.js

@@ -10,11 +10,11 @@ ko.bindingHandlers['checked'] = {
                 return; // "checked" binding only responds to checkboxes and selected radio buttons
             }
 
-            var modelValue = valueAccessor();
-            if ((element.type == "checkbox") && (ko.utils.unwrapObservable(modelValue) instanceof Array)) {
+            var modelValue = valueAccessor(), unwrappedValue = ko.utils.unwrapObservable(modelValue);
+            if ((element.type == "checkbox") && (unwrappedValue instanceof Array)) {
                 // For checkboxes bound to an array, we add/remove the checkbox value to that array
                 // This works for both observable and non-observable arrays
-                var existingEntryIndex = ko.utils.arrayIndexOf(ko.utils.unwrapObservable(modelValue), element.value);
+                var existingEntryIndex = ko.utils.arrayIndexOf(unwrappedValue, element.value);
                 if (element.checked && (existingEntryIndex < 0))
                     modelValue.push(element.value);
                 else if ((!element.checked) && (existingEntryIndex >= 0))

src/binding/defaultBindings/foreach.js

@@ -3,20 +3,26 @@
 ko.bindingHandlers['foreach'] = {
     makeTemplateValueAccessor: function(valueAccessor) {
         return function() {
-            var bindingValue = ko.utils.unwrapObservable(valueAccessor());
+            var modelValue = valueAccessor(),
+                unwrappedValue = ko.utils.peekObservable(modelValue);    // Unwrap without setting a dependency here
 
-            // If bindingValue is the array, just pass it on its own
-            if ((!bindingValue) || typeof bindingValue.length == "number")
-                return { 'foreach': bindingValue, 'templateEngine': ko.nativeTemplateEngine.instance };
+            // If unwrappedValue is the array, pass in the wrapped value on its own
+            // The value will be unwrapped and tracked within the template binding
+            // (See https://github.com/SteveSanderson/knockout/issues/523)
+            if ((!unwrappedValue) || typeof unwrappedValue.length == "number")
+                return { 'foreach': modelValue, 'templateEngine': ko.nativeTemplateEngine.instance };
 
-            // If bindingValue.data is the array, preserve all relevant options
+            // If unwrappedValue.data is the array, preserve all relevant options and unwrap again value so we get updates
+            ko.utils.unwrapObservable(modelValue);
             return {
-                'foreach': bindingValue['data'],
-                'as': bindingValue['as'],
-                'includeDestroyed': bindingValue['includeDestroyed'],
-                'afterAdd': bindingValue['afterAdd'],
-                'beforeRemove': bindingValue['beforeRemove'],
-                'afterRender': bindingValue['afterRender'],
+                'foreach': unwrappedValue['data'],
+                'as': unwrappedValue['as'],
+                'includeDestroyed': unwrappedValue['includeDestroyed'],
+                'afterAdd': unwrappedValue['afterAdd'],
+                'beforeRemove': unwrappedValue['beforeRemove'],
+                'afterRender': unwrappedValue['afterRender'],
+                'beforeMove': unwrappedValue['beforeMove'],
+                'afterMove': unwrappedValue['afterMove'],
                 'templateEngine': ko.nativeTemplateEngine.instance
             };
         };

src/binding/defaultBindings/hasfocus.js

@@ -29,7 +29,7 @@ ko.bindingHandlers['hasfocus'] = {
         if (!element[hasfocusUpdatingProperty]) {
             var value = ko.utils.unwrapObservable(valueAccessor());
             value ? element.focus() : element.blur();
-            ko.utils.triggerEvent(element, value ? "focusin" : "focusout"); // For IE, which doesn't reliably fire "focus" or "blur" events synchronously
+            ko.dependencyDetection.ignore(ko.utils.triggerEvent, null, [element, value ? "focusin" : "focusout"]); // For IE, which doesn't reliably fire "focus" or "blur" events synchronously
         }
     }
 };

src/binding/defaultBindings/options.js

@@ -8,7 +8,7 @@ function ensureDropdownSelectionIsConsistentWithModelValue(element, modelValue,
     // If they aren't equal, either we prefer the dropdown value, or the model value couldn't be represented, so either way,
     // change the model value to match the dropdown.
     if (modelValue !== ko.selectExtensions.readValue(element))
-        ko.utils.triggerEvent(element, "change");
+        ko.dependencyDetection.ignore(ko.utils.triggerEvent, null, [element, "change"]);
 };
 
 ko.bindingHandlers['options'] = {
@@ -93,7 +93,7 @@ ko.bindingHandlers['options'] = {
                 // Ensure consistency between model value and selected option.
                 // If the dropdown is being populated for the first time here (or was otherwise previously empty),
                 // the dropdown selection state is meaningless, so we preserve the model value.
-                ensureDropdownSelectionIsConsistentWithModelValue(element, ko.utils.unwrapObservable(allBindings['value']), /* preferModelValue */ true);
+                ensureDropdownSelectionIsConsistentWithModelValue(element, ko.utils.peekObservable(allBindings['value']), /* preferModelValue */ true);
             }
 
             // Workaround for IE9 bug

src/binding/editDetection/arrayToDomNodeChildren.js

@@ -56,7 +56,7 @@
             if (mappedNodes.length > 0) {
                 ko.utils.replaceDomNodes(fixUpNodesToBeMovedOrRemoved(mappedNodes), newMappedNodes);
                 if (callbackAfterAddingNodes)
-                    callbackAfterAddingNodes(valueToMap, newMappedNodes, index);
+                    ko.dependencyDetection.ignore(callbackAfterAddingNodes, null, [valueToMap, newMappedNodes, index]);
             }
 
             // Replace the contents of the mappedNodes array, thereby updating the record
@@ -155,10 +155,6 @@
         // Call beforeMove first before any changes have been made to the DOM
         callCallback(options['beforeMove'], itemsForMoveCallbacks);
 
-        // Next remove nodes for deleted items; or call beforeRemove, which will remove them
-        ko.utils.arrayForEach(nodesToDelete, options['beforeRemove'] ? ko.cleanNode : ko.removeNode);
-        callCallback(options['beforeRemove'], itemsForBeforeRemoveCallbacks);
-
         // Next add/reorder the remaining items (will include deleted items if there's a beforeRemove callback)
         for (var i = 0, nextNode = ko.virtualElements.firstChild(domNode), lastNode, node; mapData = itemsToProcess[i]; i++) {
             // Get nodes for newly added items
@@ -178,6 +174,10 @@
             }
         }
 
+        // Next remove nodes for deleted items; or call beforeRemove, which will remove them
+        ko.utils.arrayForEach(nodesToDelete, options['beforeRemove'] ? ko.cleanNode : ko.removeNode);
+        callCallback(options['beforeRemove'], itemsForBeforeRemoveCallbacks);
+
         // Finally call afterMove and afterAdd callbacks
         callCallback(options['afterMove'], itemsForMoveCallbacks);
         callCallback(options['afterAdd'], itemsForAfterAddCallbacks);

src/binding/expressionRewriting.js

@@ -181,7 +181,7 @@ ko.expressionRewriting = (function () {
                 var propWriters = allBindingsAccessor()['_ko_property_writers'];
                 if (propWriters && propWriters[key])
                     propWriters[key](value);
-            } else if (!checkIfDifferent || property() !== value) {
+            } else if (!checkIfDifferent || property.peek() !== value) {
                 property(value);
             }
         }

src/subscribables/dependencyDetection.js

@@ -23,10 +23,10 @@ ko.dependencyDetection = (function () {
             }
         },
 
-        ignore: function(callback, callbackTarget) {
+        ignore: function(callback, callbackTarget, callbackArgs) {
             try {
                 _frames.push(null);
-                callback.call(callbackTarget);
+                return callback.apply(callbackTarget, callbackArgs || []);
             } finally {
                 _frames.pop();
             }