Merge pull request #2234 from knockout/2226-deferUpdates-if-binding-2

Descendant bindings of if, with, etc. should have a dependency on the parent condition
knockout · Jun 7, 2017 · 04a8f84 · 04a8f84
2 parents 1b73f8a + bb70d30
commit 04a8f84
Show file tree

Hide file tree

Showing 6 changed files with 110 additions and 49 deletions.
diff --git a/spec/asyncBindingBehaviors.js b/spec/asyncBindingBehaviors.js
@@ -152,23 +152,70 @@ describe("Deferred bindings", function() {
     });
 
     it('Should update "if" binding before descendant bindings', function() {
-        // Based on example at http://stackoverflow.com/q/43341484/1287183
-        testNode.innerHTML = '<div data-bind="if: hasAddress()"><div data-bind="text: address().street"></div></div>';
+        // Based on example at https://github.com/knockout/knockout/pull/2226
+        testNode.innerHTML = '<div data-bind="if: hasAddress()"><span data-bind="text: streetNumber().toLowerCase()"></span> <span data-bind="text: street().toLowerCase()"></span></div>';
         var vm = {
-            address: ko.observable(),
-            hasAddress: ko.pureComputed(function () { return vm.address() != null; })
+            street: ko.observable(),
+            streetNumber: ko.observable(),
+            hasAddress: ko.pureComputed(function () { return vm.streetNumber() && vm.street(); })
         };
 
         ko.applyBindings(vm, testNode);
         jasmine.Clock.tick(1);
-        expect(testNode.childNodes[0]).toContainHtml('');
+        expect(testNode.childNodes[0]).toContainText('');
 
-        vm.address({street: '123 my street'});
+        vm.street('my street');
+        vm.streetNumber('123');
         jasmine.Clock.tick(1);
-        expect(testNode.childNodes[0]).toContainHtml('<div data-bind="text: address().street">123 my street</div>');
+        expect(testNode.childNodes[0]).toContainText('123 my street');
 
-        vm.address(null);
+        vm.street(null);
+        vm.streetNumber(null);
         jasmine.Clock.tick(1);
-        expect(testNode.childNodes[0]).toContainHtml('');
+        expect(testNode.childNodes[0]).toContainText('');
     });
+
+    it('Should update "with" binding before descendant bindings', function() {
+        // Based on example at https://github.com/knockout/knockout/pull/2226
+        testNode.innerHTML = '<div data-bind="with: hasAddress()"><span data-bind="text: $parent.streetNumber().toLowerCase()"></span> <span data-bind="text: $parent.street().toLowerCase()"></span></div>';
+        var vm = {
+            street: ko.observable(),
+            streetNumber: ko.observable(),
+            hasAddress: ko.pureComputed(function () { return vm.streetNumber() && vm.street(); })
+        };
+
+        ko.applyBindings(vm, testNode);
+        jasmine.Clock.tick(1);
+        expect(testNode.childNodes[0]).toContainText('');
+
+        vm.street('my street');
+        vm.streetNumber('123');
+        jasmine.Clock.tick(1);
+        expect(testNode.childNodes[0]).toContainText('123 my street');
+
+        vm.street(null);
+        vm.streetNumber(null);
+        jasmine.Clock.tick(1);
+        expect(testNode.childNodes[0]).toContainText('');
+    });
+
+    it('Should leave descendant nodes unchanged if the value is truthy and remains truthy when changed', function() {
+        var someItem = ko.observable(true);
+        testNode.innerHTML = "<div data-bind='if: someItem'><span data-bind='text: (++counter)'></span></div>";
+        var originalNode = testNode.childNodes[0].childNodes[0];
+
+        // Value is initially true, so nodes are retained
+        ko.applyBindings({ someItem: someItem, counter: 0 }, testNode);
+        expect(testNode.childNodes[0].childNodes[0].tagName.toLowerCase()).toEqual("span");
+        expect(testNode.childNodes[0].childNodes[0]).toEqual(originalNode);
+        expect(testNode).toContainText("1");
+
+        // Change the value to a different truthy value; see the previous SPAN remains
+        someItem('different truthy value');
+        jasmine.Clock.tick(1);
+        expect(testNode.childNodes[0].childNodes[0].tagName.toLowerCase()).toEqual("span");
+        expect(testNode.childNodes[0].childNodes[0]).toEqual(originalNode);
+        expect(testNode).toContainText("1");
+    });
+
 });
diff --git a/spec/defaultBindings/ifBehaviors.js b/spec/defaultBindings/ifBehaviors.js
@@ -21,18 +21,20 @@ describe('Binding: If', function() {
 
     it('Should leave descendant nodes unchanged if the value is truthy and remains truthy when changed', function() {
         var someItem = ko.observable(true);
-        testNode.innerHTML = "<div data-bind='if: someItem'><span></span></div>";
+        testNode.innerHTML = "<div data-bind='if: someItem'><span data-bind='text: (++counter)'></span></div>";
         var originalNode = testNode.childNodes[0].childNodes[0];
 
         // Value is initially true, so nodes are retained
-        ko.applyBindings({ someItem: someItem }, testNode);
+        ko.applyBindings({ someItem: someItem, counter: 0 }, testNode);
         expect(testNode.childNodes[0].childNodes[0].tagName.toLowerCase()).toEqual("span");
         expect(testNode.childNodes[0].childNodes[0]).toEqual(originalNode);
+        expect(testNode).toContainText("1");
 
         // Change the value to a different truthy value; see the previous SPAN remains
         someItem('different truthy value');
         expect(testNode.childNodes[0].childNodes[0].tagName.toLowerCase()).toEqual("span");
         expect(testNode.childNodes[0].childNodes[0]).toEqual(originalNode);
+        expect(testNode).toContainText("1");
     });
 
     it('Should toggle the presence and bindedness of descendant nodes according to the truthiness of the value', function() {

diff --git a/spec/defaultBindings/withBehaviors.js b/spec/defaultBindings/withBehaviors.js
@@ -178,19 +178,25 @@ describe('Binding: With', function() {
     });
 
     it('Should provide access to an observable viewModel through $rawData', function() {
-        testNode.innerHTML = "<div data-bind='with: item'><input data-bind='value: $rawData'/></div>";
+        testNode.innerHTML = "<div data-bind='with: item'><input data-bind='value: $rawData'/><div data-bind='text: $data'></div></div>";
         var item = ko.observable('one');
         ko.applyBindings({ item: item }, testNode);
-        expect(item.getSubscriptionsCount('change')).toEqual(2);    // only subscriptions are the with and value bindings
+        expect(item.getSubscriptionsCount('change')).toEqual(3);    // subscriptions are the with and value bindings, and the binding context
         expect(testNode.childNodes[0]).toHaveValues(['one']);
+        expect(testNode.childNodes[0]).toContainText('one');
 
         // Should update observable when input is changed
         testNode.childNodes[0].childNodes[0].value = 'two';
         ko.utils.triggerEvent(testNode.childNodes[0].childNodes[0], "change");
         expect(item()).toEqual('two');
+        expect(testNode.childNodes[0]).toContainText('two');
 
         // Should update the input when the observable changes
         item('three');
         expect(testNode.childNodes[0]).toHaveValues(['three']);
+        expect(testNode.childNodes[0]).toContainText('three');
+
+        // subscription count is stable
+        expect(item.getSubscriptionsCount('change')).toEqual(3);
     });
 });
diff --git a/spec/lib/jasmine.extensions.js b/spec/lib/jasmine.extensions.js
@@ -77,7 +77,7 @@ jasmine.Matchers.prototype.toHaveTexts = function (expectedTexts) {
 };
 
 jasmine.Matchers.prototype.toHaveValues = function (expectedValues) {
-    var values = ko.utils.arrayMap(this.actual.childNodes, function (node) { return node.value; });
+    var values = ko.utils.arrayFilter(ko.utils.arrayMap(this.actual.childNodes, function (node) { return node.value; }), function (value) { return value !== undefined; });
     this.actual = values;   // Fix explanatory message
     return this.env.equals_(values, expectedValues);
 };

diff --git a/src/binding/bindingAttributeSyntax.js b/src/binding/bindingAttributeSyntax.js
@@ -18,6 +18,8 @@
         return ko.bindingHandlers[bindingKey];
     };
 
+    var inheritParentVm = {};
+
     // The ko.bindingContext constructor is only called directly to create the root context. For child
     // contexts, use bindingContext.createChildContext or bindingContext.extend.
     ko.bindingContext = function(dataItemOrAccessor, parentContext, dataItemAlias, extendCallback, options) {
@@ -30,7 +32,7 @@
             // we call the function to retrieve the view model. If the function accesses any observables or returns
             // an observable, the dependency is tracked, and those observables can later cause the binding
             // context to be updated.
-            var dataItemOrObservable = isFunc ? dataItemOrAccessor() : dataItemOrAccessor,
+            var dataItemOrObservable = isFunc ? realDataItemOrAccessor() : realDataItemOrAccessor,
                 dataItem = ko.utils.unwrapObservable(dataItemOrObservable);
 
             if (parentContext) {
@@ -53,8 +55,14 @@
                 // See https://github.com/SteveSanderson/knockout/issues/490
                 self['ko'] = ko;
             }
-            self['$rawData'] = dataItemOrObservable;
-            self['$data'] = dataItem;
+
+            if (shouldInheritData) {
+                dataItem = self['$data'];
+            } else {
+                self['$rawData'] = dataItemOrObservable;
+                self['$data'] = dataItem;
+            }
+
             if (dataItemAlias)
                 self[dataItemAlias] = dataItem;
 
@@ -71,7 +79,9 @@
         }
 
         var self = this,
-            isFunc = typeof(dataItemOrAccessor) == "function" && !ko.isObservable(dataItemOrAccessor),
+            shouldInheritData = dataItemOrAccessor === inheritParentVm,
+            realDataItemOrAccessor = shouldInheritData ? undefined : dataItemOrAccessor,
+            isFunc = typeof(realDataItemOrAccessor) == "function" && !ko.isObservable(realDataItemOrAccessor),
             nodes,
             subscribable;
 
@@ -136,10 +146,7 @@
     ko.bindingContext.prototype['extend'] = function(properties) {
         // If the parent context references an observable view model, "_subscribable" will always be the
         // latest view model object. If not, "_subscribable" isn't set, and we can use the static "$data" value.
-        return new ko.bindingContext(this._subscribable || this['$data'], this, null, function(self, parentContext) {
-            // This "child" context doesn't directly track a parent observable view model,
-            // so we need to manually set the $rawData value to match the parent.
-            self['$rawData'] = parentContext['$rawData'];
+        return new ko.bindingContext(inheritParentVm, this, null, function(self, parentContext) {
             ko.utils.extend(self, typeof(properties) == "function" ? properties() : properties);
         });
     };

diff --git a/src/binding/defaultBindings/ifIfnotWith.js b/src/binding/defaultBindings/ifIfnotWith.js
@@ -1,34 +1,37 @@
 // Makes a binding like with or if
-function makeWithIfBinding(bindingKey, isWith, isNot, makeContextCallback) {
+function makeWithIfBinding(bindingKey, isWith, isNot) {
     ko.bindingHandlers[bindingKey] = {
         'init': function(element, valueAccessor, allBindings, viewModel, bindingContext) {
-            var didDisplayOnLastUpdate,
-                savedNodes;
+            var savedNodes;
+            var ifCondition = !isWith && ko.computed(function() {
+                return !isNot !== !ko.utils.unwrapObservable(valueAccessor());
+            }, null, { disposeWhenNodeIsRemoved: element });
+
             ko.computed(function() {
-                var rawValue = valueAccessor(),
-                    dataValue = ko.utils.unwrapObservable(rawValue),
-                    shouldDisplay = !isNot !== !dataValue, // equivalent to isNot ? !dataValue : !!dataValue
-                    isFirstRender = !savedNodes,
-                    needsRefresh = isFirstRender || isWith || (shouldDisplay !== didDisplayOnLastUpdate);
+                var shouldDisplay = isWith ? !!ko.utils.unwrapObservable(valueAccessor()) : ifCondition(),
+                    isFirstRender = !savedNodes;
 
-                if (needsRefresh) {
-                    // Save a copy of the inner nodes on the initial update, but only if we have dependencies.
-                    if (isFirstRender && ko.computedContext.getDependenciesCount()) {
-                        savedNodes = ko.utils.cloneNodes(ko.virtualElements.childNodes(element), true /* shouldCleanNodes */);
-                    }
+                // Save a copy of the inner nodes on the initial update, but only if we have dependencies.
+                if (isFirstRender && ko.computedContext.getDependenciesCount()) {
+                    savedNodes = ko.utils.cloneNodes(ko.virtualElements.childNodes(element), true /* shouldCleanNodes */);
+                }
 
-                    if (shouldDisplay) {
-                        if (!isFirstRender) {
-                            ko.virtualElements.setDomNodeChildren(element, ko.utils.cloneNodes(savedNodes));
-                        }
-                        ko.applyBindingsToDescendants(makeContextCallback ? makeContextCallback(bindingContext, rawValue) : bindingContext, element);
-                    } else {
-                        ko.virtualElements.emptyNode(element);
+                if (shouldDisplay) {
+                    if (!isFirstRender) {
+                        ko.virtualElements.setDomNodeChildren(element, ko.utils.cloneNodes(savedNodes));
                     }
-
-                    didDisplayOnLastUpdate = shouldDisplay;
+                    ko.applyBindingsToDescendants(
+                        isWith ?
+                            bindingContext['createChildContext'](valueAccessor) :
+                            ifCondition.isActive() ?
+                                bindingContext['extend'](function() { ifCondition(); return null; }) :
+                                bindingContext,
+                        element);
+                } else {
+                    ko.virtualElements.emptyNode(element);
                 }
             }, null, { disposeWhenNodeIsRemoved: element });
+
             return { 'controlsDescendantBindings': true };
         }
     };
@@ -39,8 +42,4 @@ function makeWithIfBinding(bindingKey, isWith, isNot, makeContextCallback) {
 // Construct the actual binding handlers
 makeWithIfBinding('if');
 makeWithIfBinding('ifnot', false /* isWith */, true /* isNot */);
-makeWithIfBinding('with', true /* isWith */, false /* isNot */,
-    function(bindingContext, dataValue) {
-        return bindingContext.createStaticChildContext(dataValue);
-    }
-);
+makeWithIfBinding('with', true /* isWith */);