Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(insertTab): Render inserted nav html only once #4179

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+73 −87
Open

fix(insertTab): Render inserted nav html only once #4179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
20908ec
fix: Fix checking if `scope` is a jquery element
gadenbuie Jan 14, 2025
7aa1ec5
refactor: Don't check binding validity if `scope` isn't an element
gadenbuie Jan 22, 2025
b9f3094
fix(insertTab): Render inserted nav html only once
gadenbuie Jan 23, 2025
4040039
chore: Don't need to delay binding
gadenbuie Jan 23, 2025
86d9cea
`yarn build` (GitHub Actions)
gadenbuie Jan 23, 2025
af6ecdb
fix: Bind all after inserting nav controls
gadenbuie Jan 24, 2025
6722594
`yarn build` (GitHub Actions)
gadenbuie Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 29 additions & 52 deletions inst/www/shared/shiny.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20604,6 +20604,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 @@ -20622,6 +20625,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 @@ -20672,7 +20678,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 @@ -24534,7 +24540,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 @@ -24579,8 +24586,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 @@ -24589,24 +24599,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 @@ -24621,57 +24632,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 = _createForOfIteratorHelper6($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
Open in desktop

Large diffs are not rendered by default.

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

Large diffs are not rendered by default.

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

Large diffs are not rendered by default.

15 changes: 14 additions & 1 deletion srcts/src/shiny/bind.ts
View file
Open in desktop
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
52 changes: 24 additions & 28 deletions srcts/src/shiny/shinyapp.ts
View file
Open in desktop
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");
Copy link
Member

Choose a reason for hiding this comment

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

Note B in the mega-comment below says "renderContent must be called on an element that's attached to the document". Is that going to be a problem here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It didn't seem to be in testing but I'll take another look. The mega comment is quite old; while the insert tab message handler hasn't changed much since it was written, renderContent() certainly has seen a few changes.

The comment was added here 91dbb0e in #1794

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it seems like renderContentAsync() is essentially the same but renderHtml() has changed. (Leaving crumbs for myself)
https://github.com/rstudio/shiny/blob/91dbb0e77bfe137335e628ba314c6c9c399e2b71/srcjs/output_binding_html.js

Copy link
Member Author

Choose a reason for hiding this comment

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

I found a very helpful comment from you @jcheng5 in that PR thread that I'll move up to a top-level comment here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The mega comment also states

// D) renderContent has a bug where only position "replace" (the default)
//    uses the jQuery functions, so other positions like "beforeend" will
//    prevent child script tags from running.

but one core difference is that renderHtml() does now use jQuery for all insertion positions. The change was made in #3630.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I found it! Output elements need to be attached to the DOM, otherwise they aren't bound

shiny/srcts/src/shiny/bind.ts

Lines 317 to 320 in d764ea9

// In some uncommon cases, elements that are later in the
// matches array can be removed from the document by earlier
// iterations. See https://github.com/rstudio/shiny/issues/1399
if (!$.contains(document.documentElement, el)) continue;

So we just need to call bindAll() again after inserting the nav controls, in case they contain outputs. af6ecdb

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's a fun twist: I don't think this is needed anymore. The issue from #1399 doesn't reproduce if I take out the above line (added in #1402). (This change smells like an indirect fix that was reasonable at the time but might not be needed anymore.)

Copy link
Member

Choose a reason for hiding this comment

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

After further reflection, I'm fine with this the way you've written it. I was going to suggest doing all the tag manipulation with a parsed-but-not-rendered-or-inserted $liTag, then calling renderContentAsync without doing the appending the $liTag as a separate step. And if it were me I'd still maybe try to do it, but it's a pretty marginal improvement in code clarity and no difference in behavior AFAICT. So I'll leave it up to you.


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,26 +1158,16 @@ 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) {
Expand Down