Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(select): avoid checking option element selected properties in render #5994

Closed
wants to merge 3 commits into from
Closed

fix(select): avoid checking option element selected properties in render #5994

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a284b1
fix(select): avoid checking option element selected properties in render
Jan 26, 2014
956f65d
Clean up null option explicitly, instead of tracking change event
Jan 30, 2014
ef9b16d
Updated test description and added expect statements
Jan 31, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/ng/directive/select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,12 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
value = valueFn(scope, locals);
}
}
// Update the null option's selected property here so $render cleans it up correctly
if (optionGroupsCache[0].length > 1) {
if (optionGroupsCache[0][1].id !== key) {
optionGroupsCache[0][1].selected = false;
}
}
}
ctrl.$setViewValue(value);
});
Expand Down Expand Up @@ -529,7 +535,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
lastElement.val(existingOption.id = option.id);
}
// lastElement.prop('selected') provided by jQuery has side-effects
if (lastElement[0].selected !== option.selected) {
if (existingOption.selected !== option.selected) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that this broke a test on my angular application related to selected option validation. Here's an example of what's broken: http://plnkr.co/edit/hPjcZP?p=preview

If you change the angular version to 1.2.14 or earlier, the behavior I expected works fine. The problem with this fix introduced in 1.2.15, when I revert the value back to the previous selected option each 'existingOption' looks the same as 'option' and so the 'selected' attribute on the select isn't updated. This causes the select element to become out of sync with the model.

Is there a better way to fix this issue? Is there a better way for me to implement my behavior?

I understand that this isn't how validation is typically done, but in my case I would rather revert the model value rather than setting it to undefined, if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@butchpeters I could be totally wrong, but I would say that what you're doing is not the best use of parsers, since they're meant to transform or inspect values for the model and not to set the values in your view. It looks like 1.3 has some commit and rollback functions which would help here, though.

Is there a reason you can't set the select to invalid ($setValidity)? It's a little weird for me as a user to choose an option and then have it instantly change back (but I don't know what this actually looks like in your app).

The 'correct' behavior here to me would be to set the disabled attribute on the option you don't want chosen. That's hard with ng-options, but there are some solutions here: http://stackoverflow.com/questions/16202254/ng-options-with-disabled-rows. You could also remove the unselectable option with a filter on the ng-options expression.

lastElement.prop('selected', (existingOption.selected = option.selected));
}
} else {
Expand Down
15 changes: 15 additions & 0 deletions test/ng/directive/selectSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,21 @@ describe('select', function() {
expect(sortedHtml(options[2])).toEqual('<option value="1">3</option>');
});

it('should ignore option object selected changes', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this description should be improved. without reading the test code, it is not clear to me what we are testing here.

createSingleSelect();

scope.$apply(function() {
scope.values = [{name: 'A'}, {name: 'B'}, {name: 'C'}];
scope.selected = scope.values[0];
});

var options = element.find('option');
var optionToSelect = options.eq(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to make the test more readable we should also add expect(optionToSelect.text()).toBe('B');

optionToSelect.prop('selected', true);
scope.$digest();
expect(optionToSelect.prop('selected')).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and also:

expect(scope.selected).toBe(scope.values[1]);

however your change fails this test. can you please look into why and fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that's the correct behavior. The selected property on the option element doesn't always reflect the actual selected value: hovering over an option in FF will set that property, even though the user has not made a selection yet. And since a change event isn't fired when you're just hovering over an option, the model value shouldn't change.

});

describe('binding', function() {

it('should bind to scope value', function() {
Expand Down