Permalink
Browse files

Fixes a regression with SC.CollectionView that occurred after a previ…

…ous fix to remove a view DOM leak. SC.CollectionView previously created a view with the same layerId as the view to be replaced before replacing it, which only worked because of some weird logic external to the CollectionView that used to prevent a view from being able to find its layer if it didn't have a parentView (which was also the cause of a leak because the layer might still exist in the DOM). In any case, having two views use the same layerId at the same time is a bad idea.

So instead we essentially use removeChild().insertBefore() rather than insertBefore().removeChild() (i.e. replaceChild()).
  • Loading branch information...
1 parent dc44ddf commit 12454580c73c9834f1a905ad6f8a3ae7c8159bbe @publickeating publickeating committed with publickeating Jan 4, 2013
Showing with 10 additions and 6 deletions.
  1. +10 −6 frameworks/desktop/views/collection.js
@@ -965,14 +965,18 @@ SC.CollectionView = SC.View.extend(SC.CollectionViewDelegate, SC.CollectionConte
// …then the redraws…
for (i = 0, len = viewsToRedraw.length; i < len; ++i) {
idx = viewsToRedraw[i];
- existing = itemViews ? itemViews[idx] : null;
+ existing = itemViews[idx];
+
+ // Because the new view will get the same layerId as the old view, we need
+ // to remove the old view first.
+ containerView.removeChild(existing);
+ existing.destroy();
+
+ // Insert the replacement view before the following view.
+ existing = (idx === len - 1) ? null : itemViews[idx + 1];
view = this.itemViewForContentIndex(idx, YES);
- // if the existing view has a layer, remove it immediately from
- // the parent. This is necessary because the old and new views
- // will use the same layerId
- existing.destroyLayer();
- containerView.replaceChild(view, existing);
+ containerView.insertBefore(view, existing);
}
// …and finally the creations.

0 comments on commit 1245458

Please sign in to comment.