Fix #2349, #2329, #1817: bugs triggered by networkD3 sankey plot

* All of these were caused by the presence of multiple body tags on the page, which happened because networkD3's sankey plot generates SVGs containing body tags via SVG's foreignObject tag * In various places, the 'body' jQuery selector string is used under the assumption there is only one 'body' tag on the page. The presence of multiple 'body' tags breaks reliant code in strange ways. * The fix was to use document.body or 'body:first' instead of 'body'.
rstudio · Mar 27, 2019 · 99715d6 · 99715d6
1 parent 20329fe
commit 99715d6
Show file tree

Hide file tree

Showing 11 changed files with 67 additions and 22 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -23,6 +23,8 @@ shiny 1.3.0
 
 * Fixed [#2305](https://github.com/rstudio/shiny/issues/2305), [#2322](https://github.com/rstudio/shiny/issues/2322), [#2351](https://github.com/rstudio/shiny/issues/2351): When an input in dynamic UI is restored from bookmarks, it would keep getting set to the same value. ([#2360](https://github.com/rstudio/shiny/pull/2360))
 
+* Fixed [#2349](https://github.com/rstudio/shiny/issues/2349), [#2329](https://github.com/rstudio/shiny/issues/2329), [#1817](https://github.com/rstudio/shiny/issues/1817): These were various bugs triggered by the presence of the [networkD3](https://christophergandrud.github.io/networkD3/) package's Sankey plot in an app. Impacted features included `dateRangeInput`, `withProgressBar`, and bookmarking ([#2359](https://github.com/rstudio/shiny/pull/2359))
+
 ### Documentation Updates
 
 * Fixed [#2247](https://github.com/rstudio/shiny/issues/2247): `renderCachedPlot` now supports using promises for either `expr` or `cacheKeyExpr`. (Shiny v1.2.0 supported async `expr`, but only if `cacheKeyExpr` was async as well; now you can use any combination of sync/async for `expr` and `cacheKeyExpr`.) [#2261](https://github.com/rstudio/shiny/pull/2261)

diff --git a/inst/www/shared/datepicker/js/bootstrap-datepicker.js b/inst/www/shared/datepicker/js/bootstrap-datepicker.js
@@ -671,7 +671,7 @@
 				visualPadding = 10,
 				container = $(this.o.container),
 				windowWidth = container.width(),
-				scrollTop = this.o.container === 'body' ? $(document).scrollTop() : container.scrollTop(),
+				scrollTop = this.o.container === 'body:first' ? $(document).scrollTop() : container.scrollTop(),
 				appendOffset = container.offset();
 
 			var parentsZindex = [];
@@ -686,7 +686,7 @@
 			var left = offset.left - appendOffset.left,
 				top = offset.top - appendOffset.top;
 
-			if (this.o.container !== 'body') {
+			if (this.o.container !== 'body:first') {
 				top += scrollTop;
 			}
 
@@ -1766,7 +1766,7 @@
 		enableOnReadonly: true,
 		showOnFocus: true,
 		zIndexOffset: 10,
-		container: 'body',
+		container: 'body:first',
 		immediateUpdates: false,
 		title: '',
 		templates: {

diff --git a/inst/www/shared/datepicker/js/bootstrap-datepicker.min.js b/inst/www/shared/datepicker/js/bootstrap-datepicker.min.js
diff --git a/srcjs/init_shiny.js b/srcjs/init_shiny.js
@@ -413,17 +413,17 @@ function initShiny() {
   // Need to register callbacks for each Bootstrap 3 class.
   var bs3classes = ['modal', 'dropdown', 'tab', 'tooltip', 'popover', 'collapse'];
   $.each(bs3classes, function(idx, classname) {
-    $('body').on('shown.bs.' + classname + '.sendImageSize', '*',
+    $(document.body).on('shown.bs.' + classname + '.sendImageSize', '*',
       filterEventsByNamespace('bs', sendImageSize));
-    $('body').on('shown.bs.' + classname + '.sendOutputHiddenState ' +
+    $(document.body).on('shown.bs.' + classname + '.sendOutputHiddenState ' +
                  'hidden.bs.' + classname + '.sendOutputHiddenState',
                  '*', filterEventsByNamespace('bs', sendOutputHiddenState));
   });
 
   // This is needed for Bootstrap 2 compatibility and for non-Bootstrap
   // related shown/hidden events (like conditionalPanel)
-  $('body').on('shown.sendImageSize', '*', sendImageSize);
-  $('body').on('shown.sendOutputHiddenState hidden.sendOutputHiddenState', '*',
+  $(document.body).on('shown.sendImageSize', '*', sendImageSize);
+  $(document.body).on('shown.sendOutputHiddenState hidden.sendOutputHiddenState', '*',
                sendOutputHiddenState);
 
   // Send initial pixel ratio, and update it if it changes

diff --git a/srcjs/input_binding_fileinput.js b/srcjs/input_binding_fileinput.js
@@ -13,7 +13,7 @@ var IE8FileUploader = function(shinyapp, id, fileEl) {
     this.iframe.id = iframeId;
     this.iframe.name = iframeId;
     this.iframe.setAttribute('style', 'position: fixed; top: 0; left: 0; width: 0; height: 0; border: none');
-    $('body').append(this.iframe);
+    $(document.body).append(this.iframe);
     var iframeDestroy = function() {
       // Forces Shiny to flushReact, flush outputs, etc. Without this we get
       // invalidated reactives, but observers don't actually execute.

diff --git a/srcjs/modal.js b/srcjs/modal.js
@@ -15,7 +15,7 @@ exports.modal = {
     let $modal = $('#shiny-modal-wrapper');
     if ($modal.length === 0) {
       $modal = $('<div id="shiny-modal-wrapper"></div>');
-      $('body').append($modal);
+      $(document.body).append($modal);
 
       // If the wrapper's content is a Bootstrap modal, then when the inner
       // modal is hidden, remove the entire thing, including wrapper.

diff --git a/srcjs/notifications.js b/srcjs/notifications.js
@@ -97,7 +97,7 @@ exports.notifications = (function() {
     if ($panel.length > 0)
       return $panel;
 
-    $('body').append('<div id="shiny-notification-panel">');
+    $(document.body).append('<div id="shiny-notification-panel">');
 
     return $panel;
   }

diff --git a/srcjs/shinyapp.js b/srcjs/shinyapp.js
@@ -1078,7 +1078,7 @@ var ShinyApp = function() {
         var $container = $('.shiny-progress-container');
         if ($container.length === 0) {
           $container = $('<div class="shiny-progress-container"></div>');
-          $('body').append($container);
+          $(document.body).append($container);
         }
 
         // Add div for just this progress ID

diff --git a/tools/README.md b/tools/README.md
@@ -101,15 +101,15 @@ To update the version of babel-polyfill:
 
 After updating, our patches to `bootstrap-datepicker` must be applied using the script `applyDatepickerPatches.R`
 
-After updating and applying patches, `yarn grunt` should be run per the instructions above.
+After updating and applying patches, `yarn grunt` should be run per the instructions above in order to generate a minified JavaScript file.
 
 ## Making a new patch
 
 To create a new patch:
 
 1. Make any necessary changes to files in `inst/www/shared/datepicker`
 1. **Do not commit your changes.**
-1. Instead, create a patch with a command like `git diff > tools/datepicker-patches/012-a-description.patch`
+1. Instead, create a patch with a command like `git diff > tools/datepicker-patches/012-a-description.patch`. Patches are applied in alphabetic order (per `list.files`), so you should name your patch based on the last one in `tools/datepicker-patches` so that it's applied last.
 1. Add the new `.patch` file to the repo with a descriptive commit message
 1. Revert `bootstrap-datepicker` to its unpatched state by running `updateBootstrapDatepicker.R`
 1. Apply all patches, including the one you just made, by running `applyDatepickerPatches.R`

diff --git a/tools/datepicker-patches/001-fix-networkD3-sankey-svg-bugs.patch b/tools/datepicker-patches/001-fix-networkD3-sankey-svg-bugs.patch
@@ -0,0 +1,31 @@
+diff --git a/inst/www/shared/datepicker/js/bootstrap-datepicker.js b/inst/www/shared/datepicker/js/bootstrap-datepicker.js
+index 97f5c086..2a0d8ae6 100644
+--- a/inst/www/shared/datepicker/js/bootstrap-datepicker.js
++++ b/inst/www/shared/datepicker/js/bootstrap-datepicker.js
+@@ -671,7 +671,7 @@
+ 				visualPadding = 10,
+ 				container = $(this.o.container),
+ 				windowWidth = container.width(),
+-				scrollTop = this.o.container === 'body' ? $(document).scrollTop() : container.scrollTop(),
++				scrollTop = this.o.container === 'body:first' ? $(document).scrollTop() : container.scrollTop(),
+ 				appendOffset = container.offset();
+
+ 			var parentsZindex = [];
+@@ -686,7 +686,7 @@
+ 			var left = offset.left - appendOffset.left,
+ 				top = offset.top - appendOffset.top;
+
+-			if (this.o.container !== 'body') {
++			if (this.o.container !== 'body:first') {
+ 				top += scrollTop;
+ 			}
+
+@@ -1766,7 +1766,7 @@
+ 		enableOnReadonly: true,
+ 		showOnFocus: true,
+ 		zIndexOffset: 10,
+-		container: 'body',
++		container: 'body:first',
+ 		immediateUpdates: false,
+ 		title: '',
+ 		templates: {
diff --git a/tools/updateBootstrapDatepicker.R b/tools/updateBootstrapDatepicker.R
@@ -13,12 +13,24 @@ dest_file <- file.path(tempdir(), paste0("bootstrap-datepicker-", version, ".zip
 url       <- sprintf("https://github.com/uxsolutions/bootstrap-datepicker/releases/download/%s/bootstrap-datepicker-%s-dist.zip", tag, version)
 
 download.file(url, dest_file)
-unzip(
-  dest_file,
-  files = c(
-    "js/bootstrap-datepicker.js",
-    "css/bootstrap-datepicker3.css",
-    "css/bootstrap-datepicker3.min.css"
-  ),
-  exdir = dest_dir
+unzipped <- tempdir()
+unzip(dest_file, exdir = unzipped)
+
+file.copy(
+  file.path(unzipped, "js", "bootstrap-datepicker.js"),
+  file.path(dest_dir, "js"),
+  overwrite = TRUE
+)
+
+file.copy(
+  dir(file.path(unzipped, "locales"), "\\.js$", full.names = TRUE),
+  file.path(dest_dir, "js", "locales"),
+  overwrite = TRUE
+)
+
+file.copy(
+  dir(file.path(unzipped, "css"), "^bootstrap-datepicker3(\\.min)?\\.css$",
+      full.names = TRUE),
+  file.path(dest_dir, "css"),
+  overwrite = TRUE
 )