Skip to content

Commit

Permalink
Fix bug in template renderer
Browse files Browse the repository at this point in the history
  • Loading branch information
kentmw committed Jun 15, 2015
1 parent 21d7dd1 commit 7fdd5f7
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 16 deletions.
21 changes: 13 additions & 8 deletions modules/templateRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
function swapElementNodes(currentNode, newNode, ignoreElements) {
var $currentNode = $(currentNode),
$newNode = $(newNode),
idx = 0,
shouldIgnore,
$currChildNodes,
$newChildNodes,
Expand All @@ -65,10 +66,14 @@
return;
}

// Remove current attributes
// Remove current attributes that have changed
currentAttributes = currentNode.attributes;
while (currentAttributes.length > 0) {
currentNode.removeAttribute(currentAttributes[0].name);
while (idx < currentAttributes.length) {

This comment has been minimized.

Copy link
@andyhuang91

andyhuang91 Jun 22, 2015

This is a bug. You aren't actually iterating over all elements. currentNode.attributes is a NamedNodeMap, which is a "live" array representing the actual state of the DOM element at all times. In this case, each time you remove an element by calling currentNode.removeAttribute, you shorten the length of currentAttributes. Since you still always call idx++, you'll end up skipping over every attribute that occurs immediately after a removed attribute.

Also, what's the point of removing only the attributes that non-longer exist (as opposed to simply removing all attributes) if the next block manually sets all attributes from newNode anyways?

This comment has been minimized.

Copy link
@arikwex

arikwex Jun 22, 2015

Contributor

"Also, what's the point of removing only the attributes that non-longer exist (as opposed to simply removing all attributes) if the next block manually sets all attributes from newNode anyways?"

That's what we originally had here. Turns out that some DOM elements (like svg elements) aren't happy when you remove certain attributes. The templateRenderer would crash on an "illegal" removeAttribute, so we moved to this way. Good catch on this bug though. Hope it didn't plague you for too long :-\

currentAttr = currentAttributes[idx].name;
if (!_.contains(currentAttr, newNode.attributes)) {
currentNode.removeAttribute(currentAttr);
}
idx++;
}

// Set new attributes
Expand Down Expand Up @@ -196,16 +201,16 @@
});
return newDOM;
},
/**

/**
* Determines if the element supports selection. As per spec, https://html.spec.whatwg.org/multipage/forms.html#do-not-apply
* selection is only allowed for text, search, tel, url, password. Other input types will throw an exception in chrome
* @param el {Element} the DOM element to check
* @return {Boolean} boolean indicating whether or not the selection is allowed for {Element} el
* @param el {Element} the DOM element to check
* @return {Boolean} boolean indicating whether or not the selection is allowed for {Element} el
* @method supportsSelection
*/
supportsSelection : function (el) {
return (/text|password|search|tel|url/).test(el.type);
return (/text|password|search|tel|url/).test(el.type);
},

/**
Expand Down
28 changes: 22 additions & 6 deletions torso-bundle.js
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@
function swapElementNodes(currentNode, newNode, ignoreElements) {
var $currentNode = $(currentNode),
$newNode = $(newNode),
idx = 0,
shouldIgnore,
$currChildNodes,
$newChildNodes,
Expand All @@ -296,10 +297,14 @@
return;
}

// Remove current attributes
// Remove current attributes that have changed
currentAttributes = currentNode.attributes;
while (currentAttributes.length > 0) {
currentNode.removeAttribute(currentAttributes[0].name);
while (idx < currentAttributes.length) {
currentAttr = currentAttributes[idx].name;
if (!_.contains(currentAttr, newNode.attributes)) {
currentNode.removeAttribute(currentAttr);
}
idx++;
}

// Set new attributes
Expand Down Expand Up @@ -402,11 +407,11 @@
hotswapKeepCaret: function(currentNode, newNode, ignoreElements) {
var currentCaret,
activeElement = document.activeElement;
if (activeElement && activeElement.hasAttribute('value')) {
if (activeElement && activeElement.hasAttribute('value') && this.supportsSelection(activeElement)) {
currentCaret = this.getCaretPosition(activeElement);
}
this.hotswap(currentNode, newNode, ignoreElements);
if (activeElement) {
if (activeElement && this.supportsSelection(activeElement)) {
this.setCaretPosition(activeElement, currentCaret);
}
},
Expand All @@ -428,6 +433,17 @@
return newDOM;
},

/**
* Determines if the element supports selection. As per spec, https://html.spec.whatwg.org/multipage/forms.html#do-not-apply
* selection is only allowed for text, search, tel, url, password. Other input types will throw an exception in chrome
* @param el {Element} the DOM element to check
* @return {Boolean} boolean indicating whether or not the selection is allowed for {Element} el
* @method supportsSelection
*/
supportsSelection : function (el) {
return (/text|password|search|tel|url/).test(el.type);
},

/**
* Method that returns the current caret (cursor) position of a given element.
* Source: http://stackoverflow.com/questions/2897155/get-cursor-position-in-characters-within-a-text-input-field
Expand Down Expand Up @@ -3470,7 +3486,6 @@
* Override to add more functionality but remember to call this.listViewSetup(args) first
* @method initialize
* @param args {Object} - options argument
* @param args.childModel {String} - name of the model argument passed to the child view during initialization
* @param args.childView {Backbone.View definition} - the class definition of the child view. This view will be instantiated
* for every model returned by modelsToRender()
* @param args.collection {Backbone.Collection instance} - The collection that will back this list view. A subclass of list view
Expand All @@ -3485,6 +3500,7 @@
* functionality.
* @param [args.renderWait=0] {Numeric} - If provided, will collect any internally invoked renders (typically through collection events like reset) for a duration specified by renderWait in milliseconds and then calls a single render instead. Helps to remove unnecessary render calls when modifying the collection often.
* @param [args.modelId='cid'] {String} - model property used as identifier for a given model. This property is saved and used to find the corresponding view.
* @param [args.childModel='model'] {String} - name of the model argument passed to the child view during initialization
*/
initialize: function(args) {
this.super();
Expand Down
Loading

0 comments on commit 7fdd5f7

Please sign in to comment.