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

fix(select): revert commit to fix skipping over blank disabled options #8221

Closed
wants to merge 6 commits into from
45 changes: 24 additions & 21 deletions src/ng/directive/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -405,21 +405,35 @@ 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);
render();
});
});

ctrl.$render = render;

// TODO(vojta): can't we optimize this ?
scope.$watch(render);
scope.$watch(valuesFn, render, true);
scope.$watch(getSelectedSet, render, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that you can conceivably render() multiple times per digest (not including retrying dirtychecking)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well previously render() was being called a minimum of 2 times for every digest, whether or not there was a change to any of the model values.
I initially put these into a watchGroup() call but that doesn't have a parameter to watch deeply.

Copy link
Contributor

Choose a reason for hiding this comment

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

but we shouldn't really need deep watching afaik? since we only test if values are the same with identity equality anyhow

Copy link
Contributor

Choose a reason for hiding this comment

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

or is something different now that we do need to do that

Copy link
Contributor

Choose a reason for hiding this comment

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

The deep watches are needed because we need to re-render if the list of options changes (valuesFn).
Also, the selectedSet is recreated every time and so we need to check whether its properties have changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really sold on that approach.

Previously, we were doing this work in the watch expression, and knew specifically which options changed and how.

Now, we're deep-watching with the same watch action twice, without knowing what's changed and what hasn't. For small collections of options it won' t matter, so if we're shipping this, really emphasize that it's not intended for large collections or complex collections of options


function getSelectedSet() {
var selectedSet = false;
if (multiple) {
var modelValue = ctrl.$modelValue;
if (trackFn && isArray(modelValue)) {
selectedSet = new HashMap([]);
var locals = {};
for (var trackIndex = 0; trackIndex < modelValue.length; trackIndex++) {
locals[valueName] = modelValue[trackIndex];
selectedSet.put(trackFn(scope, locals), modelValue[trackIndex]);
}
} else {
selectedSet = new HashMap(modelValue);
}
}
return selectedSet;
}


function render() {
// Temporary location for the option groups before we render them
Expand All @@ -437,22 +451,11 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
groupIndex, index,
locals = {},
selected,
selectedSet = false, // nothing is selected yet
selectedSet = getSelectedSet(),
lastElement,
element,
label;

if (multiple) {
if (trackFn && isArray(modelValue)) {
selectedSet = new HashMap([]);
for (var trackIndex = 0; trackIndex < modelValue.length; trackIndex++) {
locals[valueName] = modelValue[trackIndex];
selectedSet.put(trackFn(scope, locals), modelValue[trackIndex]);
}
} else {
selectedSet = new HashMap(modelValue);
}
}

// We now build up the list of options we need (we merge later)
for (index = 0; length = keys.length, index < length; index++) {
Expand Down Expand Up @@ -548,7 +551,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
lastElement.val(existingOption.id = option.id);
}
// lastElement.prop('selected') provided by jQuery has side-effects
if (existingOption.selected !== option.selected) {
if (lastElement[0].selected !== option.selected) {
lastElement.prop('selected', (existingOption.selected = option.selected));
if (msie) {
// See #7692
Expand Down
73 changes: 73 additions & 0 deletions test/ng/directive/selectSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,8 @@ describe('select', function() {

it('should not update selected property of an option element on digest with no change event',
function() {
// ng-options="value.name for value in values"
// ng-model="selected"
createSingleSelect();

scope.$apply(function() {
Expand All @@ -743,6 +745,11 @@ describe('select', function() {
});

var options = element.find('option');

expect(scope.selected).toEqual({ name: 'A' });
expect(options.eq(0).prop('selected')).toBe(true);
expect(options.eq(1).prop('selected')).toBe(false);

var optionToSelect = options.eq(1);

expect(optionToSelect.text()).toBe('B');
Expand Down Expand Up @@ -1148,8 +1155,73 @@ describe('select', function() {
browserTrigger(element, 'change');
expect(scope.selected).toEqual(null);
});


// Regression https://github.com/angular/angular.js/issues/7855
it('should update the model with ng-change', function() {
createSelect({
'ng-change':'change()',
'ng-model':'selected',
'ng-options':'value for value in values'
});

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

scope.change = function() {
scope.selected = 'A';
};

element.find('option')[1].selected = true;

browserTrigger(element, 'change');
expect(element.find('option')[0].selected).toBeTruthy();
expect(scope.selected).toEqual('A');
});
});

describe('disabled blank', function() {
it('should select disabled blank by default', function() {
var html = '<select ng-model="someModel" ng-options="c for c in choices">' +
'<option value="" disabled>Choose One</option>' +
'</select>';
scope.$apply(function() {
scope.choices = ['A', 'B', 'C'];
});

compile(html);

var options = element.find('option');
var optionToSelect = options.eq(0);
expect(optionToSelect.text()).toBe('Choose One');
expect(optionToSelect.prop('selected')).toBe(true);
expect(element[0].value).toBe('');

dealoc(element);
});


it('should select disabled blank by default when select is required', function() {
var html = '<select ng-model="someModel" ng-options="c for c in choices" required>' +
'<option value="" disabled>Choose One</option>' +
'</select>';
scope.$apply(function() {
scope.choices = ['A', 'B', 'C'];
});

compile(html);

var options = element.find('option');
var optionToSelect = options.eq(0);
expect(optionToSelect.text()).toBe('Choose One');
expect(optionToSelect.prop('selected')).toBe(true);
expect(element[0].value).toBe('');

dealoc(element);
});
});

describe('select-many', function() {

Expand Down Expand Up @@ -1197,6 +1269,7 @@ describe('select', function() {
expect(scope.selected).toEqual([scope.values[0]]);
});


it('should select from object', function() {
createSelect({
'ng-model':'selected',
Expand Down