InheritBindingContext breaks identically named element.ref bindings in nested custom elements

[rytmis]

I'm submitting a bug report

• Library Version:
  aurelia-templating 1.3.0

Please tell us about your environment:

• Operating System:
  Windows 10

• Node Version:
  7.6.0

• NPM Version:
  4.1.2

• JSPM OR Webpack AND Version
  webpack 2.2.1

• Browser:
  Chrome 56

• Language:
  all

Current behavior:

If you have two nested custom elements that both have element.ref bindings with the same name, setting inheritBindingContext = true via processContent will break the ref binding on the inner element:

https://gist.run/?id=b31d1281caf4bddd4ddce2e47f0b993e

Switching inheritBindingContext to false makes it work:

https://gist.run/?id=241a6c39c28a1d4e84e65609884f6759

Expected/desired behavior:

• What is the expected behavior?

A workaround is to declare the target of the element reference as a settable property:

https://gist.run/?id=8ddc18eefde8169a9bf2000e11fa885a

I would expect that inheriting the binding context would not mess up refs in child elements.

• What is the motivation / use case for changing the behavior?

Consistency in ref binding behavior. The actual use case this came up in involves a combination of inheritBindingContext and a custom element that renders itself recursively (a tree node).

[rytmis]

I've been looking into this a bit.

I believe that the reason this happens is that in aurelia-binding, AccessScope.prototype.assign calls getContextFor which intentionally traverses the parent override context chain looking for a context that defines the named property.

I wrote the following failing test case for access-scope.spec.js:

  it('assigns value to bindingContext when parent has identical property name', () => {
    let scope = { bindingContext: { foo: undefined }, overrideContext: createOverrideContext({ foo: undefined }) };
    foo.assign(scope, 'baz');

    expect(scope.bindingContext.foo).toBe('baz');
  });

and was able to make it pass without breaking any additional tests by adding this after the if (ancestor) branch in getContextFor:

  if (scope.overrideContext && name in scope.overrideContext) {
    return scope.overrideContext;
  }
  if (scope.bindingContext && name in scope.bindingContext) {
    return scope.bindingContext;
  }

however, that feels very hacky and makes the flow of getContextFor really hard to understand. Also, I'm not sure the test case accurately represents the real-life error.

Intuitively, it seems to me that when assigning a property, it doesn't really make sense to traverse the chain upwards as long as there is a bindingContext or overrideContext in the current scope. I'd love to fix this in a way that makes sense and create a PR, but unfortunately I don't think I understand the moving parts well enough to do that.

[bigopon]

This is an expected behavior with binding system, as by default it look up from immediate binding context to root one and fallback to the root. To properly handle this scenario, simply predefine the property used for reference in the constructor, or before bind() lifecycle to any value:

export class ChildElement {
  this.theRef = undefined;  
}

working at https://gist.run/?id=53173d838af7ef9b65cecd8b6145fc67

@rytmis I'll close this, but please feel free to reopen if you see any issues.