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.

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: Fix checking if scope is a jquery element #4176

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 24 additions & 14 deletions inst/www/shared/shiny.js
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 Object.prototype.hasOwnProperty.call(value, "jquery") && 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 @@ -24642,36 +24648,40 @@
_iterator3.s();
case 27:
if ((_step3 = _iterator3.n()).done) {
_context15.next = 34;
_context15.next = 35;
break;
}
el = _step3.value;
$tabContent[0].appendChild(el);
_context15.next = 32;
if (!(el.nodeType === Node.ELEMENT_NODE)) {
_context15.next = 33;
break;
}
_context15.next = 33;
return renderContentAsync(el, el.innerHTML || el.textContent);
case 32:
case 33:
_context15.next = 27;
break;
case 34:
_context15.next = 39;
case 35:
_context15.next = 40;
break;
case 36:
_context15.prev = 36;
case 37:
_context15.prev = 37;
_context15.t0 = _context15["catch"](25);
_iterator3.e(_context15.t0);
case 39:
_context15.prev = 39;
case 40:
_context15.prev = 40;
_iterator3.f();
return _context15.finish(39);
case 42:
return _context15.finish(40);
case 43:
if (message.select) {
$liTag.find("a").tab("show");
}
case 43:
case 44:
case "end":
return _context15.stop();
}
}, _callee15, null, [[25, 36, 39, 42]]);
}, _callee15, null, [[25, 37, 40, 43]]);
}));
return function(_x17) {
return _ref10.apply(this, arguments);
Expand Down
4 changes: 2 additions & 2 deletions inst/www/shared/shiny.js.map

Large diffs are not rendered by default.

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

Large diffs are not rendered by default.

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

Large diffs are not rendered by default.

18 changes: 17 additions & 1 deletion srcts/src/shiny/bind.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,18 @@ 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 (
Object.prototype.hasOwnProperty.call(value, "jquery") &&
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 +91,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 +162,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
20 changes: 14 additions & 6 deletions srcts/src/shiny/shinyapp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1053,6 +1053,12 @@ class ShinyApp {
const $tabContent = getTabContent($tabset);
let tabsetId = $parentTabset.attr("data-tabsetid");

// TODO: Only render tab content HTML once
// The following lines turn the content/nav control HTML into DOM nodes,
// but we don't insert these directly, instead we take the HTML from
// these nodes and pass it through `renderContentAsync()`. This means
// the inserted HTML may not perfectly match the message HTML, esp. if
// the content uses web components that alter their HTML when loaded.
const $divTag = $(message.divTag.html);
Comment on lines +1056 to 1062
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this shouldn't be a comment in the source, but the idea is to come back to this in a subsequent PR and see if we can do this with only one html -> DOM pass instead of two.

const $liTag = $(message.liTag.html);
const $aTag = $liTag.find("> a");
Expand Down Expand Up @@ -1166,12 +1172,14 @@ class ShinyApp {
// 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);
if (el.nodeType === Node.ELEMENT_NODE) {
// 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);
}
}

if (message.select) {
Expand Down
Loading