Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit f40f54c

Browse files
Jeff Balbonicaitp
Jeff Balboni
authored andcommittedMar 19, 2014
fix(select): avoid checking option element selected properties in render
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
1 parent 37bc5ef commit f40f54c

File tree

2 files changed

+28
-1
lines changed

2 files changed

+28
-1
lines changed
 

‎src/ng/directive/select.js

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,12 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
394394
value = valueFn(scope, locals);
395395
}
396396
}
397+
// Update the null option's selected property here so $render cleans it up correctly
398+
if (optionGroupsCache[0].length > 1) {
399+
if (optionGroupsCache[0][1].id !== key) {
400+
optionGroupsCache[0][1].selected = false;
401+
}
402+
}
397403
}
398404
ctrl.$setViewValue(value);
399405
});
@@ -531,7 +537,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
531537
lastElement.val(existingOption.id = option.id);
532538
}
533539
// lastElement.prop('selected') provided by jQuery has side-effects
534-
if (lastElement[0].selected !== option.selected) {
540+
if (existingOption.selected !== option.selected) {
535541
lastElement.prop('selected', (existingOption.selected = option.selected));

Comment on line R541

revolunet commented on Jun 16, 2014

@revolunet
Contributor

this reverts what was done here : 4622af3
and cause a bug in iOS

Code has comments. Press enter to view.
536542
}
537543
} else {

‎test/ng/directive/selectSpec.js

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -733,6 +733,27 @@ describe('select', function() {
733733
expect(sortedHtml(options[2])).toEqual('<option value="1">3</option>');
734734
});
735735

736+
it('should not update selected property of an option element on digest with no change event',
737+
function() {
738+
createSingleSelect();
739+
740+
scope.$apply(function() {
741+
scope.values = [{name: 'A'}, {name: 'B'}, {name: 'C'}];
742+
scope.selected = scope.values[0];
743+
});
744+
745+
var options = element.find('option');
746+
var optionToSelect = options.eq(1);
747+
748+
expect(optionToSelect.text()).toBe('B');
749+
750+
optionToSelect.prop('selected', true);
751+
scope.$digest();
752+
753+
expect(optionToSelect.prop('selected')).toBe(true);
754+
expect(scope.selected).toBe(scope.values[0]);
755+
});
756+
736757
describe('binding', function() {
737758

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

3 commit comments

Comments
 (3)

revolunet commented on Jun 16, 2014

@revolunet
Contributor

Sorry but i don't really understand that test, maybe you can help me understand better

  • you first manually change option.property of a non-selected option
  • then ensure the property is kept in the DOM, OK
  • but then we make sure it's not really selected in the model ?

as angular rewrites all the properties based on the model value during digest, for me its legit that a non-selected option (B) (based on the model value) doesn't keep the selected property, nope ?

looks like this change introduces a bug in a special case on some browsers (iOS, IE) : when we have an ngModel that doesnt belong to ngOptions, angular adds an empty option, that is removed on selection. that changes introduces a shift in the index and thus the displayed value is not the selected one. the model value, though is OK

Here's an example you can try on iOS/IE : http://run.plnkr.co/plunks/xS3sDPIv7DnuxqA8A5bI/

Any idea ?

caitp commented on Jun 16, 2014

@caitp
Contributor

@revolunet your issue that you bring up is a known issue, someone has been working on a fix for it but it's not clear that they've ever submitted it yet, if you want to try and work on that. It isn't super high priority at the moment, but we'd like to get that fixed.

However, and this is important, ng-model throws a wrench into the whole "model -> view" thing. With ng-model, we're basically saying that the view / user interactions have the ability to change the model, and different browsers do this sort of differently for select controls. IE, before this patch was checked in, we had some issues on firefox, now after this patch is checked in, we have some issues on IE/iOS. Ideally, we don't have any of those issues. The web makes this hard, but that's what we're going for here. But yeah, the model does need to update to reflect changes to the selected state of options.

Anyways, if you are interested in working on a fix for the IE/iOS issue, that would be awesome

revolunet commented on Jun 16, 2014

@revolunet
Contributor

thanks fo the feedback, i'm working on it, trying to sort out the select directive code which is not very easily readable :)

IMHO this issue should be a priority as a simple combination of an invalid ngModel (eg: empty value) with ngOptions is buggy on IE and all iOS which is quite a lot of browsers :/

This repository has been archived.