Skip to content

Commit

Permalink
Fix bug where resize() isn't called on some widgets
Browse files Browse the repository at this point in the history
If there were multiple static widgets in the same document, not
all would receive resize() calls. Repro case is to create an
R Markdown ioslides presentation, and on slide 4 add one leaflet
map, and on slide 5 add another one. Only the second one would
be rendered properly. Here's an example, see slide 4:

https://rawgit.com/ldecicco-USGS/toxEval/master/samplePres.html

This was due to me using function-scoped variables inside of a
loop that created closures:

for (var i = 0; i < bindings.length; i++) {
  var binding = bindings[i];
  var matches = binding.find(document.documentElement);
  for (var j = 0; j < matches.length; j++) {
    var el = matches[j];
    function resizeHandler() {
      // do stuff with binding and el
    }
  }
}

The binding and el variables are scoped to the entire function,
so all the iterations all share the same variable, and therefore
all the resizeHandler closures are bound to the same binding and
el values (the final ones).

The solution is to change the for-loops into forEach calls, since
the callback function into forEach will result in an isolated
scope being created for each iteration.
  • Loading branch information
jcheng5 committed May 21, 2015
1 parent 86994f5 commit 1582c46
Showing 1 changed file with 16 additions and 7 deletions.
23 changes: 16 additions & 7 deletions inst/www/htmlwidgets.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,17 @@
return target;
}

// IE8 doesn't support Array.forEach.
function forEach(values, callback, thisArg) {
if (values.forEach) {
values.forEach(callback, thisArg);
} else {
for (var i = 0; i < values.length; i++) {
callback.call(thisArg, values[i], i, values);
}
}
}

// Replaces the specified method with the return value of funcSource.
//
// Note that funcSource should not BE the new method, it should be a function
Expand Down Expand Up @@ -431,15 +442,13 @@
// Statically render all elements that are of this widget's class
window.HTMLWidgets.staticRender = function() {
var bindings = window.HTMLWidgets.widgets || [];
for (var i = 0; i < bindings.length; i++) {
var binding = bindings[i];
forEach(bindings, function(binding) {
var matches = binding.find(document.documentElement);
for (var j = 0; j < matches.length; j++) {
var el = matches[j];
forEach(matches, function(el) {
var sizeObj = initSizing(el, binding);

if (hasClass(el, "html-widget-static-bound"))
continue;
return;
el.className = el.className + " html-widget-static-bound";

var initResult;
Expand Down Expand Up @@ -504,8 +513,8 @@
}
binding.renderValue(el, data.x, initResult);
}
}
}
});
});
}

// Wait until after the document has loaded to render the widgets.
Expand Down

1 comment on commit 1582c46

@timelyportfolio
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is related in some way to #102 . Thanks for finding and fixing.

Please sign in to comment.