From ae4177b829b9138f398abbfee243c5d1af6b261a Mon Sep 17 00:00:00 2001 From: webchick Date: Mon, 23 Dec 2013 12:50:36 -0800 Subject: [PATCH] Issue #2159965 by Wim Leers, jessebeach: Fix two memory leaks + subtle bugs in Edit's JS (discovered by working on the Backbone upgrade in the D7 backport). --- core/modules/edit/js/editors/formEditor.js | 4 +- core/modules/edit/js/models/EntityModel.js | 98 +++++++++++-------- core/modules/edit/js/models/FieldModel.js | 7 +- core/modules/edit/js/views/AppView.js | 55 +++++++---- .../edit/js/views/ContextualLinkView.js | 2 +- .../edit/js/views/EntityToolbarView.js | 14 ++- 6 files changed, 107 insertions(+), 73 deletions(-) diff --git a/core/modules/edit/js/editors/formEditor.js b/core/modules/edit/js/editors/formEditor.js index 2d8fb8ed270..7eda4e1b630 100644 --- a/core/modules/edit/js/editors/formEditor.js +++ b/core/modules/edit/js/editors/formEditor.js @@ -183,7 +183,9 @@ Drupal.edit.editors.form = Drupal.edit.EditorView.extend({ fieldModel.set('htmlForOtherViewModes', response.other_view_modes); // Finally, set the 'html' attribute on the field model. This will cause // the field to be rerendered. - fieldModel.set('html', response.data); + _.defer(function () { + fieldModel.set('html', response.data); + }); }; // Unsuccessfully saved; validation errors. diff --git a/core/modules/edit/js/models/EntityModel.js b/core/modules/edit/js/models/EntityModel.js index 913abb3b831..7358bcbf50d 100644 --- a/core/modules/edit/js/models/EntityModel.js +++ b/core/modules/edit/js/models/EntityModel.js @@ -81,9 +81,11 @@ Drupal.edit.EntityModel = Backbone.Model.extend({ var to = state; switch (to) { case 'closed': - this.set('isActive', false); - this.set('inTempStore', false); - this.set('isDirty', false); + this.set({ + 'isActive': false, + 'inTempStore': false, + 'isDirty': false + }); break; case 'launching': @@ -103,18 +105,9 @@ Drupal.edit.EntityModel = Backbone.Model.extend({ case 'committing': // The user indicated they want to save the entity. - // For fields already in a candidate-ish state, trigger a change - // event so that the entityModel can move to the next state in - // committing. - this.get('fields').chain() - .filter(function (fieldModel) { - return _.intersection([fieldModel.get('state')], Drupal.edit.app.readyFieldStates).length; - }) - .each(function (fieldModel) { - fieldModel.trigger('change:state', fieldModel, fieldModel.get('state'), options); - }); + var fields = this.get('fields'); // For fields that are in an active state, transition them to candidate. - this.get('fields').chain() + fields.chain() .filter(function (fieldModel) { return _.intersection([fieldModel.get('state')], ['active']).length; }) @@ -123,7 +116,7 @@ Drupal.edit.EntityModel = Backbone.Model.extend({ }); // For fields that are in a changed state, field values must first be // stored in TempStore. - this.get('fields').chain() + fields.chain() .filter(function (fieldModel) { return _.intersection([fieldModel.get('state')], Drupal.edit.app.changedFieldStates).length; }) @@ -192,18 +185,56 @@ Drupal.edit.EntityModel = Backbone.Model.extend({ } }, + /** + * Updates a Field and Entity model's "inTempStore" when appropriate. + * + * Helper function. + * + * @param Drupal.edit.EntityModel entityModel + * The model of the entity for which a field's state attribute has changed. + * @param Drupal.edit.FieldModel fieldModel + * The model of the field whose state attribute has changed. + * + * @see fieldStateChange() + */ + _updateInTempStoreAttributes: function (entityModel, fieldModel) { + var current = fieldModel.get('state'); + var previous = fieldModel.previous('state'); + var fieldsInTempStore = entityModel.get('fieldsInTempStore'); + // If the fieldModel changed to the 'saved' state: remember that this + // field was saved to TempStore. + if (current === 'saved') { + // Mark the entity as saved in TempStore, so that we can pass the + // proper "reset TempStore" boolean value when communicating with the + // server. + entityModel.set('inTempStore', true); + // Mark the field as saved in TempStore, so that visual indicators + // signifying just that may be rendered. + fieldModel.set('inTempStore', true); + // Remember that this field is in TempStore, restore when rerendered. + fieldsInTempStore.push(fieldModel.get('fieldID')); + fieldsInTempStore = _.uniq(fieldsInTempStore); + entityModel.set('fieldsInTempStore', fieldsInTempStore); + } + // If the fieldModel changed to the 'candidate' state from the + // 'inactive' state, then this is a field for this entity that got + // rerendered. Restore its previous 'inTempStore' attribute value. + else if (current === 'candidate' && previous === 'inactive') { + fieldModel.set('inTempStore', _.intersection([fieldModel.get('fieldID')], fieldsInTempStore).length > 0); + } + }, + /** * Reacts to state changes in this entity's fields. * * @param Drupal.edit.FieldModel fieldModel - * The model of the field whose state property changed. + * The model of the field whose state attribute changed. * @param String state * The state of the associated field. One of Drupal.edit.FieldModel.states. */ fieldStateChange: function (fieldModel, state) { var entityModel = this; var fieldState = state; - var fieldsInTempStore = this.get('fieldsInTempStore'); // Switch on the entityModel state. // The EntityModel responds to FieldModel state changes as a function of its // state. For example, a field switching back to 'candidate' state when its @@ -245,43 +276,22 @@ Drupal.edit.EntityModel = Backbone.Model.extend({ if (fieldState === 'changed') { entityModel.set('isDirty', true); } - // If the fieldModel changed to the 'saved' state: remember that this - // field was saved to TempStore. - else if (fieldState === 'saved') { - // Mark the entity as saved in TempStore, so that we can pass the - // proper "reset TempStore" boolean value when communicating with the - // server. - entityModel.set('inTempStore', true); - // Mark the field as saved in TempStore, so that visual indicators - // signifying just that may be rendered. - fieldModel.set('inTempStore', true); - // Remember that this field is in TempStore, restore when rerendered. - fieldsInTempStore.push(fieldModel.get('fieldID')); - fieldsInTempStore = _.uniq(fieldsInTempStore); - entityModel.set('fieldsInTempStore', fieldsInTempStore); - } - // If the fieldModel changed to the 'candidate' state from the - // 'inactive' state, then this is a field for this entity that got - // rerendered. Restore its previous 'inTempStore' attribute value. - else if (fieldState === 'candidate' && fieldModel.previous('state') === 'inactive') { - fieldModel.set('inTempStore', _.intersection([fieldModel.get('fieldID')], fieldsInTempStore).length > 0); + else { + this._updateInTempStoreAttributes(entityModel, fieldModel); } break; case 'committing': // If the field save returned a validation error, set the state of the - // entity back to opened. + // entity back to 'opened'. if (fieldState === 'invalid') { // A state change in reaction to another state change must be deferred. _.defer(function() { entityModel.set('state', 'opened', { reason: 'invalid' }); }); } - // If the fieldModel changed to the 'candidate' state from the - // 'inactive' state, then this is a field for this entity that got - // rerendered. Restore its previous 'inTempStore' attribute value. - else if (fieldState === 'candidate' && fieldModel.previous('state') === 'inactive') { - fieldModel.set('inTempStore', _.intersection([fieldModel.get('fieldID')], fieldsInTempStore).length > 0); + else { + this._updateInTempStoreAttributes(entityModel, fieldModel); } // Attempt to save the entity. If the entity's fields are not yet all in @@ -503,6 +513,8 @@ Drupal.edit.EntityModel = Backbone.Model.extend({ destroy: function (options) { Backbone.Model.prototype.destroy.apply(this, options); + this.off(null, null, this); + // Destroy all fields of this entity. this.get('fields').each(function (fieldModel) { fieldModel.destroy(); diff --git a/core/modules/edit/js/models/FieldModel.js b/core/modules/edit/js/models/FieldModel.js index 13f88cc45ba..a9d3021c8da 100644 --- a/core/modules/edit/js/models/FieldModel.js +++ b/core/modules/edit/js/models/FieldModel.js @@ -82,7 +82,7 @@ Drupal.edit.FieldModel = Backbone.Model.extend({ if (this.get('state') !== 'inactive') { throw new Error("FieldModel cannot be destroyed if it is not inactive state."); } - Backbone.Model.prototype.destroy.apply(this, options); + Backbone.Model.prototype.destroy.call(this, options); }, /** @@ -97,11 +97,6 @@ Drupal.edit.FieldModel = Backbone.Model.extend({ * {@inheritdoc} */ validate: function (attrs, options) { - // We only care about validating the 'state' attribute. - if (!_.has(attrs, 'state')) { - return; - } - var current = this.get('state'); var next = attrs.state; if (current !== next) { diff --git a/core/modules/edit/js/views/AppView.js b/core/modules/edit/js/views/AppView.js index 2d85d2ebb24..3c842f26525 100644 --- a/core/modules/edit/js/views/AppView.js +++ b/core/modules/edit/js/views/AppView.js @@ -436,31 +436,48 @@ Drupal.edit.AppView = Backbone.View.extend({ var $fieldWrapper = $(fieldModel.get('el')); var $context = $fieldWrapper.parent(); + var renderField = function () { + // Destroy the field model; this will cause all attached views to be + // destroyed too, and removal from all collections in which it exists. + fieldModel.destroy(); + + // Replace the old content with the new content. + $fieldWrapper.replaceWith(html); + + // Attach behaviors again to the modified piece of HTML; this will + // create a new field model and call rerenderedFieldToCandidate() with + // it. + Drupal.attachBehaviors($context); + }; + // When propagating the changes of another instance of this field, this // field is not being actively edited and hence no state changes are // necessary. So: only update the state of this field when the rerendering - // of this field happens not because of propagation, but because it is being - // edited itself. + // of this field happens not because of propagation, but because it is + // being edited itself. if (!options.propagation) { - // First set the state to 'candidate', to allow all attached views to - // clean up all their "active state"-related changes. - fieldModel.set('state', 'candidate'); + // Deferred because renderUpdatedField is reacting to a field model change + // event, and we want to make sure that event fully propagates before + // making another change to the same model. + _.defer(function () { + // First set the state to 'candidate', to allow all attached views to + // clean up all their "active state"-related changes. + fieldModel.set('state', 'candidate'); - // Set the field's state to 'inactive', to enable the updating of its DOM - // value. - fieldModel.set('state', 'inactive', { reason: 'rerender' }); + // Similarly, the above .set() call's change event must fully propagate + // before calling it again. + _.defer(function () { + // Set the field's state to 'inactive', to enable the updating of its + // DOM value. + fieldModel.set('state', 'inactive', { reason: 'rerender' }); + + renderField(); + }); + }); + } + else { + renderField(); } - - // Destroy the field model; this will cause all attached views to be - // destroyed too, and removal from all collections in which it exists. - fieldModel.destroy(); - - // Replace the old content with the new content. - $fieldWrapper.replaceWith(html); - - // Attach behaviors again to the modified piece of HTML; this will create - // a new field model and call rerenderedFieldToCandidate() with it. - Drupal.attachBehaviors($context); }, /** diff --git a/core/modules/edit/js/views/ContextualLinkView.js b/core/modules/edit/js/views/ContextualLinkView.js index 0478d7c93c0..3e9dee6c86f 100644 --- a/core/modules/edit/js/views/ContextualLinkView.js +++ b/core/modules/edit/js/views/ContextualLinkView.js @@ -35,7 +35,7 @@ Drupal.edit.ContextualLinkView = Backbone.View.extend({ */ initialize: function (options) { // Insert the text of the quick edit toggle. - this.$el.find('a').text(this.options.strings.quickEdit); + this.$el.find('a').text(options.strings.quickEdit); // Initial render. this.render(); // Re-render whenever this entity's isActive attribute changes. diff --git a/core/modules/edit/js/views/EntityToolbarView.js b/core/modules/edit/js/views/EntityToolbarView.js index c5844d27355..a2906aa2005 100644 --- a/core/modules/edit/js/views/EntityToolbarView.js +++ b/core/modules/edit/js/views/EntityToolbarView.js @@ -13,9 +13,9 @@ Drupal.edit.EntityToolbarView = Backbone.View.extend({ events: function () { var map = { - 'click.edit button.action-save': 'onClickSave', - 'click.edit button.action-cancel': 'onClickCancel', - 'mouseenter.edit': 'onMouseenter' + 'click button.action-save': 'onClickSave', + 'click button.action-cancel': 'onClickCancel', + 'mouseenter': 'onMouseenter' }; return map; }, @@ -117,7 +117,15 @@ Drupal.edit.EntityToolbarView = Backbone.View.extend({ * {@inheritdoc} */ remove: function () { + // Remove additional DOM elements controlled by this View. this.$fence.remove(); + + // Stop listening to additional events. + this.appModel.off(null, null, this); + this.model.get('fields').off(null, null, this); + $(window).off('resize.edit scroll.edit'); + $(document).off('drupalViewportOffsetChange.edit'); + Backbone.View.prototype.remove.call(this); },