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

ngOptions with "track by" triggers ngChange when ngModel did not change #11936

Closed
ryanhart2 opened this issue May 24, 2015 · 6 comments
Closed

Comments

@ryanhart2
Copy link
Contributor

Overview of the Issue

<select
  ng-options="option.id for option in vm.options track by option.id"
  ng-model="vm.model"
  ng-change="vm.update()">
</select>

vm.options is an array of objects with a unique id property. When the <select> is first created, ngChange is triggered if ngModel is an object that does not have the same reference as any of the objects in vm.options even though it may be identical in value to one of them. As there has not been a change to the ngModel value, it is not expected that ngChange is triggered.

Motivation for or Use Case

There are two <select> elements for the user to select the make and model of a car. The options available for <select>model are dependent on the selected value in <select>make. Therefore, when the selected value in <select>make changes, the selected value in <select>model should be cleared. This works with no issues. However, when navigating away from and back to the <select> elements, the <select>model is cleared even though the value of <select>make did not change. This is not expected.

Angular Version(s)
This issue does not occur with version 1.3.15 through to 1.4.0-beta.6
This issue does occur with versions 1.4.0-rc.0 through to at least 1.4.0-rc.2

Browsers and Operating System
Chrome 43.0.2357.65 m, Firefox 38.0.1, IE 11.0.9600.17801
Windows 8.1 Pro

Reproduce the Error
http://plnkr.co/edit/xe4HPLZVEHqO7XYW1kGX?p=preview

Related Issues
#11448
#11447

Suggest a Fix
The issue seems to result from this commit: 171b9f7

In the code below, when previousValue !== nextValue is true, ngModelCtrl.$setViewValue(nextValue) is invoked, even though ngOptions.trackBy is true and !equals(previousValue, nextValue) is false.

// Check to see if the value has changed due to the update to the options
if (!ngModelCtrl.$isEmpty(previousValue)) {
  var nextValue = selectCtrl.readValue();
  if (ngOptions.trackBy && !equals(previousValue, nextValue) ||
        previousValue !== nextValue) {
    ngModelCtrl.$setViewValue(nextValue);
    ngModelCtrl.$render();
  }
}

The code to compare previousValue and nextValue could be changed to:

if (ngOptions.trackBy ? !equals(previousValue, nextValue) : previousValue !== nextValue) {

This would result in a deep comparison when track by is used and a reference comparison when track by is not used.

@ryanhart2 ryanhart2 changed the title NgOptions with "track by" triggers ngChange when ngModel did not change ngOptions with "track by" triggers ngChange when ngModel did not change May 24, 2015
@Narretz
Copy link
Contributor

Narretz commented May 25, 2015

Hi, thanks for the detailed bug report. In the plnkr, I can't see ngChange being triggered after the plnkr is initalized. (I put a console.log inside the makeModelChanged fn) Is there something missing?

@ryanhart2
Copy link
Contributor Author

Hi, thanks for having a look. I have added a console.log to make it clear when ngChange is triggered and also to show when the controller is created and destroyed. The problem occurs when the controller is created and the ngModels have valid values, not when they are undefined. The 'Remove' checkbox is there to toggle the existence of the controller using ng-if.

These are the steps to recreate the issue, including the console logs:

  1. Run Plunk controller is created
  2. Select make = Ford make changed to Ford and model reset to null
  3. Select model = Escape
  4. Select make = Toyota make changed to Toyota and model reset to null
  5. Select model = Camry
  6. Check 'Remove' controller is destroyed
  7. Uncheck 'Remove' controller is created make changed to Toyota and model reset to null

It is the last console log make changed to Toyota and model reset to null that is not expected and does not occur with versions earlier than 1.4.0-rc.0

@Narretz Narretz modified the milestones: 1.4.x - jaracimrman-existence, Purgatory May 26, 2015
@Narretz
Copy link
Contributor

Narretz commented May 26, 2015

Thanks, that clears it up! Would you like to take a shot at a PR with your suggested fix?

@ryanhart2
Copy link
Contributor Author

I will be away for a week, but if it hasn't been resolved by someone else by then, I will give it a go.

seaster pushed a commit to seaster/angular.js that referenced this issue Jun 1, 2015
Change the check on changed value for ngOptions to ensure that a reference check is only done
when trackBy is not used.  Previously, if trackBy was being used but the values of the objects were
equal a reference check was then used which would cause the check to be true if the objects had
different memory references but the values were equal.  This can cause issues with forms being
marked dirty when the value of the model as not actually changed.

Closes angular#11936
seaster pushed a commit to seaster/angular.js that referenced this issue Jun 1, 2015
Change the check on changed value for ngOptions to ensure that a reference check is only done
when trackBy is not used.  Previously, if trackBy was being used but the values of the objects were
equal a reference check was then used which would cause the check to be true if the objects had
different memory references but the values were equal.  This can cause issues with forms being
marked dirty when the value of the model as not actually changed.

Closes angular#11936
netman92 pushed a commit to netman92/angular.js that referenced this issue Aug 8, 2015
Change the check on changed value for ngOptions to ensure that a reference check is only done
when trackBy is not used.  Previously, if trackBy was being used but the values of the objects were
equal a reference check was then used which would cause the check to be true if the objects had
different memory references but the values were equal.  This can cause issues with forms being
marked dirty when the value of the model as not actually changed.

Closes angular#11936
Closes angular#11996
@dmackerman
Copy link

Seeing this while using an Angular Material select box as well. Angular version 1.4.5. Wondering if there's a workaround in the meantime?

@Narretz
Copy link
Contributor

Narretz commented Sep 14, 2015

This should be fixed in 1.4. Can you show a demo?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants