fix(select): manage select controller options correctly #9421
Conversation
shahata
changed the title
|
This is still work in progress and requires some refactoring, but I'm putting it out just to see if ppl think that this is the right way to handle this... /cc @btford |
shahata
changed the title
shahata
added
component: forms
frequency: moderate
severity: regression
and removed
In Progress
labels
| @@ -179,7 +179,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { | |||
| // Workaround for https://code.google.com/p/chromium/issues/detail?id=381459 | |||
| // Adding an <option selected="selected"> element to a <select required="required"> should | |||
| // automatically select the new element | |||
| if (element[0].hasAttribute('selected')) { | |||
| if (element && element[0].hasAttribute('selected')) { | |||
|
This seems like the right approach. |
|
@btford should I merge this then? or do you think it needs more work? |
jeffbcross
force-pushed
the
master
branch
2 times, most recently
from
abdaab7 to
30996f8
Compare
|
Could you add to the commit message to explain what was broken and how this fixes it? This will need a rebase since @tbosch and I have refactored select. I rebased pretty easily and pushed to my fork: https://github.com/jeffbcross/angular.js/tree/pr-9421 @btford I will assign this and take a deeper look since I've been working in select this week. |
|
This wasn't introduced by this commit, but the property names of objects in the test don't match their position in the comprehension expression. e.g.: scope.robots = [
{key: 1, value: 'c3p0'},
{key: 2, value: 'r2d2'}
];
compile('<select ng-model="robot" ' +
'ng-options="item.key as item.value for item in robots">' +
'</select>');should be scope.robots = [
{value: 1, label: 'c3p0'},
{value: 2, label: 'r2d2'}
];
compile('<select ng-model="robot" ' +
'ng-options="item.value as item.label for item in robots">' +
'</select>'); |
This fixes a regression that was introduced in 2bcd02d. Basically, the problem was that render() removed the wrong option from the select controller since it assumed that the option that was removed has the same label as the excessive option in existingOptions, but this is only correct if the option was popped from the end of the array. We now remember for each label whether it was added or removed (or removed at some point and then added at a different point) and report to the select controller only about options that were actually removed or added, ignoring any options that just moved. Closes #9418
|
@jeffbcross I've rebased this and fixed the commit msg and property names in tests |
|
@shahata I think this is good. It took me some looking around to understand how it was working. What do you think about adding this comment above the function? /** I've already got the change locally, so you can just let me know if it's good to go. |
|
Sure, comment is fine |
|
Landed as 2435e2b Thanks! |
Closes #9418