Skip to content

Commit

Permalink
Merge pull request #2148 from knockout/2148-contextfor-undefined-for-…
Browse files Browse the repository at this point in the history 
…unbound

dataFor / contextFor not honoring controlsDescendantBindings
  • Loading branch information
@mbest
mbest authored Apr 8, 2017
2 parents 65e7f0d + edd0823 commit ccec0fc
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 79 deletions.
63 changes: 46 additions & 17 deletions spec/bindingAttributeBehaviors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ describe('Binding attribute syntax', function() {
beforeEach(jasmine.prepareTestNode);

it('applyBindings should accept no parameters and then act on document.body with undefined model', function() {
this.after(function () { ko.utils.domData.clear(document.body); }); // Just to avoid interfering with other specs
this.after(function () { ko.cleanNode(document.body); }); // Just to avoid interfering with other specs

var didInit = false;
ko.bindingHandlers.test = {
init: function (element, valueAccessor, allBindings, viewModel) {
expect(element.id).toEqual("testElement");
expect(viewModel).toEqual(undefined);
expect(viewModel).toBeUndefined();
didInit = true;
}
};
Expand All @@ -18,7 +18,7 @@ describe('Binding attribute syntax', function() {
});

it('applyBindings should accept one parameter and then act on document.body with parameter as model', function() {
this.after(function () { ko.utils.domData.clear(document.body); }); // Just to avoid interfering with other specs
this.after(function () { ko.cleanNode(document.body); }); // Just to avoid interfering with other specs

var didInit = false;
var suppliedViewModel = {};
Expand Down Expand Up @@ -171,7 +171,7 @@ describe('Binding attribute syntax', function() {
init: function() { return { controlsDescendantBindings : true } }
};
ko.bindingHandlers.test2 = ko.bindingHandlers.test1;
testNode.innerHTML = "<div data-bind='test1: true, test2: true'></div>"
testNode.innerHTML = "<div data-bind='test1: true, test2: true'></div>";
expect(function () {
ko.applyBindings(null, testNode);
}).toThrowContaining("Multiple bindings (test1 and test2) are trying to control descendant bindings of the same element.");
Expand All @@ -195,7 +195,7 @@ describe('Binding attribute syntax', function() {
ko.applyBindings(vm, testNode);
expect(testNode).toContainText("my value");
expect(ko.contextFor(testNode.childNodes[0].childNodes[0].childNodes[0]).$customProp).toEqual("my value");
expect(ko.contextFor(testNode.childNodes[0].childNodes[0]).$customProp).toEqual(undefined); // Should not affect original binding context
expect(ko.contextFor(testNode.childNodes[0].childNodes[0]).$customProp).toBeUndefined(); // Should not affect original binding context

// vale of $data and $parent should be unchanged in extended context
expect(ko.contextFor(testNode.childNodes[0].childNodes[0].childNodes[0]).$data).toEqual(vm.sub);
Expand All @@ -221,8 +221,8 @@ describe('Binding attribute syntax', function() {
expect(testNode.childNodes[0].childNodes[0]).toContainText("Bert");

// Can't get binding context for unbound nodes
expect(ko.dataFor(testNode)).toEqual(undefined);
expect(ko.contextFor(testNode)).toEqual(undefined);
expect(ko.dataFor(testNode)).toBeUndefined();
expect(ko.contextFor(testNode)).toBeUndefined();

// Can get binding context for directly bound nodes
expect(ko.dataFor(testNode.childNodes[0]).name).toEqual("Bert");
Expand All @@ -231,10 +231,39 @@ describe('Binding attribute syntax', function() {
// Can get binding context for descendants of directly bound nodes
expect(ko.dataFor(testNode.childNodes[0].childNodes[0]).name).toEqual("Bert");
expect(ko.contextFor(testNode.childNodes[0].childNodes[0]).$data.name).toEqual("Bert");

// Also test that a non-node object returns nothing and doesn't crash
expect(ko.dataFor({})).toBeUndefined();
expect(ko.contextFor({})).toBeUndefined();
});

it('Should not return a context object for unbound elements that are descendants of bound elements', function() {
// From https://github.com/knockout/knockout/issues/2148
testNode.innerHTML = '<div data-bind="visible: isVisible"><span>Some text</span><div data-bind="allowBindings: false"><input data-bind="value: someValue"></div></div>';

ko.bindingHandlers.allowBindings = {
init: function(elem, valueAccessor) {
// Let bindings proceed as normal *only if* my value is false
var shouldAllowBindings = ko.unwrap(valueAccessor());
return { controlsDescendantBindings: !shouldAllowBindings };
}
};
var vm = {isVisible: true};
ko.applyBindings(vm, testNode);

// All of the bound nodes return the viewmodel
expect(ko.dataFor(testNode.childNodes[0])).toBe(vm);
expect(ko.dataFor(testNode.childNodes[0].childNodes[0])).toBe(vm);
expect(ko.dataFor(testNode.childNodes[0].childNodes[1])).toBe(vm);
expect(ko.contextFor(testNode.childNodes[0].childNodes[1]).$data).toBe(vm);

// The unbound child node returns undefined
expect(ko.dataFor(testNode.childNodes[0].childNodes[1].childNodes[0])).toBeUndefined();
expect(ko.contextFor(testNode.childNodes[0].childNodes[1].childNodes[0])).toBeUndefined();
});

it('Should not be allowed to use containerless binding syntax for bindings other than whitelisted ones', function() {
testNode.innerHTML = "Hello <!-- ko visible: false -->Some text<!-- /ko --> Goodbye"
testNode.innerHTML = "Hello <!-- ko visible: false -->Some text<!-- /ko --> Goodbye";
expect(function () {
ko.applyBindings(null, testNode);
}).toThrow("The binding 'visible' cannot be used with virtual elements");
Expand All @@ -253,7 +282,7 @@ describe('Binding attribute syntax', function() {
});

it('Should be allowed to express containerless bindings with arbitrary internal whitespace and newlines', function() {
testNode.innerHTML = "Hello <!-- ko\n" +
testNode.innerHTML = "Hello <!-- ko\n" +
" with\n" +
" : \n "+
" { \n" +
Expand All @@ -280,7 +309,7 @@ describe('Binding attribute syntax', function() {
};
ko.virtualElements.allowedBindings['test'] = true;

testNode.innerHTML = "Hello <!-- ko test: false -->Some text<!-- /ko --> Goodbye"
testNode.innerHTML = "Hello <!-- ko test: false -->Some text<!-- /ko --> Goodbye";
ko.applyBindings(null, testNode);

expect(countNodes).toEqual(1);
Expand All @@ -292,7 +321,7 @@ describe('Binding attribute syntax', function() {
ko.bindingHandlers.test = { init: function () { initCalls++ } };
ko.virtualElements.allowedBindings['test'] = true;

testNode.innerHTML = "Hello <!-- ko if: true --><!-- ko test: false -->Some text<!-- /ko --><!-- /ko --> Goodbye"
testNode.innerHTML = "Hello <!-- ko if: true --><!-- ko test: false -->Some text<!-- /ko --><!-- /ko --> Goodbye";
ko.applyBindings(null, testNode);

expect(initCalls).toEqual(1);
Expand All @@ -309,9 +338,9 @@ describe('Binding attribute syntax', function() {
});

it('Should automatically bind virtual descendants of containerless markers if no binding controlsDescendantBindings', function() {
testNode.innerHTML = "Hello <!-- ko dummy: false --><span data-bind='text: \"WasBound\"'>Some text</span><!-- /ko --> Goodbye";
ko.applyBindings(null, testNode);
expect(testNode).toContainText("Hello WasBound Goodbye");
testNode.innerHTML = "Hello <!-- ko dummy: false --><span data-bind='text: \"WasBound\"'>Some text</span><!-- /ko --> Goodbye";
ko.applyBindings(null, testNode);
expect(testNode).toContainText("Hello WasBound Goodbye");
});

it('Should be able to set and access correct context in custom containerless binding', function() {
Expand All @@ -324,7 +353,7 @@ describe('Binding attribute syntax', function() {
};
ko.virtualElements.allowedBindings['bindChildrenWithCustomContext'] = true;

testNode.innerHTML = "Hello <!-- ko bindChildrenWithCustomContext: true --><div>Some text</div><!-- /ko --> Goodbye"
testNode.innerHTML = "Hello <!-- ko bindChildrenWithCustomContext: true --><div>Some text</div><!-- /ko --> Goodbye";
ko.applyBindings(null, testNode);

expect(ko.dataFor(testNode.childNodes[2]).myCustomData).toEqual(123);
Expand All @@ -340,7 +369,7 @@ describe('Binding attribute syntax', function() {
}
};

testNode.innerHTML = "Hello <div data-bind='bindChildrenWithCustomContext: true'><!-- ko nonexistentHandler: 123 --><div>Some text</div><!-- /ko --></div> Goodbye"
testNode.innerHTML = "Hello <div data-bind='bindChildrenWithCustomContext: true'><!-- ko nonexistentHandler: 123 --><div>Some text</div><!-- /ko --></div> Goodbye";
ko.applyBindings(null, testNode);

expect(ko.dataFor(testNode.childNodes[1].childNodes[0]).myCustomData).toEqual(123);
Expand All @@ -357,7 +386,7 @@ describe('Binding attribute syntax', function() {
}
};

testNode.innerHTML = "Hello <div data-bind='bindChildrenWithCustomContext: true'><!-- ko with: myCustomData --><div>Some text</div><!-- /ko --></div> Goodbye"
testNode.innerHTML = "Hello <div data-bind='bindChildrenWithCustomContext: true'><!-- ko with: myCustomData --><div>Some text</div><!-- /ko --></div> Goodbye";
ko.applyBindings(null, testNode);

expect(ko.contextFor(testNode.childNodes[1].childNodes[0]).customValue).toEqual('xyz');
Expand Down
2 changes: 1 addition & 1 deletion spec/defaultBindings/foreachBehaviors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ describe('Binding: Foreach', function() {
var someItems = ko.observableArray([ 'A', 'B', 'C' ]),
callback = function(element, index, data) { if (data === 'D') throw "Exception"; };

ko.applyBindings({someItems: someItems, callback: callback });
ko.applyBindings({someItems: someItems, callback: callback }, testNode);
expect(testNode.childNodes[0]).toContainText('ABC');

expect(function() { someItems.push('D'); }).toThrow("Exception");
Expand Down
103 changes: 42 additions & 61 deletions src/binding/bindingAttributeSyntax.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,56 +194,51 @@
throw new Error("The binding '" + bindingName + "' cannot be used with virtual elements")
}

function applyBindingsToDescendantsInternal (bindingContext, elementOrVirtualElement, bindingContextsMayDifferFromDomParentElement) {
var currentChild,
nextInQueue = ko.virtualElements.firstChild(elementOrVirtualElement),
provider = ko.bindingProvider['instance'],
preprocessNode = provider['preprocessNode'];

// Preprocessing allows a binding provider to mutate a node before bindings are applied to it. For example it's
// possible to insert new siblings after it, and/or replace the node with a different one. This can be used to
// implement custom binding syntaxes, such as {{ value }} for string interpolation, or custom element types that
// trigger insertion of <template> contents at that point in the document.
if (preprocessNode) {
function applyBindingsToDescendantsInternal(bindingContext, elementOrVirtualElement) {
var nextInQueue = ko.virtualElements.firstChild(elementOrVirtualElement);

if (nextInQueue) {
var currentChild,
provider = ko.bindingProvider['instance'],
preprocessNode = provider['preprocessNode'];

// Preprocessing allows a binding provider to mutate a node before bindings are applied to it. For example it's
// possible to insert new siblings after it, and/or replace the node with a different one. This can be used to
// implement custom binding syntaxes, such as {{ value }} for string interpolation, or custom element types that
// trigger insertion of <template> contents at that point in the document.
if (preprocessNode) {
while (currentChild = nextInQueue) {
nextInQueue = ko.virtualElements.nextSibling(currentChild);
preprocessNode.call(provider, currentChild);
}
// Reset nextInQueue for the next loop
nextInQueue = ko.virtualElements.firstChild(elementOrVirtualElement);
}

while (currentChild = nextInQueue) {
// Keep a record of the next child *before* applying bindings, in case the binding removes the current child from its position
nextInQueue = ko.virtualElements.nextSibling(currentChild);
preprocessNode.call(provider, currentChild);
applyBindingsToNodeAndDescendantsInternal(bindingContext, currentChild);
}
// Reset nextInQueue for the next loop
nextInQueue = ko.virtualElements.firstChild(elementOrVirtualElement);
}

while (currentChild = nextInQueue) {
// Keep a record of the next child *before* applying bindings, in case the binding removes the current child from its position
nextInQueue = ko.virtualElements.nextSibling(currentChild);
applyBindingsToNodeAndDescendantsInternal(bindingContext, currentChild, bindingContextsMayDifferFromDomParentElement);
}
}

function applyBindingsToNodeAndDescendantsInternal (bindingContext, nodeVerified, bindingContextMayDifferFromDomParentElement) {
function applyBindingsToNodeAndDescendantsInternal(bindingContext, nodeVerified) {
var shouldBindDescendants = true;

// Perf optimisation: Apply bindings only if...
// (1) We need to store the binding context on this node (because it may differ from the DOM parent node's binding context)
// Note that we can't store binding contexts on non-elements (e.g., text nodes), as IE doesn't allow expando properties for those
// (2) It might have bindings (e.g., it has a data-bind attribute, or it's a marker for a containerless template)
var isElement = (nodeVerified.nodeType === 1);
if (isElement) // Workaround IE <= 8 HTML parsing weirdness
ko.virtualElements.normaliseVirtualElementDomStructure(nodeVerified);

var shouldApplyBindings = (isElement && bindingContextMayDifferFromDomParentElement) // Case (1)
|| ko.bindingProvider['instance']['nodeHasBindings'](nodeVerified); // Case (2)
// Perf optimisation: Apply bindings only if...
// (1) We need to store the binding info for the node (all element nodes)
// (2) It might have bindings (e.g., it has a data-bind attribute, or it's a marker for a containerless template)
var shouldApplyBindings = isElement || ko.bindingProvider['instance']['nodeHasBindings'](nodeVerified);
if (shouldApplyBindings)
shouldBindDescendants = applyBindingsToNodeInternal(nodeVerified, null, bindingContext, bindingContextMayDifferFromDomParentElement)['shouldBindDescendants'];
shouldBindDescendants = applyBindingsToNodeInternal(nodeVerified, null, bindingContext)['shouldBindDescendants'];

if (shouldBindDescendants && !bindingDoesNotRecurseIntoElementTypes[ko.utils.tagNameLower(nodeVerified)]) {
// We're recursing automatically into (real or virtual) child nodes without changing binding contexts. So,
// * For children of a *real* element, the binding context is certainly the same as on their DOM .parentNode,
// hence bindingContextsMayDifferFromDomParentElement is false
// * For children of a *virtual* element, we can't be sure. Evaluating .parentNode on those children may
// skip over any number of intermediate virtual elements, any of which might define a custom binding context,
// hence bindingContextsMayDifferFromDomParentElement is true
applyBindingsToDescendantsInternal(bindingContext, nodeVerified, /* bindingContextsMayDifferFromDomParentElement: */ !isElement);
applyBindingsToDescendantsInternal(bindingContext, nodeVerified);
}
}

Expand Down Expand Up @@ -283,21 +278,18 @@
return result;
}

function applyBindingsToNodeInternal(node, sourceBindings, bindingContext, bindingContextMayDifferFromDomParentElement) {
function applyBindingsToNodeInternal(node, sourceBindings, bindingContext) {
// Prevent multiple applyBindings calls for the same node, except when a binding value is specified
var alreadyBound = ko.utils.domData.get(node, boundElementDomDataKey);
var bindingInfo = ko.utils.domData.get(node, boundElementDomDataKey);
if (!sourceBindings) {
if (alreadyBound) {
if (bindingInfo) {
throw Error("You cannot apply bindings multiple times to the same element.");
}
ko.utils.domData.set(node, boundElementDomDataKey, true);
}

// Optimization: Don't store the binding context on this node if it's definitely the same as on node.parentNode, because
// we can easily recover it just by scanning up the node's ancestors in the DOM
// (note: here, parent node means "real DOM parent" not "virtual parent", as there's no O(1) way to find the virtual parent)
if (!alreadyBound && bindingContextMayDifferFromDomParentElement)
ko.storedBindingContextForNode(node, bindingContext);
ko.utils.domData.set(node, boundElementDomDataKey, {context: bindingContext});
if (bindingContext._subscribable)
bindingContext._subscribable._addNode(node);
}

// Use bindings if given, otherwise fall back on asking the bindings provider to give us some bindings
var bindings;
Expand Down Expand Up @@ -402,15 +394,9 @@
};
};

var storedBindingContextDomDataKey = ko.utils.domData.nextKey();
ko.storedBindingContextForNode = function (node, bindingContext) {
if (arguments.length == 2) {
ko.utils.domData.set(node, storedBindingContextDomDataKey, bindingContext);
if (bindingContext._subscribable)
bindingContext._subscribable._addNode(node);
} else {
return ko.utils.domData.get(node, storedBindingContextDomDataKey);
}
ko.storedBindingContextForNode = function (node) {
var bindingInfo = ko.utils.domData.get(node, boundElementDomDataKey);
return bindingInfo && bindingInfo.context;
}

function getBindingContext(viewModelOrBindingContext) {
Expand Down Expand Up @@ -457,13 +443,8 @@
// Retrieving binding context from arbitrary nodes
ko.contextFor = function(node) {
// We can only do something meaningful for elements and comment nodes (in particular, not text nodes, as IE can't store domdata for them)
switch (node.nodeType) {
case 1:
case 8:
var context = ko.storedBindingContextForNode(node);
if (context) return context;
if (node.parentNode) return ko.contextFor(node.parentNode);
break;
if (node && (node.nodeType === 1 || node.nodeType === 8)) {
return ko.storedBindingContextForNode(node);
}
return undefined;
};
Expand Down

0 comments on commit ccec0fc

Please sign in to comment.