Permalink
Browse files

Add `Element.purge` for cleaning up event listeners and element stora…

…ge keys on elements that will be removed from the page. Make `Element.update` perform similar cleanup automatically. (Andrew Dupont, Tobie Langel)
  • Loading branch information...
savetheclocktower committed May 7, 2010
1 parent 38e85b8 commit c9d6c3ee14747179a1038bc21055f2d6ccd492c4
Showing with 83 additions and 7 deletions.
  1. +6 −4 CHANGELOG
  2. +47 −3 src/dom/dom.js
  3. +30 −0 test/unit/dom_test.js
View
@@ -1,10 +1,12 @@
-Fix issue where `Element.Layout#get` would fail to interpret negative pixel values. (Sebastien Gruhier, Andrew Dupont)
+* Add `Element.purge` for cleaning up event listeners and element storage keys on elements that will be removed from the page. Make `Element.update` perform similar cleanup automatically. (Andrew Dupont, Tobie Langel)
-Fix bugs in layout.js. Add tests for `Element.Layout#toCSS`, `#toObject`, and `#toHash`. (RStankov, Andrew Dupont)
+* Fix issue where `Element.Layout#get` would fail to interpret negative pixel values. (Sebastien Gruhier, Andrew Dupont)
-Add `Element.Layout#toObject` and `Element.Layout.toHash`. (Andrew Dupont)
+* Fix bugs in layout.js. Add tests for `Element.Layout#toCSS`, `#toObject`, and `#toHash`. (RStankov, Andrew Dupont)
-Make `Element.Layout#toCSS` return camelized property names, as expected by `Element.setStyle`. [#1021 state:resolved] (njakobsen, Andrew Dupont)
+* Add `Element.Layout#toObject` and `Element.Layout.toHash`. (Andrew Dupont)
+
+* Make `Element.Layout#toCSS` return camelized property names, as expected by `Element.setStyle`. [#1021 state:resolved] (njakobsen, Andrew Dupont)
*1.7_rc1* (April 1, 2010)
View
@@ -189,6 +189,19 @@ if (!Node.ELEMENT_NODE) {
Element.idCounter = 1;
Element.cache = { };
+// Performs cleanup on an element before it is removed from the page.
+// See `Element#purge`.
+function purgeElement(element) {
+ // Must go first because it relies on Element.Storage.
+ Element.stopObserving(element);
+
+ var uid = element._prototypeUID;
+ if (uid) {
+ element._prototypeUID = void 0;
+ delete Element.Storage[uid];
+ }
+}
+

This comment has been minimized.

Show comment
Hide comment
@RStankov

RStankov May 12, 2010

Contributor

Since the event handlers are stored in Element.Storage, if element have event handlers it will have uid, so Element.stopObserving(element); could be moved after if (uid){} before element._prototypeUID = void 0;. And in this way it will speedup this method for elements who don't have storage, for them it will be just a empty call

@RStankov

RStankov May 12, 2010

Contributor

Since the event handlers are stored in Element.Storage, if element have event handlers it will have uid, so Element.stopObserving(element); could be moved after if (uid){} before element._prototypeUID = void 0;. And in this way it will speedup this method for elements who don't have storage, for them it will be just a empty call

This comment has been minimized.

Show comment
Hide comment
@savetheclocktower

savetheclocktower May 12, 2010

Collaborator

Works for me.

@savetheclocktower

savetheclocktower May 12, 2010

Collaborator

Works for me.

/**
* mixin Element.Methods
*
@@ -449,6 +462,11 @@ Element.Methods = {
* Note that this method allows seamless content update of table related
* elements in Internet Explorer 6 and beyond.
*
+ * Any nodes replaced with `Element.update` will first have event
+ * listeners unregistered and storage keys removed. This frees up memory
+ * and prevents leaks in certain versions of Internet Explorer. (See
+ * [[Element.purge]]).
+ *
* ##### Examples
*
* language: html
@@ -551,7 +569,14 @@ Element.Methods = {
function update(element, content) {
element = $(element);
-
+
+ // Purge the element's existing contents of all storage keys and
+ // event listeners, since said content will be replaced no matter
+ // what.
+ var descendants = Element.select(element, '*'),
+ i = descendants.length;
+ while (i--) purgeElement(descendants[i]);
+

This comment has been minimized.

Show comment
Hide comment
@RStankov

RStankov May 12, 2010

Contributor

I'm wondering if here we could move this into a method purgeElementDescendants, because here and in purge method are these 3 lines.
Plus for speed I think you could element.getElementsByTagName('*') so it won't pass through selector engine and whole bunch of unneeded stuff ( + on IE it will extend all element, and this is not needed here, since they will be removed any way)

@RStankov

RStankov May 12, 2010

Contributor

I'm wondering if here we could move this into a method purgeElementDescendants, because here and in purge method are these 3 lines.
Plus for speed I think you could element.getElementsByTagName('*') so it won't pass through selector engine and whole bunch of unneeded stuff ( + on IE it will extend all element, and this is not needed here, since they will be removed any way)

This comment has been minimized.

Show comment
Hide comment
@savetheclocktower

savetheclocktower May 12, 2010

Collaborator

Works for me.

@savetheclocktower

savetheclocktower May 12, 2010

Collaborator

Works for me.

if (content && content.toElement)
content = content.toElement();
@@ -3715,8 +3740,8 @@ Element.addMethods({
uid = 0;
} else {
if (typeof element._prototypeUID === "undefined")
- element._prototypeUID = [Element.Storage.UID++];
- uid = element._prototypeUID[0];
+ element._prototypeUID = Element.Storage.UID++;

This comment has been minimized.

Show comment
Hide comment
@kangax

kangax May 8, 2010

Collaborator

Doesn't making "_prototypeUID" a primitive affects html representation in IE (outerHTML, etc.)?

@kangax

kangax May 8, 2010

Collaborator

Doesn't making "_prototypeUID" a primitive affects html representation in IE (outerHTML, etc.)?

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie May 8, 2010

Collaborator

It was essentially an issue with cloning, if I remember correctly.

@tobie

tobie May 8, 2010

Collaborator

It was essentially an issue with cloning, if I remember correctly.

This comment has been minimized.

Show comment
Hide comment
@kangax

kangax May 8, 2010

Collaborator

Well IIRC, cloneNode in IE clones elements in such way that expandos reference original objects, so that could definitely be a problem. How about making Element#clone clean-up "_prototypeUID"?

@kangax

kangax May 8, 2010

Collaborator

Well IIRC, cloneNode in IE clones elements in such way that expandos reference original objects, so that could definitely be a problem. How about making Element#clone clean-up "_prototypeUID"?

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie May 8, 2010

Collaborator

I pretty much think that's what it does already.

@tobie

tobie May 8, 2010

Collaborator

I pretty much think that's what it does already.

This comment has been minimized.

Show comment
Hide comment
@kangax

kangax May 8, 2010

Collaborator

Ok. So there are no issues with leaving property as non-primitive then.

@kangax

kangax May 8, 2010

Collaborator

Ok. So there are no issues with leaving property as non-primitive then.

+ uid = element._prototypeUID;
}
if (!Element.Storage[uid])
@@ -3786,5 +3811,24 @@ Element.addMethods({
}
}
return Element.extend(clone);
+ },
+
+ /**
+ * Element.purge(@element) -> null
+ *
+ * Removes all event listeners and storage keys from an element.
+ *
+ * To be used just before removing an element from the page.
+ **/
+ purge: function(element) {
+ if (!(element = $(element))) return;
+ purgeElement(element);
+
+ var descendants = Element.select(element, '*'),
+ i = descendants.length;
+
+ while (i--) purgeElement(descendants[i]);
+
+ return null;

This comment has been minimized.

Show comment
Hide comment
@RStankov

RStankov May 25, 2010

Contributor

Here should be "return element", In most of my code I expect to do $(element).purge().remove();

@RStankov

RStankov May 25, 2010

Contributor

Here should be "return element", In most of my code I expect to do $(element).purge().remove();

This comment has been minimized.

Show comment
Hide comment
@savetheclocktower

savetheclocktower May 25, 2010

Collaborator

My thinking here was that return null would prevent chaining on purpose, as if to emphasize that you shouldn't be calling any methods on this element after it's purged. So you'd do $(element).remove().purge(). But then that means the documentation is wrong and should say "To be used just after removing an element from the page."

I'm happy to change it so that it returns the element, though. I'll check with Tobie and Sam and see if they have an opinion.

@savetheclocktower

savetheclocktower May 25, 2010

Collaborator

My thinking here was that return null would prevent chaining on purpose, as if to emphasize that you shouldn't be calling any methods on this element after it's purged. So you'd do $(element).remove().purge(). But then that means the documentation is wrong and should say "To be used just after removing an element from the page."

I'm happy to change it so that it returns the element, though. I'll check with Tobie and Sam and see if they have an opinion.

This comment has been minimized.

Show comment
Hide comment
@madrobby

madrobby May 25, 2010

Collaborator

+1 for chaining in this case. The order really shouldn't matter.

Maybe we can add a destroy() convenience method to purge and remove at the same time?

@madrobby

madrobby May 25, 2010

Collaborator

+1 for chaining in this case. The order really shouldn't matter.

Maybe we can add a destroy() convenience method to purge and remove at the same time?

This comment has been minimized.

Show comment
Hide comment
@tobie

tobie May 25, 2010

Collaborator

I think returning the element is more consistant. Calling a method on the element after having purged it doesn't have any side effects, so it's ok to do that. And yes, adding Element#destroy was sort of planned for a future release.

@tobie

tobie May 25, 2010

Collaborator

I think returning the element is more consistant. Calling a method on the element after having purged it doesn't have any side effects, so it's ok to do that. And yes, adding Element#destroy was sort of planned for a future release.

This comment has been minimized.

Show comment
Hide comment
@savetheclocktower

savetheclocktower May 25, 2010

Collaborator

Adding destroy was discussed, too, but because this happened so late in the 1.7 release cycle Tobie and I decided to add as few methods as possible.

@savetheclocktower

savetheclocktower May 25, 2010

Collaborator

Adding destroy was discussed, too, but because this happened so late in the 1.7 release cycle Tobie and I decided to add as few methods as possible.

}
});
View
@@ -1501,6 +1501,36 @@ new Test.Unit.Runner({
this.assert(deepClone.firstChild);
this.assertEqual('SPAN', deepClone.firstChild.nodeName.toUpperCase());
this.assert(!deepClone.down('span')._prototypeUID);
+ },
+
+ testElementPurge: function() {
+ var element = new Element('div');
+ element.store('foo', 'bar');
+
+ var uid = element._prototypeUID;
+ this.assert(uid in Element.Storage, "newly-created element's uid should exist in `Element.Storage`");
+
+ element.purge();
+
+ this.assert(!(uid in Element.Storage), "purged element's UID should no longer exist in `Element.Storage`");
+ this.assert(!(Object.isNumber(element._prototypeUID)), "purged element's UID should no longer exist in `Element.Storage`");
+
+ // Should purge elements replaced via innerHTML.
+ var parent = new Element('div');
+ var child = new Element('p').update('lorem ipsum');
+
+ parent.insert(child);
+ child.store('foo', 'bar');
+ child.observe('test:event', function(event) { event.stop(); });
+ var childUID = child._prototypeUID;
+
+ parent.update("");
+
+ // At this point, `child` should have been purged.
+ this.assert(!(childUID in Element.Storage), "purged element's UID should no longer exist in `Element.Storage`");
+
+ var event = child.fire('test:event');
+ this.assert(!event.stopped, "fired event should not have been stopped");
}
});

0 comments on commit c9d6c3e

Please sign in to comment.