perf(ngOptions): prevent initial options repaiting #15812

pbr1111 · 2017-03-14T17:22:03Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.

What is the current behavior? (You can also link to an open issue here)
Refers to this issue: ngOptions slow performance in IE due to option rerendering

What is the new behavior (if this is a feature change)?
Now the options are loaded once when we create the <select ng-options> directive. Also, if we add an <option> element with an ng-if directive, it is not deleted now.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:
None.

Narretz · 2017-03-15T10:47:24Z

Hi,
thanks for the Pr. As you can see in the Travis logs, there are a lot of test failures. Most of these come from the fact that there's a different writeSelectValue for select multiple, so you'll to make the change to this fn too.

Please also note that we definitely need to support an empty option with ngIf - there are some tests for this, and I assume some of them will fail in the current state.

You can run the unit tests locally with grunt autotest. This runs the test every time you change a file.

pbr1111 · 2017-03-15T19:37:12Z

Hi, let's see if I can fix it. Thanks for the info!

pbr1111 · 2017-03-17T18:24:32Z

The code just passed all the tests. This solution avoids double repainting of <options> and also makes ng-if work correctly in the empty option.

Narretz · 2017-03-17T19:37:16Z

Thx @pbr1111, I will review this next week. Was there any reason for closing / reopening the issue so often?

pbr1111 · 2017-03-17T20:15:16Z

I close and reopen because eslint tests fails several times and I've made a rebase of my branch each time it fails to upload the new commit with the errors solved.

Narretz · 2017-03-17T20:37:41Z

If you push new / changed code, it'll run Travis again, no need to close / reopen the issue.

gkalpak · 2017-03-20T12:17:26Z

src/ng/directive/ngOptions.js

@@ -624,7 +629,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
        // 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) {
+        if (option.label !== undefined) {


Why this change?

element.label always will be undefined (because we create a new empty option element each time), then it's useless to read element.label value if we know that its value is undefined.

You should use isDefinedhere

gkalpak · 2017-03-20T12:20:20Z

src/ng/directive/ngOptions.js

-          break;
-        }
+      var optionsToRemove = [];
+      for (var i = 0, children = selectElement[0].childNodes, ii = children.length; i < ii; i++) {


Is the move from jqLite/jQuery APIs to native APIs part of the perf fix or is it just a "generally good idea" type of change?

jqLite/jQuery children() method doesn't return the nodes of nodeType equal to NODE_TYPE_COMMENT. We can use jqLite/jQuery contents() method instead, but I prefer to use native API.

gkalpak · 2017-03-20T14:11:32Z

src/ng/directive/ngOptions.js

+      var optionsToRemove = [];
+      for (var i = 0, children = selectElement[0].childNodes, ii = children.length; i < ii; i++) {
+          if (!selectCtrl.hasEmptyOption
+              && (children[i].value === '' || children[i].nodeType === NODE_TYPE_COMMENT)) {


So, any comment will be interpreted as empty option? This doesn't seem right.

True. I'm doing changes to my commit. I will simplify this whole part and leave it as close to the previous code.

pbr1111 · 2017-03-20T18:32:37Z

I just modified the previous commit. Now the select element is emptied after it founds (or not) an empty option (the code is the same as before, it works because the options haven't yet been compiled in postLink, my previous solution was totally incorrect). The empty option element is readded if it's provided.

Narretz · 2017-03-20T18:38:27Z

@pbr1111 You can just push a new commit and you / we will squash before merging.

it works because the options haven't yet been compiled in postLink, my previous solution was totally incorrect

You should include a test that works with the correct version but fails with the incorrect one - it sounds like we are missing a test there

We should also have a test that ensures we don't paint the option twice.

pbr1111 · 2017-03-24T20:32:07Z

I added some tests to check both cases. The second test 2ec5c07 should fail with the current implementation, but it doesn't fail with eb3661c commit.

Narretz

Looking good so far! Some minor clarifications and changes nedded. I would like for @petebacondarwin too also take a look because I remember he once wasn't a fan of doing if (!options) return

Narretz · 2017-03-27T10:00:36Z

src/ng/directive/ngOptions.js

@@ -449,36 +452,38 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
      if (!multiple) {

        selectCtrl.writeValue = function writeNgOptionsValue(value) {
-          var selectedOption = options.selectValueMap[selectElement.val()];
-          var option = options.getOptionFromViewValue(value);
+          if (options) {


Can you change to if (!options) return? This will reduce the whitespace changes.

Narretz · 2017-03-27T10:12:17Z

test/ng/directive/ngOptionsSpec.js

+      ];
+      scope.isBlank = true;
+
+      createSingleSelect('<!-- test comment --><option ng-if="isBlank" value="">blank</option><!-- test comment 2 -->');


Line is too long, please split at around 100 chars

Narretz · 2017-03-27T10:13:35Z

test/ng/directive/ngOptionsSpec.js

@@ -2641,6 +2658,23 @@ describe('ngOptions', function() {
      dealoc(element);
    }));

+
+    it('should ignore the comments within the select', function() {


There are no expectations regarding the comments you added. Should this test that all comments that are not part of the empty option are removed?

Narretz · 2017-03-27T10:14:33Z

test/ng/directive/ngOptionsSpec.js

@@ -779,6 +779,23 @@ describe('ngOptions', function() {
    expect(options[2]).not.toBeMarkedAsSelected();
  });

+
+  it('should render the options only one time', function() {


the initial options

Narretz · 2017-03-27T10:19:23Z

test/ng/directive/ngOptionsSpec.js

+      'ng-model': 'value'
+    });
+
+    element.attr('ng-options', 'value for value in values');


you should pass the ngoptions expression like you did with ng-model. Or use createSingleSelect which has useful defaults.

Narretz · 2017-03-27T10:23:19Z

src/ng/directive/ngOptions.js

@@ -624,7 +629,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
        // 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) {
+        if (option.label !== undefined) {


You should use isDefinedhere

Narretz · 2017-03-27T10:42:20Z

src/ng/directive/ngOptions.js

@@ -428,6 +428,9 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
        }
      }

+      // Remove all the elements inside the select.
+      selectElement.empty();


Please add something like:
"The empty option will be compiled and rendered before we first generate the options"

Narretz · 2017-03-27T10:47:01Z

test/ng/directive/ngOptionsSpec.js

+    var repaint = 0;
+    element[0].addEventListener('DOMSubtreeModified', function(ev) {
+      repaint++;
+      expect(repaint).toBeLessThan(2);


Firefox 47 failed this expect. Probably because DOMSubtreeModifiedbehaves differently in it. Maybe we should use MutationObserver instead for Browsers that support it?

Avoid double execution of `updateOptions()` method, which causes a complete repainting of all `<option>` elements. Fixes #15801

ngOptions should ignore comments within a `<select>`element and keep only the `` comment of the empty option. Ref #15812

…tions Since 97b3e00, ngOptions are repainting the options twice when it initializes. This test verifies that the options arent unnecessarily repainted. Ref #15812

Narretz · 2017-04-04T15:01:18Z

test/ng/directive/ngOptionsSpec.js

+      'ng-model': 'value'
+    });
+
+    // Add ngOptions as attribute before recompiling the select element


Thanks for clarifying why you did it like this ;)
However, recompiling is generally a bad idea, and I wouldn't put it in a test. The better option is to add the listener via a custom directive.

Narretz · 2017-04-04T15:02:09Z

Unfortunately, Firefox still reports 2 paints ... :(

Fix included from angular#15812

Avoid double execution of `updateOptions()` method, which causes a complete repainting of all `<option>` elements. Fixes angular#15801 Closes angular#15812

Avoid double execution of `updateOptions()` method, which causes a complete repainting of all `<option>` elements. Fixes angular#15801 Closes angular#15812 Close angular#16071

Avoid double execution of `updateOptions()` method, which causes a complete repainting of all `<option>` elements. Fixes #15801 Closes #15812 Close #16071

googlebot added the cla: yes label Mar 14, 2017

Narretz added component: forms type: bug type: perf labels Mar 15, 2017

Narretz added this to the 1.6.x milestone Mar 15, 2017

Narretz added the needs: work label Mar 15, 2017

pbr1111 closed this Mar 17, 2017

pbr1111 reopened this Mar 17, 2017

pbr1111 closed this Mar 17, 2017

pbr1111 reopened this Mar 17, 2017

pbr1111 closed this Mar 17, 2017

pbr1111 reopened this Mar 17, 2017

Narretz self-assigned this Mar 17, 2017

gkalpak reviewed Mar 20, 2017

View reviewed changes

Narretz suggested changes Mar 27, 2017

View reviewed changes

pbr1111 added 3 commits April 2, 2017 16:25

perf(ngOptions): prevent initial options repaiting

4017019

Avoid double execution of `updateOptions()` method, which causes a complete repainting of all `<option>` elements. Fixes #15801

test(ngOptions): ignore comments within a <select> element

7135eeb

ngOptions should ignore comments within a `<select>`element and keep only the `` comment of the empty option. Ref #15812

test(ngOptions): ensure options aren't repainted when initialize ngOp…

8651d9f

…tions Since 97b3e00, ngOptions are repainting the options twice when it initializes. This test verifies that the options arent unnecessarily repainted. Ref #15812

Narretz suggested changes Apr 4, 2017

View reviewed changes

Narretz added a commit to Narretz/angular.js that referenced this pull request Apr 26, 2017

fix(ngOptions): allow option with ngIf to be removed and re-added

0b874c0

Fix included from angular#15812

Narretz mentioned this pull request Jun 27, 2017

perf(ngOptions): prevent initial options repainting #16071

Merged

2 tasks

Narretz closed this in 6572838 Jun 29, 2017

Narretz pushed a commit that referenced this pull request Jun 29, 2017

perf(ngOptions): prevent initial options repainting

ff52b18

Avoid double execution of `updateOptions()` method, which causes a complete repainting of all `<option>` elements. Fixes #15801 Closes #15812 Close #16071

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(ngOptions): prevent initial options repaiting #15812

perf(ngOptions): prevent initial options repaiting #15812

pbr1111 commented Mar 14, 2017 •

edited

Loading

Narretz commented Mar 15, 2017

pbr1111 commented Mar 15, 2017 •

edited

Loading

pbr1111 commented Mar 17, 2017

Narretz commented Mar 17, 2017

pbr1111 commented Mar 17, 2017

Narretz commented Mar 17, 2017

gkalpak Mar 20, 2017

pbr1111 Mar 20, 2017

Narretz Mar 27, 2017

gkalpak Mar 20, 2017

pbr1111 Mar 20, 2017

gkalpak Mar 20, 2017

pbr1111 Mar 20, 2017

pbr1111 commented Mar 20, 2017

Narretz commented Mar 20, 2017 •

edited

Loading

pbr1111 commented Mar 24, 2017

Narretz left a comment

Narretz Mar 27, 2017

Narretz Mar 27, 2017

Narretz Mar 27, 2017

Narretz Mar 27, 2017

Narretz Mar 27, 2017

Narretz Mar 27, 2017

Narretz Mar 27, 2017

Narretz Mar 27, 2017

Narretz Apr 4, 2017

Narretz commented Apr 4, 2017

perf(ngOptions): prevent initial options repaiting #15812

perf(ngOptions): prevent initial options repaiting #15812

Conversation

pbr1111 commented Mar 14, 2017 • edited Loading

Narretz commented Mar 15, 2017

pbr1111 commented Mar 15, 2017 • edited Loading

pbr1111 commented Mar 17, 2017

Narretz commented Mar 17, 2017

pbr1111 commented Mar 17, 2017

Narretz commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbr1111 commented Mar 20, 2017

Narretz commented Mar 20, 2017 • edited Loading

pbr1111 commented Mar 24, 2017

Narretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Apr 4, 2017

pbr1111 commented Mar 14, 2017 •

edited

Loading

pbr1111 commented Mar 15, 2017 •

edited

Loading

Narretz commented Mar 20, 2017 •

edited

Loading