ngOptions slow performance in IE due to option rerendering

[pbr1111]

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Currently, when we create a <select> with ngOptions, the updateOptions() method is called twice, causing a complete repainting of the options. The first time is called manually and the second time is called from the getWatchables watch. This is the code:

// We need to do this here to ensure that the options object is defined
// when we first hit it in writeNgOptionsValue
updateOptions();

// We will re-render the option elements if the option values or labels change
scope.$watchCollection(ngOptions.getWatchables, updateOptions);

This could be a bug, or not. In my opinion it doesn't matter if we reuse the elements, but with the current implementation it's a performance flaw.

The problem appears with the perf(ngOptions): use documentFragment to populate select commit when the behavior of updateOptions() was changed.

The main problem with this commit occurs when options are already defined (the options have changed), all current options are removed from the DOM one by one. In slow browsers (like IE), removing a child is an expensive operation. With this commit you have tried to solve a performance problem, creating a worse one.

The real performance problem is solved with this commit perf(ngOptions): avoid calls to element.value. I tried to revert the commit Revert 'perf(ngOptions): use documentFragment to populate select' commit and the performance in IE is now similar to Chrome.

I create a plunker to show the difference: http://plnkr.co/edit/T83r9GXG6NfMZqSR5jXZ (to change between Angular current implementation and the revert of the commit Revert 'perf(ngOptions): use documentFragment to populate select' commit change the commented <scripts src="">). To be able to calculate the render time of <select> I've made a wrap inside a custom directive. The time it takes to render is displayed in the browser console. The <select> have 10000 items to render (IE will not be able to load the <select> with the current implementation). Change between the angular.js scripts and see the difference.

I'll show you another bug introduced in this commit. The new updateOptions method starts with:

// We must remove all current options, but cannot simply set innerHTML = null
// since the providedEmptyOption might have an ngIf on it that inserts comments which we
// must preserve.
// Instead, iterate over the current option elements and remove them or their optgroup
// parents
if (options) {

    for (var i = options.items.length - 1; i >= 0; i--) {
	var option = options.items[i];
	if (isDefined(option.group)) {
		jqLiteRemove(option.element.parentNode);
	} else {
		jqLiteRemove(option.element);
	}
   }
}

I read the comment, then I tried to create the following <select>:

<select ng-options="getValue(item) as getDisplayValue(item) for item in itemsSource">
    <option id="nullableItem" ng-if="false" value=""></option>
</select>

The surprise was that no ngIf comment came out. The reason? All content in the <select>is deleted before it is created (selectElement.empty() called before the first updateOptions() is called in postLink). Then it would be the same to change the previous code for this one a little (we don't need to access to the element.parentNode) faster:

if (options) {
    jqLiteEmpty(selectElement[0]);
}

To show you how badly implemented it is, if we see the method updateOptionElement, there is an if to check if the label is different ... Why it needs to check? The option element is new, no label, no value. At most it could look if it is undefined, because in some browsers it is expensive to change the textContent property.

function updateOptionElement(option, element) {
  option.element = element;
  element.disabled = option.disabled;
  // NOTE: The label must be set before the value, otherwise IE10/11/EDGE create unresponsive
  // selects in certain circumstances when multiple selects are next to each other and display
  // the option list in listbox style, i.e. the select is [multiple], or specifies a [size].
  // See https://github.com/angular/angular.js/issues/11314 for more info.
  // This is unfortunately untestable with unit / e2e tests
  if (option.label !== element.label) {
    element.label = option.label;
    element.textContent = option.label;
  }
  element.value = option.selectValue;
}

What is the expected behavior?
I already commented it in the previous section.

What is the motivation / use case for changing the behavior?
Performance flaw.

Which versions of AngularJS, and which browser / OS are affected by this issue? Did this work in previous versions of AngularJS? Please also test with the latest stable and snapshot (https://code.angularjs.org/snapshot/) versions.
Angular 1.6.0 and later (all browsers, but with IE is very noticeable).

Other information (e.g. stacktraces, related issues, suggestions how to fix)
Revert the perf(ngOptions): use documentFragment to populate select commit.
If you want to use a documentFragment, you can use it when options is empty (first rendering).

[Narretz]

Thanks for the detailed report, I will investigate later. Two thoughts:

1. documentFragment improved the performance in IE when I last tested, and so far we haven't got any complaints about this. With which versions of IE did you test this? I tested in IE11 and Edge, and while the rendering is not instant, the select renders and is usable.
2. 10000 options is something I would never ever recommend to do because it's a very bad user experience and I'd argue that ngOptions is not built for that
3. the documentFragment fix solved some very difficult to fix / test IE option update bugs, so we cannot revert it
4. the ngIf bug seems to be valid. Please note that this code has been iterated many times, so there might be remnants of previous implementations that are unnessecary now

[pbr1111]

Hi,

1. I tested with IE 11.579.14393.0. I know about <select multple> previous bugs (like this Multiple select boxes freezes under asynchronous conditions in IE11 mostly of them caused because IE does something weird when it assigns the label attribute). I think the solution of implementing a documentFragment is good, because the data to be displayed are small and a repaint is a short time. It is correct as long as the updateOptions() method is not called twice when creating a <select ng-options> as it is happening now as I explained in the initial post.

   I do not know if it would work fine, but commenting the line:

   // We need to do this here to ensure that the options object is defined
   // when we first hit it in writeNgOptionsValue
   updateOptions();
   

   And then check if options are loaded in writeNgOptionsValue method, like this:

   selectCtrl.writeValue = function writeNgOptionsValue(value) {
       if (options) {
   

   If we don't have options, I think it's useless to execute the writeValue method because it manages some types of <option> elements (the selected element, unknown and empty option elements).

2. I placed 10000 items to see the performance in an extreme way. I know it's a very bad practice to use <select> with many options, there are other ways to do it, but it's a simple example. We could have 500 <select> on a same page with 20 elements each. Although it would be a bad UX practice too.

3. It's ok.

4. Ok! I have seen some parts of the code that have been simply adapted to make the new code work (most of them mentioned in the first post).

I hope I have resolved the doubts. Thank you for you answer!

[Narretz]

Would you like to open a PR that fixes the ngIf bug, removes the double initial updating, and any other possible simplifications? This would also show us which of our tests will fail with these changes