fix(select): prevent unknown option being added to select when bound …

[divercraig]

…to null property

If a select directive was bound, using ng-model, to a property with a value of null this would
result in an unknown option being added to the select element with the value "? object:null ?".
This change prevents a null value from adding an unknown option meaning that the extra option is
not added as a child of the select element

Closes #11872

[caitp]

I'm not sure this is right. On the surface, it looks okay, but the thing is, the unknown options are there for a reason (which is, when the model value is defined but doesn't match one of the specified options). I think this probably behaves as expected if the value is non-null, but it's inconsistent with the way ngModel normally works.

[divercraig]

I don't really think unknown options should be added for null model values. The value hasn't been defined if it's null.
I haven't changed the implementation of unknown options which are non-null, only null options.

[caitp]

Yes, but this still violates the norm of ngModel, I'm not positive we want to do that. "null" does not mean "undefined", it means "null"

[divercraig]

urgh, this has failed a test due to an (i believe) unrelated timeout.
How can I resubmit for travis testing without closing the pull request and creating a new one?

[caitp]

@petebacondarwin what do you think, break the ngModel norm here or no?

[divercraig]

This change is trying to fix a problem that was introduced between 1.2.23 and now. I guess it's a regression.

[petebacondarwin]

I was also worried about this change; The question is whether null can be a valid value or not. I think I was even the one who wrote the test that explicitly checks that null triggers the unknown option. At the time I assumed that we should consider null a valid value, as @caitp points out.

Looking at the code further I realise now that select actually sets the model to empty string if the user actually selects the "empty" option. Moreover, when we are talking about non-ngOptions based select directives then the values for the options can only be strings, so we can never match null against any of the options.

Here is a bit of a clearer plunker with regards the types of the models: http://plnkr.co/edit/Vc5ngCtdhzWwsQXcscKC?p=preview

So I think we should merge this PR as in the case of a select rather than an ngOptions directive, null is indistinguishable from undefined (or empty string) for that matter.

Can anyone spot a pain point in current applications from merging this? @caitp, @Narretz, @divercraig

[petebacondarwin]

@divercraig - I just noticed that this behaviour has been the case even before 1.2.23. See http://plnkr.co/edit/Vc5ngCtdhzWwsQXcscKC?p=preview
So I am not sure how it is a regression.

[caitp]

personally I think it's working as expected, but I don't care a whole lot if it is merged

[petebacondarwin]

@divercraig - Here is a simple workaround that doesn't require us to make a potentially breaking change to the core, and which works for all recent versions of Angular 1.2, 1.3 and 1.4.

http://plnkr.co/edit/6y0eMiLq6VFVqZLPnVui?p=preview

[divercraig]

@petebacondarwin I can see your "issue" in 1.2.23 which is weird because I only started noticing this problem in my project when I upgraded to 1.3.15 from 1.2.23.

I acknowledge that there is a workaround, but I think if we think about it this change makes sense and is safe:

• Why would anyone expect the current behaviour (the addition of a new empty option with a value like ? object:null ?)
• null and undefined and empty string are all different things it's true... but in the context of an option element we cannot represent null or undefined and as such use empty string. So in the context of an option in html null, undefined and empty string are treated as the same thing.

[petebacondarwin]

@divercraig - OK. I agree with you. The current behaviour, whether it is "correct" or not, is confusing and surprising. I'll land this PR this week.