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

select multi: $render is called twice when model reference changes #12731

Open
tomyam1 opened this issue Sep 2, 2015 · 3 comments
Open

select multi: $render is called twice when model reference changes #12731

tomyam1 opened this issue Sep 2, 2015 · 3 comments

Comments

@tomyam1
Copy link
Contributor

tomyam1 commented Sep 2, 2015

This is a followup on a previous issue. I've just found this issue on my own directives and I think it'd be better fixed in angular as well.

The $render function of a multi-select element is called twice whenever the model value changes by reference.
See plnker: http://plnkr.co/edit/1npwJi5tCS7gGLafYfRp?p=preview

After a model changes, $render is called first from ngModel.
It will be be called again from the viewValue watch of the select directive.

This can be fixed by keeping the last rendered value instead of the last view value in select:

// https://github.com/angular/angular.js/blob/master/src/ng/directive/select.js#L368

var lastRendered;

ngModelCtrl.$render = function() {
  selectCtrl.writeValue(ngModelCtrl.$viewValue);
  if (attr.multiple) {
    lastRendered = shallowCopy(ngModelCtrl.$viewValue);
  }
};

...

if (attr.multiple) {

  ...
  // we have to do it on each watch since ngModel watches reference, but
  // we need to work of an array, so we need to see if anything was inserted/removed
  var lastViewRef = NaN;
  scope.$watch(function selectMultipleWatch() {
    if (lastViewRef === ngModelCtrl.$viewValue && !equals(lastRendered, ngModelCtrl.$viewValue)) {
      ngModelCtrl.$render();
    }
    lastViewRef = ngModelCtrl.$viewValue;
  });

}

To make the spec cover this case. Just add an additional $digest after every $apply (before checking the number of $render calls).

@petebacondarwin
Copy link
Contributor

This is a performance issue rather than a consistency issue, right?

@petebacondarwin
Copy link
Contributor

Really we should be fixing this at the ngModel level, which at the moment doesn't handle "complex" model values very well.

@petebacondarwin petebacondarwin added this to the Backlog milestone Sep 2, 2015
@tomyam1
Copy link
Contributor Author

tomyam1 commented Sep 2, 2015

Yes, it's a performance and not a consistency issue.

Narretz added a commit to Narretz/angular.js that referenced this issue Jul 11, 2016
When the select model changes, we add the "selected" attribute to the selected option, so that
screen readers know which option is selected.

Previously, we failed to remove the attribute when the model changed to match a different option, or
the unknown / empty option.

When using "track by", the behavior would also show when a user selected an option, and then the
model was changed, because track by watches the tracked expression, and calls the $render function
on change.

This fix reads the current select value, finds the matching option and removes the "selected"
attribute.

Fixes angular#14892
Fixes angular#14419
Related angular#12731
Narretz added a commit to Narretz/angular.js that referenced this issue Jul 22, 2016
When the select model changes, we add the "selected" attribute to the selected option, so that
screen readers know which option is selected.

Previously, we failed to remove the attribute when the model changed to match a different option, or
the unknown / empty option.

When using "track by", the behavior would also show when a user selected an option, and then the
model was changed, because track by watches the tracked expression, and calls the $render function
on change.

This fix reads the current select value, finds the matching option and removes the "selected"
attribute.

Fixes angular#14892
Fixes angular#14419
Related angular#12731
Narretz added a commit to Narretz/angular.js that referenced this issue Jul 23, 2016
When the select model changes, we add the "selected" attribute to the selected option, so that
screen readers know which option is selected.

Previously, we failed to remove the attribute from the selected / empty option when the model changed
to match a different option, or the unknown / empty option.

When using "track by", the behavior would also show when a user selected an option, and then the
model was changed, because track by watches the tracked expression, and calls the $render function
on change.

This fix reads the current select value, finds the matching option and removes the "selected"
attribute.

IE9 had to be special cased, as it will report option.hasAttribute('selected') === true even
if the option's property and attribute have been unset (even the dev tools show not selected attribute).

I've added a custom matcher that accounts for this behavior. In all other browsers, property and
attribute should always be in the same state. Since few people will use screen readers with IE9, I
hope this is a satisfactory solution to the problem.

Fixes angular#14892
Fixes angular#14419
Related angular#12731
Narretz added a commit to Narretz/angular.js that referenced this issue Jul 23, 2016
When the select model changes, we add the "selected" attribute to the selected option, so that
screen readers know which option is selected.

Previously, we failed to remove the attribute from the selected / empty option when the model changed
to match a different option, or the unknown / empty option.

When using "track by", the behavior would also show when a user selected an option, and then the
model was changed, because track by watches the tracked expression, and calls the $render function
on change.

This fix reads the current select value, finds the matching option and removes the "selected"
attribute.

IE9 had to be special cased, as it will report option.hasAttribute('selected') === true even
if the option's property and attribute have been unset (even the dev tools show not selected attribute).

I've added a custom matcher that accounts for this behavior. In all other browsers, property and
attribute should always be in the same state. Since few people will use screen readers with IE9, I
hope this is a satisfactory solution to the problem.

Fixes angular#14892
Fixes angular#14419
Related angular#12731
Narretz added a commit to Narretz/angular.js that referenced this issue Jul 23, 2016
When the select model changes, we add the "selected" attribute to the selected option, so that
screen readers know which option is selected.

Previously, we failed to remove the attribute from the selected / empty option when the model changed
to match a different option, or the unknown / empty option.

When using "track by", the behavior would also show when a user selected an option, and then the
model was changed, because track by watches the tracked expression, and calls the $render function
on change.

This fix reads the current select value, finds the matching option and removes the "selected"
attribute.

IE9 had to be special cased, as it will report option.hasAttribute('selected') === true even
if the option's property and attribute have been unset (even the dev tools show not selected attribute).

I've added a custom matcher that accounts for this behavior. In all other browsers, property and
attribute should always be in the same state. Since few people will use screen readers with IE9, I
hope this is a satisfactory solution to the problem.

Fixes angular#14892
Fixes angular#14419
Related angular#12731
Narretz added a commit that referenced this issue Jul 23, 2016
When the select model changes, we add the "selected" attribute to the selected option, so that
screen readers know which option is selected.

Previously, we failed to remove the attribute from the selected / empty option when the model changed
to match a different option, or the unknown / empty option.

When using "track by", the behavior would also show when a user selected an option, and then the
model was changed, because track by watches the tracked expression, and calls the $render function
on change.

This fix reads the current select value, finds the matching option and removes the "selected"
attribute.

IE9 had to be special cased, as it will report option.hasAttribute('selected') === true even
if the option's property and attribute have been unset (even the dev tools show not selected attribute).

I've added a custom matcher that accounts for this behavior. In all other browsers, property and
attribute should always be in the same state. Since few people will use screen readers with IE9, I
hope this is a satisfactory solution to the problem.

Fixes #14892
Fixes #14419
Related #12731
PR (#14894)
Narretz added a commit that referenced this issue Jul 23, 2016
When the select model changes, we add the "selected" attribute to the selected option, so that
screen readers know which option is selected.

Previously, we failed to remove the attribute from the selected / empty option when the model changed
to match a different option, or the unknown / empty option.

When using "track by", the behavior would also show when a user selected an option, and then the
model was changed, because track by watches the tracked expression, and calls the $render function
on change.

This fix reads the current select value, finds the matching option and removes the "selected"
attribute.

IE9 had to be special cased, as it will report option.hasAttribute('selected') === true even
if the option's property and attribute have been unset (even the dev tools show not selected attribute).

I've added a custom matcher that accounts for this behavior. In all other browsers, property and
attribute should always be in the same state. Since few people will use screen readers with IE9, I
hope this is a satisfactory solution to the problem.

Fixes #14892
Fixes #14419
Related #12731
PR (#14894)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants