fix for #11343 - bindToController for multiple directives

[jtorbicki]

Second attempt due to merge conflicts :) Fix for issue #11343

[pkozlowski-opensource]

@jtorbicki could you please add test(s) for this as well? Thnx!

[jtorbicki]

Test added

[caitp]

technically, you're not supposed to have multiple "new scope" directives on the same element, so this doesn't really work in practice (if it does work, it's a bug!).

I'm a bit uncomfortable with this because, given the above, it could become unclear what's happening if you have directives which implicitly depend on there being a new scope directive on the same element to work correctly.

[jtorbicki]

@caitp This is not what the issue is about. The "scope: true" functionality creating the common scope that's being shared among the directives is already in the Angular. Those directives can interact with one another by reading and writing to the scope directly without the need of bindToController. Whether this is good or not is probably up for another discussion. This issue is about bindToController binding the data for only single directive leaving all others in the undefined state. That's obviously not correct. It should either bind for all, or bindToController should be disabled for "scope: true".
On a side note: In my view there is nothing wrong with multiple directives sharing the same common scope, especially with the bindToController option that binds the data for different controllers in all directives, so there is no danger of polluting the scope.

[caitp]

Believe it or not, I understand what this issue is about --- The problem is the "scope: true" behaviour being shared by multiple directives on the same element. This was never meant to be supported, there is a WONTFIX'd bug about this, because it's explicitly not supported, it's just that we fail to throw the error sometimes (depending on types of scopes used, and directive priorities). This is never something that should be depended on

[petebacondarwin]

We do support multiple directives asking for new normal scope (i.e. not isolate) as can be seen in the docs (https://docs.angularjs.org/api/ng/service/$compile#-scope-):

The scope property can be true, an object or a falsy value:

true: A new child scope that prototypically inherits from its parent will be created for the directive's element. If multiple directives on the same element request a new scope, only one new scope is created. The new scope rule does not apply for the root of the template since the root of the template always gets a new scope.

So this is a valid bug and the fix looks pretty good to me too. I will give it a review and land this week.

[sbialy]

@caitp I believe @jtorbicki is right - it seems perfectly fine for multiple directives to share a scope using scope: true. #4402 is probably the one you're referring to - and it seems to me that this behaviour is intended and supported.
Using bindToController makes it inconsistent so this PR is valid as long as multiple scope: true directives on the same element are allowed.

[caitp]

@sbialy no, that's not the issue I'm referring to --- However, it is not supported for there to be multiple new scope directives, whether they are new isolate scopes, or new "proper" scopes, on the same element. The fact that it works sometimes, is a bug

Basically, this needs to not depend on the "scope" property of the DDO at all --- it can't use that at all

[jtorbicki]

@caitp Can you provide any reference that multiple "scope: true" are not supported? All I could find in the docs was this: https://docs.angularjs.org/error/$compile/multidir and it doesn't confirm what you'r saying.
Another thing is a comment from compile.js:

This directive is trying to add a child scope. Check that there is no isolated scope already

Again, only isolated scope is mentioned.

[sbialy]

@caitp I know what the issue is about - but the fact that it's allowed to create multiple scope: true and in the same time it's not allowed to have an isolated scope and any other scope says something. If it was not supported it would be very easy to throw an exception in that situation as well, and yet it's not the case.

[caitp]

unfortunately it's not that easy @sbialy, the compiler is a pretty complicated piece of machinery, there are a lot of factors at play, and they aren't organized in a way to make this trivial. We do attempt to throw, but it doesn't always work out

[sbialy]

I think the reason is more obvious @caitp - it's just legal: https://docs.angularjs.org/error/$compile/multidir specifies invalid configuration and scope: true is not among them.

[jwgmeligmeyling]

The docs say:

child scope + child scope => Both directives will share one single child scope

Doing so currently breaks bindToController, and I think it unexpectedly does so.
I will test this PR and see if it works for me 😄

[petebacondarwin]

Why have you changed this from checking newScopeDirective to checking controllerDirectives[i]?

[petebacondarwin]

Oh hold on, you are actually calculating the controller directive here - probably should use a different name for the var.

[petebacondarwin]

@jtorbicki, @sbialy and @jwgmeligmeyling - I agree that this is a valid bug and the fix is approximately correct. There was some churn in this part of the compiler not long ago, so I am rebasing this and merging. Thanks