Material: select: required unsets model value with AngularJS 1.7.8

[jazdw]

Bug, enhancement request, or proposal:

Bug

CodePen and steps to reproduce the issue:

CodePen Demo which demonstrates the issue:

https://codepen.io/jazdw/pen/VRyVYa?editors=1011

Detailed Reproduction Steps:

1. Run the codepen
2. Observe the error on the form and that $ctrl.value is now undefined
3. Also see that "Test value" remains selected in the drop down and has an asterisk next to it

What is the expected behavior?

It should not delete the value from the model, input should not have an error.

What is the current behavior?

Value is deleted from the model, input has an error.

What is the use-case or motivation for changing an existing behavior?

Which versions of AngularJS, Material, OS, and browsers are affected?

• AngularJS: 1.7.8
• AngularJS Material: 1.1.13
• OS: Windows 10
• Browsers: Chrome (latest), Firefox (latest)

Is there anything else we should know? Stack Traces, Screenshots, etc.

Seems like behavior changes over the last few released of AngularJS have exposed this problem:
https://github.com/angular/angular.js/commit/69262239632027b373258e75c670b89132ad9edb
https://github.com/angular/angular.js/commit/5ad4f5562c37b1cb575e3e5fddd96e9dd10408e2
https://github.com/angular/angular.js/commit/a4c7bdccd76c39c30e33f6215da9a00cc8acde2c

The cause of the problem seems to be the $validate() method being called by the AngularJS required directive (via attr.$observe on required attribute) before the options are added to the mdSelect controller. When $validate() is called the mdSelect $isEmpty implementation returns true as the options are not populated, this results in the ngModelController writing undefined to the scope.

[Narretz]

Is that something that can be fixed by Material? Or does it have to be fixed in Angularjs?

[gkalpak]

@jazdw, I am confused by the example. The issue seems to be caused by having ng-model (without a value!?) on <test-component>: <test-component required ng-model></test-component>

That doesn't seem valid (or intentional?).

#

Can you, please, explain what exactly you are trying to achieve or provide a valid example?

[jazdw]

Is that something that can be fixed by Material? Or does it have to be fixed in Angularjs?

Not entirely sure, it seems to be due to some strange behavior in AngularJS Material which causes issues with the latest AngularJS changes I linked to.

That doesn't seem valid (or intentional?)

Yeah wasn't intentional. I have updated the Codepen so it is clearer. Basically there is a strange timing issue where if you set required = true at just the right time it causes the model value to be set to undefined.

What happens is:

1. AngularJS required directive detects a change to the required attribute of the element via attr.$observe()
2. AngularJS required directive calls ngModelController.$validate()
3. mdSelect controller's validator runs, at this point the options are not populated
4. mdSelect controller's $isEmpty() implementation returns true
5. ngModelController writes undefined to the scope and marks the form control as having an error

I guess one thing I am not clear on is why the mdSelect controller's validator fails if the model value is not in the list of options. This doesn't seem correct to me.

[Splaktar]

Is that something that can be fixed by Material? Or does it have to be fixed in Angularjs?

@Narretz I'm certainly open to that, but I would need to know what needs to be changed and what the correct behaviors are. Also we would need to document that this issue exists when using AngularJS 1.7.8+ with prior versions of AngularJS Material.

[Splaktar]

@jazdw your CodePen has:

<span>My value is {{$ctrl.test === undefined ? 'Undefined :(' : $ctrl.test}}</span>`

Shouldn't this be?

<span>My value is {{$ctrl.value === undefined ? 'Undefined :(' : $ctrl.value}}</span>`

[jazdw]

Shouldn't this be?

Yes, sorry. Fixed. Same result.

[Splaktar]

The value doesn't change, but the component's initial state does change if you use ng-value as recommended in the AngularJS Material md-select Docs.

ng-value="test"

[gkalpak]

Here are some preliminary results from my investigation:

The issue seems to be (somehow) related to the fact that options can be updated at arbitrary times, which in turn can change the select's validation state. (For example, adding a new option could make the current $viewValue map to an actual option, which would allow setting a defined $modelValue, which would make the required validator pass, which could make the control valid.) However, nothing triggers the validation logic in this case, leaving the previous state (invalid).

For example, the problem would be solved by adding a call to ngModel.$validate() here:

 $scope.$watchCollection(function() { 
   return self.options; 
 }, function() { 
+  self.ngModel.$validate(); 
   self.ngModel.$render(); 
 });

As a temporary work-around, it is possible to solve it by adding the following (hacky) directive to one's app:

.directive('mdSelectMenu', () => ({
  restrict: 'E',
  require: 'mdSelectMenu',
  link: (scope, elem, attrs, selectMenuCtrl) => {
    scope.$watchCollection(
        () => selectMenuCtrl.options,
        () => selectMenuCtrl.ngModel.$validate());
  },
}))

#

I am still investigating what the root cause is and whether this should be fixed in AngularJS or ngMaterial (but wanted to get my initial findings out there).

[gkalpak]

So, afaict, the problem stems from the following facts:

1. With angular/angular.js@0637a2124 the timing of certain ngModel related operations (such as validation and updating $modelValue) has changed.
2. SelectMenuController#addOption() may change the $modelValue and thus needs to manually run $validate() in some cases.
   _{(BTW, I wonder whether removeOption() needs to do the same 🤔)}
3. SelectMenuController#addOption() relied on some pre-angular/angular.js@0637a2124 behavior/timing.
   _{(BTW, it seems to only work on 1.7.7 "by accident".)}

#

Fixing it on the ngMaterial side:

The main issue is that addOption() relies on ngModel.$modelValue to compare against the new selected value, but in case the control has been found to be invalid before (e.g. because the select options have not been populated yet), $modelValue would be set to undefined (and it would not match the selected value).

What could be done instead is using ngModel.$$rawModelValue, which will retain the actual model value even if the control is invalid:

https://github.com/angular/material/blob/bcc5ecadc62bf3cb1f9f0a5dab034e07e5b8f8e5/src/components/select/select.js#L842

#

Fixing it on the AngularJS side:

As mentioned above, angular/angular.js@0637a2124 has introduced some timing changes, which can (in case of complex custom components) be considered breaking. If we wanted to "fix it", we could revert the commit (and follow-up fixes).

I am torn at whether it is a good idea at this point, given that it has been in for a couple of releases and nothing else was reported broken 🙈

#

@Narretz, @Splaktar, wdyt (wrt where it makes more sense to fix it)?

[Narretz]

I'm biased (since I wrote the change in AngularJS), but I vote not to revert the change in AngularJS. The setup to trigger the bug in material seems complex enough to say that this won't affect many users: you need value instead of ng-value and also set the required state with applyAsync. You can even work around the latter by using setTimeout instead of applyAsync (although this might not work in the real app).
A fix for ngMaterial is also a possibility, even if it means using the private $$rawModelValue - we are definitely not going to remove it from AngularJS. ;) @gkalpak did you test the fix with the ngMaterial unit test suite?

[gkalpak]

No, I didn't try running the tests. I just tried in the codepen. But I would be very surprised if switching from $modelValue to $$rawModelValue would break anything (given the intention of the that check).

@Splaktar, wdyt about fixing it in ngMaterial?

[Splaktar]

OK, I'm fine with fixing this in AngularJS Material, but I've been having some problems reproducing this in a unit test w/o a custom component. I guess that I will need to make the test more complex in order to reproduce the issue. Unfortunately, we don't have a single test in AngularJS Material that uses AngularJS .component().

[Splaktar]

Changing to

        if (angular.isDefined(self.ngModel.$$rawModelValue) &&
            self.hashGetter(self.ngModel.$$rawModelValue) === hashKey) {
          self.ngModel.$validate();
        }

Does not cause any failures with the AngularJS Material test suite.

[Splaktar]

With the following test, I can get $error.required to be set, even though the model has a valid value:

      it('should re-validate when the options and required value change', function() {
        $rootScope.model = 2;
        var select = $compile('<form name="testForm"><md-input-container><label>Test select</label>' +
          '<md-select ng-model="model" name="multiSelect" required="{{required}}">' +
          '<md-option ng-repeat="opt in opts" value="{{opt}}">{{opt}}</md-option>' +
          '</md-select></md-input-container></form>')($rootScope);
        $rootScope.$applyAsync(function() {
          $rootScope.opts = [1, 2, 3, 4];
          $rootScope.required = true;
        });
        $rootScope.$digest();
        $timeout.flush();

        expect($rootScope.model).toBe(2); // <-- passes
        expect($rootScope.testForm.$error.required).not.toExist(); // <-- fails
      });

But I can't get it to unset the model value as demonstrated in the CodePen for this issue.

Additionally, the behavior does not change when applying the $$rawModelValue change above.

[Splaktar]

OK, I've got a custom component in my test now

  beforeEach(module(function ($compileProvider) {
    $compileProvider.component('testComponent', {
      controller: ['$scope', function($scope) {
        this.value = 'test';

        $scope.$applyAsync(function () {
          this.required = true;
        });
      }],
      template: '<md-input-container><label>Test select</label>' +
        '<md-select name="value" ng-model="$ctrl.value" ng-required="$ctrl.required">' +
        '<md-option value="test">Test value</md-option>' +
        '</md-select></md-input-container>'
    });
  }));

But I can't seem to reproduce this there either:

  describe('with components', function() {
    it('should re-validate when the options and required value change', function() {
      var nodes = $compile('<form name="testForm">' +
        ' <test-component></test-component>' +
        '</form>')($rootScope);
      $rootScope.$digest();
      $timeout.flush();

      expect($componentController('testComponent').value).toBe('test');
      expect($rootScope.testForm.$error.required).not.toExist();
    });
  });

What am I missing?

[Narretz]

It's possible that ngMock subtly changes the timing of async tasks. I assume the example works as expected if you run it without ngMock?

[Splaktar]

You mean run it outside of the unit testing environment? or is there some way to run it as a unit test but not us ngMock?

[Narretz]

I have created the test in a plunker (re-used a setup) and it fails as expected: http://next.plnkr.co/edit/c2BXhoPHoHhyqGGQ?preview (see comment below)

So it doesn't look like it is related to ngMock.

[Narretz]

Oh okay, it's about the model value being set to undefined. That doesn't work, weird.

[Narretz]

Updated the plunker again. The problem was the $componentController use. This created a new instance of the custom component controller, but without actually compiling its template.

[Splaktar]

In my local dev, I can only make the Plunker fail if I use

        $scope.$applyAsync(() => {
          this.required = true;
        });

Using ES5, everything works as expected

        $scope.$applyAsync(function () {
          this.required = true;
        });

However, I can get it to fail with ES5 via the following

        var $ctrl = this;
        $ctrl.value = 'test';

        $scope.$applyAsync(function () {
          $ctrl.required = true;
        });

In that case, using $$rawModelValue does indeed fix the issue. Is this expected?

[Narretz]

With

function () {
          this.required = true;
        }

this is scoped to the function, so required is set as a property on the function, not on the component. This is expected.

this is not scoped to the controller function, so required is not set as a property on the controller.

[mgol]

@Narretz Does AngularJS does anything with the context of a function passed to $scope.$applyAsync? Otherwise, this in unbound functions not called as methods is not that function but window in sloppy mode & this in strict mode.

[gkalpak]

No, AngularJS does not do anything special there. What @Narretz meant in https://github.com/angular/material/issues/11679#issuecomment-493596965 (I assume) is that this does not refer to the controller instance when using a regular function (as happens when using an arrow function).

To clarify: The $applyAsync(...) call was originally taken from OP's example, which is using an arrow function, so this refers to the controller instance. It was then changed to a regular function (see https://github.com/angular/material/issues/11679#issuecomment-492372704), while trying to incorporate it in the ngMaterial test suite (which uses ES5).

[mgol]

@gkalpak

No, AngularJS does not do anything special there. What @Narretz meant in #11679 (comment) (I assume) is that this does not refer to the controller instance when using a regular function (as happens when using an arrow function).

Yes, I understood that, I've just got confused by what happens for regular non-arrow functions from @Narretz's comment. I assume it was just a mistake and the property is not actually set on a function.

Anyway, I guess this is a digression to the main topic.