ngSwitchWhen element eats ngRepeat's child scope

[ttencate]

Scope:

{
  foo: 'value',
  bars: ['one', 'two'],
}

HTML:

<div ng-switch="foo">
    <pre ng-switch-when="value" ng-repeat="bar in bars">
        foo = {{foo}}
        bar = {{bar}}
        $index = {{$index}}
    </pre>
</div>

Fiddle: http://jsfiddle.net/y68sy/1/ (http://jsfiddle.net/PqLmp/)

Expected output:

    foo = value
    bar = one
    $index = 0

    foo = value
    bar = two
    $index = 1

Actual output:

    foo = value
    bar = 
    $index = 

    foo = value
    bar = 
    $index = 

So the ngRepeat works in the sense that the element is repeated, but the inner scope is wrong; it does not contain the values that ngRepeat puts into it.

This happens because ngSwitch creates a new child scope that inherits from the ngSwitch's scope, not from the ngSwitchWhen's scope:

angular.js/src/ng/directive/ngSwitch.js

Line 169 in e8066c4

var selectedScope = scope.$new();

I'm not sure if this is expected behaviour, or whether it can be fixed without breaking something else.

[caitp]

well it's pretty clearly not a behaviour which makes sense to a programmer, but I think it's somewhat more complicated than described here. @petebacondarwin wdyt?

[petebacondarwin]

I think this is a bug. We had to change ngIf to deal with this sort of thing: see https://github.com/angular/angular.js/blob/master/src/ng/directive/ngIf.js#L93. Here we let the transclusion provide the scope.

[petebacondarwin]

See http://plnkr.co/edit/yxLgBVoyfCkqNaGgaN4H?p=preview

[caitp]

@petebacondarwin does the change pass tests there? if so we should probably fix this soon

[petebacondarwin]

I'll check today and put together a fix
On 17 Jul 2014 14:29, "Caitlin Potter" notifications@github.com wrote:

@petebacondarwin https://github.com/petebacondarwin does the change
pass tests there? if so we should probably fix this soon

—
Reply to this email directly or view it on GitHub
#8235 (comment).

[Narretz]

Meta: @petebacondarwin @caitp Aren't we supposed to only label with one of severity? Because it's a degree, not a type.

[caitp]

these are qualitative measures, and they're all applicable here.

[Narretz]

Really, I need no idea. https://github.com/angular/angular.js/blob/master/TRIAGING.md doesn't mention it, and that's also not how I understood it when Brian explained it to me.

[caitp]

I should add, "in my opinion" --- I don't believe "broken expected use case" is any more "severe" than "confusing" or "inconvenient". The fact is, this is a broken expected use case (we know this because it seems totally natural to want to use it this way), it's confusing because it's kind of hard to even reason about why this doesn't work as expected, and it's inconvenient because it requires you to work around the limitations of the compiler with hacks.

I believe each of these are qualities applicable to this issue, and I would not be able to say that any of them are more severe than any of the others.

But really, label it however you like --- these seem reasonable to me, I don't care a whole lot if we take some out

[petebacondarwin]

@caitp - I believe that @btford's whizzy spreadsheet makes use of these labels and so I expect it will only pick one - hopefully the most severe!

[lgalfaso]

@petebacondarwin I think that the version from #8235 (comment) is problematic as making it work just for this case gives some false hope that this will work for all cases and that it works like what people expect from ng-repeat
E.g.
http://plnkr.co/edit/VuaLYHTbX2qV2qyfU4ow?p=preview

[lgalfaso]

To make this case work as expected, then we need to make the priority of ng-switch-when higher than ng-repeat. E.g.
http://plnkr.co/edit/YOYPd9hvhq9OUDC42UjQ?p=preview

[btford]

meta: I label with just the "highest severity." The prioritization dashboard assumes you do this. If something is a "broken expected use," then it's almost assuredly going to be "confusing" to developers and also "inconvenient." Of course these qualitative measures are not mutually exclusive, but the idea is that cases where something doesn't work when it reasonably seems like it should are worse than cases where it's somewhat unclear why something works a certain way. Put another way, I consider flagrant violations of a developers expectations to be more severe than edge cases with unexpected behavior. I amended the triaging doc with these notes. If anyone wants to continue the discussion of our triaging process, let's open a new issue for that.

[petebacondarwin]

Thanks @lgalfaso - I will add a test and fix up. I believe that both fixes are needed, no?

[lgalfaso]

@petebacondarwin yes, both fixes are needed

[petebacondarwin]

@lgalfaso - I can't see where you changed that to make it work in that plunk??

[petebacondarwin]

Oh, OK, so you changed ngRepeat priorty from 1000 to 700?

[lgalfaso]

@petebacondarwin change the priority of ng-repeat to 700 (anyhow, now I think it would be better to change the priority of ng-switch-when to 1200)

[petebacondarwin]

And ngSwitchDefault too.

[petebacondarwin]

The question is will that break some other use case? I can't think of one since we don't allow ngSwitchWhen to take dynamic values, so people wouldn't be able to generate a bunch of different ngSwitchWhen elements using ngRepeat...

[lgalfaso]

I can't think of anything that will break (and as you mention, the fact that ng-switch-when does not allow dynamic values is key)

[petebacondarwin]

Actually this whole thing is broken if you have ngSwitchWhen and ngSwitchDefault with ngRepeats mixed in: http://plnkr.co/edit/lcJH1E3H6r3hynoO5iT0?p=preview

:-(

[lgalfaso]

@petebacondarwin it looks like ng-switch needs the same care as ng-if. Look at
http://plnkr.co/edit/hefdNeuoEEifEvgUCE7y?p=preview

[petebacondarwin]

Yes I agree I was just doing that refactoring :-)

[petebacondarwin]

Closing in favour of #8244