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
  • Loading branch information
Jeff Balboni authored and caitp committed Mar 19, 2014
1 parent 37bc5ef commit f40f54c
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));

This comment has been minimized.

Copy link
@revolunet

revolunet Jun 16, 2014

Contributor

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

}
} 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

3 comments on commit f40f54c

@revolunet
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@caitp caitp commented on f40f54c Jun 16, 2014

Choose a reason for hiding this comment

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

@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
Copy link
Contributor

Choose a reason for hiding this comment

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

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 :/

Please sign in to comment.