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

Commit

Permalink
fix(select): avoid checking option element selected properties in render
Browse files Browse the repository at this point in the history
In Firefox, hovering over an option in an open select menu updates the selected property of option
elements. This means that when a render is triggered by the digest cycle, and the list of options
is being rendered, the selected properties are reset to the values from the model and the option
hovered over changes. This fix changes the code to only use DOM elements' selected properties in a
comparison when a change event has been fired. Otherwise, the internal new and existing option
arrays are used.

Closes #2448
Closes #5994
Closes #6769
  • Loading branch information
Jeff Balboni authored and caitp committed Mar 20, 2014
1 parent 320f6d1 commit dc149de
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 1 deletion.
8 changes: 7 additions & 1 deletion src/ng/directive/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,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 @@ -531,7 +537,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) {
lastElement.prop('selected', (existingOption.selected = option.selected));
}
} else {
Expand Down
21 changes: 21 additions & 0 deletions test/ng/directive/selectSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,27 @@ describe('select', function() {
expect(sortedHtml(options[2])).toEqual('<option value="1">3</option>');
});

it('should not update selected property of an option element on digest with no change event',
function() {
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);

expect(optionToSelect.text()).toBe('B');

optionToSelect.prop('selected', true);
scope.$digest();

expect(optionToSelect.prop('selected')).toBe(true);
expect(scope.selected).toBe(scope.values[0]);
});

describe('binding', function() {

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

4 comments on commit dc149de

@eendeego
Copy link

Choose a reason for hiding this comment

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

This patch breaks selects with a required attribute and a blank disabled option.

Please check this jsbin on both chrome and firefox.

@caitp
Copy link
Contributor

@caitp caitp commented on dc149de Jun 5, 2014

Choose a reason for hiding this comment

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

@luismreis please open an issue regarding this, and link to the jsbin from there.

@eendeego
Copy link

Choose a reason for hiding this comment

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

Ok, sorry for the disruption. Issue #7715 filed.

@caitp
Copy link
Contributor

@caitp caitp commented on dc149de Jun 5, 2014

Choose a reason for hiding this comment

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

No problem, thanks =)

Please sign in to comment.