Skip to content

Commit

Permalink
fix(insertTab): Render inserted nav html only once (#4179)
Browse files Browse the repository at this point in the history 
* fix: Fix checking if `scope` is a jquery element

Fixes rstudio/bslib#1159

* refactor: Don't check binding validity if `scope` isn't an element

* fix(insertTab): Render inserted nav html only once

* chore: Don't need to delay binding

* fix: Bind all after inserting nav controls

Output bindings require outputs to be attached to the DOM.

* chore: align comment

* chore: Add news item
  • Loading branch information
@gadenbuie
gadenbuie authored Jan 27, 2025
1 parent b8a5aef commit 55b37fd
Show file tree
Hide file tree
Showing 7 changed files with 83 additions and 95 deletions.
2 changes: 2 additions & 0 deletions NEWS.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

* The Shiny Client Console (enabled with `shiny::devmode()`) no longer displays duplicate warning or error message. (#4177)

* Updated the JavaScript used when inserting a tab to avoid rendering dynamic UI elements twice when adding the new tab via `insertTab()` or `bslib::nav_insert()`. (#4179)

# shiny 1.10.0

## New features and improvements
Expand Down
81 changes: 29 additions & 52 deletions inst/www/shared/shiny.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20708,6 +20708,9 @@
if (Array.isArray(arr))
return arr;
}
function isJQuery(value) {
return typeof value.jquery === "string";
}
function valueChangeCallback(inputs, binding, el, allowDeferred) {
var id = binding.getId(el);
if (id) {
Expand All @@ -20726,6 +20729,9 @@
var bindingsRegistry = function() {
var bindings = /* @__PURE__ */ new Map();
function checkValidity(scope) {
if (!isJQuery(scope) && !(scope instanceof HTMLElement)) {
return;
}
var duplicateIds = /* @__PURE__ */ new Map();
var problems = /* @__PURE__ */ new Set();
bindings.forEach(function(idTypes, id) {
Expand Down Expand Up @@ -20776,7 +20782,7 @@
headline: headline,
message: message
});
var scopeElement = scope instanceof HTMLElement ? scope : scope.get(0);
var scopeElement = isJQuery(scope) ? scope.get(0) : scope;
(scopeElement || window).dispatchEvent(event);
}
function addBinding(id, bindingType) {
Expand Down Expand Up @@ -24638,7 +24644,8 @@
}
addMessageHandler("shiny-insert-tab", /* @__PURE__ */ function() {
var _ref10 = _asyncToGenerator13(/* @__PURE__ */ _regeneratorRuntime13().mark(function _callee15(message) {
var $parentTabset, $tabset, $tabContent, tabsetId, $divTag, $liTag, $aTag, $targetLiTag, targetInfo, dropdown, index, tabId, _iterator3, _step3, el, getTabIndex, getDropdown;
var _$targetLiTag;
var $parentTabset, $tabset, $tabContent, tabsetId, $fragLi, $liTag, $aTag, $targetLiTag, targetInfo, dropdown, fixupDivId, index, tabId, getTabIndex, getDropdown;
return _regeneratorRuntime13().wrap(function _callee15$(_context15) {
while (1)
switch (_context15.prev = _context15.next) {
Expand Down Expand Up @@ -24683,8 +24690,11 @@
$tabset = $parentTabset;
$tabContent = getTabContent($tabset);
tabsetId = $parentTabset.attr("data-tabsetid");
$divTag = (0, import_jquery38.default)(message.divTag.html);
$liTag = (0, import_jquery38.default)(message.liTag.html);
$fragLi = (0, import_jquery38.default)("<div>");
_context15.next = 9;
return renderContentAsync($fragLi, message.liTag, "afterBegin");
case 9:
$liTag = (0, import_jquery38.default)($fragLi).find("> li");
$aTag = $liTag.find("> a");
$targetLiTag = null;
if (message.target !== null) {
Expand All @@ -24693,24 +24703,25 @@
}
dropdown = getDropdown();
if (!(dropdown !== null)) {
_context15.next = 18;
_context15.next = 20;
break;
}
if (!($aTag.attr("data-toggle") === "dropdown")) {
_context15.next = 15;
_context15.next = 17;
break;
}
throw "Cannot insert a navbarMenu inside another one";
case 15:
case 17:
$tabset = dropdown.$tabset;
tabsetId = dropdown.id;
$liTag.removeClass("nav-item").find(".nav-link").removeClass("nav-link").addClass("dropdown-item");
case 18:
case 20:
fixupDivId = "";
if ($aTag.attr("data-toggle") === "tab") {
index = getTabIndex($tabset, tabsetId);
tabId = "tab-" + tabsetId + "-" + index;
$liTag.find("> a").attr("href", "#" + tabId);
$divTag.attr("id", tabId);
fixupDivId = tabId;
}
if (message.position === "before") {
if ($targetLiTag) {
Expand All @@ -24725,57 +24736,23 @@
$tabset.append($liTag);
}
}
_context15.next = 22;
return renderContentAsync($liTag[0], {
html: $liTag.html(),
deps: message.liTag.deps
});
case 22:
_context15.next = 24;
return renderContentAsync(
$tabContent[0],
{
html: "",
deps: message.divTag.deps
},
"beforeend"
);
case 24:
_iterator3 = _createForOfIteratorHelper7($divTag.get());
_context15.prev = 25;
_iterator3.s();
_context15.next = 25;
return shinyBindAll(((_$targetLiTag = $targetLiTag) === null || _$targetLiTag === void 0 ? void 0 : _$targetLiTag.parent()) || $tabset);
case 25:
_context15.next = 27;
return renderContentAsync($tabContent[0], message.divTag, "beforeEnd");
case 27:
if ((_step3 = _iterator3.n()).done) {
_context15.next = 34;
break;
if (fixupDivId) {
$tabContent.find('[id="tab-tsid-id"]').attr("id", fixupDivId);
}
el = _step3.value;
$tabContent[0].appendChild(el);
_context15.next = 32;
return renderContentAsync(el, el.innerHTML || el.textContent);
case 32:
_context15.next = 27;
break;
case 34:
_context15.next = 39;
break;
case 36:
_context15.prev = 36;
_context15.t0 = _context15["catch"](25);
_iterator3.e(_context15.t0);
case 39:
_context15.prev = 39;
_iterator3.f();
return _context15.finish(39);
case 42:
if (message.select) {
$liTag.find("a").tab("show");
}
case 43:
case 29:
case "end":
return _context15.stop();
}
}, _callee15, null, [[25, 36, 39, 42]]);
}, _callee15);
}));
return function(_x17) {
return _ref10.apply(this, arguments);
Expand Down
4 changes: 2 additions & 2 deletions inst/www/shared/shiny.js.map
View file

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion inst/www/shared/shiny.min.js
View file

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions inst/www/shared/shiny.min.js.map
View file

Large diffs are not rendered by default.

15 changes: 14 additions & 1 deletion srcts/src/shiny/bind.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@ import { sendImageSizeFns } from "./sendImageSize";

type BindScope = HTMLElement | JQuery<HTMLElement>;

/**
* Type guard to check if a value is a jQuery object containing HTMLElements
* @param value The value to check
* @returns A type predicate indicating if the value is a jQuery<HTMLElement>
*/
function isJQuery<T = HTMLElement>(value: unknown): value is JQuery<T> {
return typeof (value as any).jquery === "string";
}

// todo make sure allowDeferred can NOT be supplied and still work
function valueChangeCallback(
inputs: InputValidateDecorator,
Expand Down Expand Up @@ -79,6 +88,10 @@ const bindingsRegistry = (() => {
* otherwise returns an ok status.
*/
function checkValidity(scope: BindScope): void {
if (!isJQuery(scope) && !(scope instanceof HTMLElement)) {
return;
}

type BindingCounts = { [T in BindingTypes]: number };
const duplicateIds = new Map<string, BindingCounts>();
const problems: Set<string> = new Set();
Expand Down Expand Up @@ -146,7 +159,7 @@ const bindingsRegistry = (() => {
}:\n${duplicateIdMsg}`;

const event = new ShinyClientMessageEvent({ headline, message });
const scopeElement = scope instanceof HTMLElement ? scope : scope.get(0);
const scopeElement = isJQuery(scope) ? scope.get(0) : scope;
(scopeElement || window).dispatchEvent(event);
}

Expand Down
68 changes: 32 additions & 36 deletions srcts/src/shiny/shinyapp.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
getShinyCreateWebsocket,
getShinyOnCustomMessage,
setShinyUser,
shinyBindAll,
shinyForgetLastInputValue,
shinyUnbindAll,
} from "./initedMethods";
Expand Down Expand Up @@ -1053,8 +1054,13 @@ class ShinyApp {
const $tabContent = getTabContent($tabset);
let tabsetId = $parentTabset.attr("data-tabsetid");

const $divTag = $(message.divTag.html);
const $liTag = $(message.liTag.html);
// Create a virtual element where we'll temporarily hold the rendered
// nav controls so we can rewrite some attributes and choose where to
// insert the new controls.
const $fragLi = $("<div>");
await renderContentAsync($fragLi, message.liTag, "afterBegin");

const $liTag = $($fragLi).find("> li");
const $aTag = $liTag.find("> a");

// Unless the item is being prepended/appended, the target tab
Expand Down Expand Up @@ -1097,13 +1103,16 @@ class ShinyApp {
// text items (which function as dividers and headers inside
// navbarMenus) and whole navbarMenus (since those get
// constructed from scratch on the R side and therefore
// there are no ids that need matching)
// there are no ids that need matching). In other words, we're
// guaranteed to be inserting only one `nav_panel()`.
let fixupDivId = "";
if ($aTag.attr("data-toggle") === "tab") {
const index = getTabIndex($tabset, tabsetId);
const tabId = "tab-" + tabsetId + "-" + index;

$liTag.find("> a").attr("href", "#" + tabId);
$divTag.attr("id", tabId);
// We'll fixup the div ID after we insert it
fixupDivId = tabId;
}

// actually insert the item into the right place
Expand All @@ -1120,11 +1129,8 @@ class ShinyApp {
$tabset.append($liTag);
}
}
await shinyBindAll($targetLiTag?.parent() || $tabset);

await renderContentAsync($liTag[0], {
html: $liTag.html(),
deps: message.liTag.deps,
});
// jcheng 2017-07-28: This next part might look a little insane versus the
// more obvious `$tabContent.append($divTag);`, but there's a method to the
// madness.
Expand Down Expand Up @@ -1152,40 +1158,30 @@ class ShinyApp {
// In theory the same problem exists for $liTag but since that content is
// much less likely to include arbitrary scripts, we're skipping it.
//
// This code could be nicer if we didn't use renderContent, but rather the
// lower-level functions that renderContent uses. Like if we pre-process
// the value of message.divTag.html for singletons, we could do that, then
// render dependencies, then do $tabContent.append($divTag).
await renderContentAsync(
$tabContent[0],
{ html: "", deps: message.divTag.deps },
// @ts-expect-error; TODO-barret; There is no usage of beforeend
"beforeend"
);
for (const el of $divTag.get()) {
// Must not use jQuery for appending el to the doc, we don't want any
// scripts to run (since they will run when renderContent takes a crack).
$tabContent[0].appendChild(el);
// If `el` itself is a script tag, this approach won't work (the script
// won't be run), since we're only sending innerHTML through renderContent
// and not the whole tag. That's fine in this case because we control the
// R code that generates this HTML, and we know that the element is not
// a script tag.
await renderContentAsync(el, el.innerHTML || el.textContent);
// garrick 2025-01-23: Keeping in mind the above, the `shiny-insert-tab`
// method was re-written to avoid adding the nav controls (liTag) and
// the nav panel contents (divTag) twice. Now, we use
// renderContentAsync() to add both sections to the DOM only once.

await renderContentAsync($tabContent[0], message.divTag, "beforeEnd");

if (fixupDivId) {
// We're inserting one nav_panel() and need to fixup the content ID
$tabContent.find('[id="tab-tsid-id"]').attr("id", fixupDivId);
}

if (message.select) {
$liTag.find("a").tab("show");
}
/* Barbara -- August 2017
Note: until now, the number of tabs in a tabsetPanel (or navbarPage
or navlistPanel) was always fixed. So, an easy way to give an id to
a tab was simply incrementing a counter. (Just like it was easy to
give a random 4-digit number to identify the tabsetPanel). Now that
we're introducing dynamic tabs, we must retrieve these numbers and
fix the dummy id given to the tab in the R side -- there, we always
set the tab id (counter dummy) to "id" and the tabset id to "tsid")
*/
* Note: until now, the number of tabs in a tabsetPanel (or navbarPage
* or navlistPanel) was always fixed. So, an easy way to give an id to
* a tab was simply incrementing a counter. (Just like it was easy to
* give a random 4-digit number to identify the tabsetPanel). Now that
* we're introducing dynamic tabs, we must retrieve these numbers and
* fix the dummy id given to the tab in the R side -- there, we always
* set the tab id (counter dummy) to "id" and the tabset id to "tsid")
*/
function getTabIndex(
$tabset: JQuery<HTMLElement>,
tabsetId: string | undefined
Expand Down

0 comments on commit 55b37fd

Please sign in to comment.