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

Repeater fixes #1602

Closed
wants to merge 6 commits into from
Closed

Repeater fixes #1602

wants to merge 6 commits into from

Conversation

IgorMinar
Copy link
Contributor

I have reverted the original fix for #933 because it was making the repeater unstable and in general was causing a lot of churn in the DOM. The current fix via ngModel is not ideal, but better IMO.

I also added proper fix for #1076. Similar fix was originally submitted by an external contributor, Misko dismissed it and accepted only tests because these tests were now passing thanks to the repeater changes - but that was only because the repeater was inefficient and was rebuilding the dom on every digest.

This commits adds test for angular#933 and simplifies the implementation of the fix

Closes angular#933
In cases when we reuse elements in a repeater but associate
them with a new scope (see angular#933 - repeating over array of
primitives) it's possible for the internal ngModel state and
the scope state to get out of sync. This change ensure that
the two are always sync-ed up even in cases where we
reassociate an element with a different (but similar) scope.

In the case of repeating over array of primitives it's still
possible to run into issue if we iterate over primitives and
use form controls or similar widgets without ngModel - oh well,
we'd likely need a special repeater for primitives to deal
with this properly, even then there might be cornercases.

Closes angular#933
I'm reverting changes that were originally done to ngRepeat to fix angular#933,
because these are now not necessary after the previous changes to keep
ngModel always synced with the DOM.
We need to watch $index in addition to cssClasses because only then
we can correctly respond to shrinking or reordered repeaters.

Closes angular#1076
@petebacondarwin
Copy link
Member

HI @IgorMinar,

This repeater fix still has a problem with input controls that are bound to model[$index] losing focus when the input changes.

See this Plnkr: http://plnkr.co/edit/aEIdRO?p=preview. I have set it up to use your build of AngularJS (this branch).

Not sure how I make a unit test to demonstrate this.

I think what you need to do, if you see the my-repeat directive, is not to create a new scope if the value that is being iterated is a primitive, but instead to just update the model. This also has a performance improvement that the DOM is not being modified as much - i.e. only items bound to the changed model have to update, whereas at the moment the whole transclusion is being destroyed and rebuild on every change.

Pete

@mhevery
Copy link
Contributor

mhevery commented Nov 25, 2012

I don't fully understand this, and I don't see how you can automatically switch between index affinity and object affinity, so I am skeptical of this. Could you better explain the logic of the repeater.

Also I think we should write a $watch function which is a simple look which '===' elements to see if anything changes before going through the expensive repeat algorithm.

@petebacondarwin
Copy link
Member

This unit test needs to pass:

  describe('stability', function() {

    ...

    it('should not reorder elements that have primitive model changes, since this causes loss of focus on input elements', function() {
      scope.items = ['hello', 'cau', 'ahoj'];
      scope.$digest();
      lis = element.find('li');

      scope.items = ['hello', 'ahoj', 'ahoj'];
      scope.$digest();
      var newLis = element.find('li');
      expect(newLis.length).toEqual(3);
      expect(newLis[1]).not.toEqual(lis[2]);
    });
  ...

Currently if you change items[1] to equal items[2], the repeater moves the elements around and any input box that was in the 1th position loses focus. Trying to move scopes/elements for primitives values is too prone to error. If it is to be done it needs to ensure that you are not stealing a different items element/scope.

I believe these are cases that need to be considered:

  1. last === next (object or primitive are exactly identical) -> No change
  2. both are primitives -> reuse scope/element but change scope model value
  3. last is object, next is primitive -> delete object scope/element if it no longer appears in next list; create new scope/element for new primitive
  4. last is primitive, next is object -> reuse scope/element if it appears in last list or create new; delete primitive's last scope/element
  5. last and next are objects -> reuse scope/element if it appears in last list or create new; delete objects last scope/element if it no longer appears

This will mean that objects are reused if they move and that scope/elements are reused if their index doesn't change. This seems like the least likely to cause application developer confusion.

@vojtajina
Copy link
Contributor

Just reviewed with Igor. I agree with merging this.

This only fixes the symptom, not the issue.

Long term, let's try the Misko's idea for optimizing ng-repeat and try to have a different implementation of ng-repeat for primitives.

@mhevery
Copy link
Contributor

mhevery commented Nov 26, 2012

+1 if you feel it should be merged.
+1 having different repeaters

On Mon, Nov 26, 2012 at 2:46 PM, Vojta Jina notifications@github.comwrote:

Just reviewed with Igor. I agree with merging this.

This only fixes the symptom, not the issue.

Long term, let's try the Misko's idea for optimizing ng-repeat and try to
have a different implementation of ng-repeat for primitives.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1602#issuecomment-10715800.

@petebacondarwin
Copy link
Member

Can you please address the issue of input boxes losing focus when you change a primitive type?
http://plnkr.co/edit/cefpO6?p=preview
Just try typing into any of the three input boxes. It is unusable in this state.

@IgorMinar
Copy link
Contributor Author

@petebacondarwin yes, this change fails the input box focus test. this is an expected behavior because the repeater uses identity affinity instead of index affinity.

the solution you described relies purely on index affinity, which means that would make the repeater unstable (there is no affinity between model identity and cloned DOM view). the stability is an important feature that you need when you start building more complex uis with repeaters (and these are never built with model composed of primitive values).

the test you wrote proves that the repeater is unstable and uses index affinity.

as misko mentioned the more we think about this the more it looks like the best choice for us is to internally implement stable repeater with affinity to object identity for objects/non-primitive values and unstable repeater (one with affinity to index/position) for primitives.

I looked at your whatChanged code briefly and I like what I see, I'm going to play with it more, but as a quick fix I'd like get a smaller low-risk change that while not perfect improves the current situation.

@IgorMinar
Copy link
Contributor Author

@mhevery

I don't fully understand this, and I don't see how you can automatically switch between index affinity and object affinity, so I am skeptical of this. Could you better explain the logic of the repeater.

the current change doesn't try to use index affinity for changing primitive values. it always uses object affinity, which turns into lame index affinity for primitives.

Also I think we should write a $watch function which is a simple look which '===' elements to see if anything changes before going through the expensive repeat algorithm.

yes, but that's for a separate PR.

@IgorMinar
Copy link
Contributor Author

@petebacondarwin how many and which of the existing ngRepeat tests fail if you try to use your repeater instead of ours?

@petebacondarwin
Copy link
Member

Hi Igor,
Thanks for looking at this. I just took the current ng-repeat.spec.js from
your branch.

There are only 2 tests failing - once I had removed dealloc, changed
jqLite to angular.element and also commented out tests for iterating over
object keys (i.e. of the form ng-repeat="(key, value) in collection)"

  1. One is your new test "myRepeat stability should reuse elements even
    when model is composed of primitives FAILED".
    *With my repeater this is by design - it treats primitives with index
    affinity rather than trying to match their values.
    *

  2. The other one is slightly more worrying but I am sure it is just a
    small bug. It seems not to pick up on the shifted array element:

    Chrome 23.0 myRepeat should iterate over an array of objects FAILED
    Expected 2 to equal 1.
    Error: Expected 2 to equal 1.
    at null.
    (C:/Users/pete/dev/tech/angular/repeat/test/unit/ng-repeat.spec.js:43:39)
    Expected 'shyam;misko;' to equal 'shyam;'.
    Error: Expected 'shyam;misko;' to equal 'shyam;'.
    at null.
    (C:/Users/pete/dev/tech/angular/repeat/test/unit/ng-repeat.spec.js:44:28)

Tomorrow I will add back in the object key iteration, which is fairly
simple, and fix this other test. Then you can take a see what you think.

Thanks again,
Pete

On 26 November 2012 15:54, Igor Minar notifications@github.com wrote:

@petebacondarwin https://github.com/petebacondarwin how many of the
existing ngRepeat tests fail if you try to use your repeater instead of
ours?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1602#issuecomment-10720546.

@IgorMinar
Copy link
Contributor Author

@petebacondarwin sounds good. I'm going to go ahead and merge what we have so far and then we can work with your changes and see if they can resolve the remaining issues.

@petebacondarwin
Copy link
Member

OK. Thanks.

On 26 November 2012 19:15, Igor Minar notifications@github.com wrote:

@petebacondarwin https://github.com/petebacondarwin sounds good. I'm
going to go ahead and merge what we have so far and then we can work with
your changes and see if they can resolve the remaining issues.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1602#issuecomment-10729298.

@IgorMinar
Copy link
Contributor Author

this landed. @petebacondarwin please open a new PR with your changes.

@pkozlowski-opensource
Copy link
Member

@IgorMinar @petebacondarwin Things got definitivelly better in 1.0.3 but there are still some issues:
http://jsfiddle.net/68CYt/3/

Opened issue: #1389

I guess you are aware of this one but just for the record.

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

Successfully merging this pull request may close these issues.

None yet

5 participants