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

Closed
wants to merge 6 commits into from
+61 −27
Closed

fix: Fix checking if scope is a jquery element #4176

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
db123a1
fix: Fix checking if `scope` is a jquery element
gadenbuie Jan 14, 2025
20b2669
`yarn build` (GitHub Actions)
gadenbuie Jan 14, 2025
33d6686
refactor: Don't check binding validity if `scope` isn't an element
gadenbuie Jan 22, 2025
6c281f3
chore: yarn build
gadenbuie Jan 22, 2025
4a01dde
fix: Only call `renderContentAsync` on element nodes
gadenbuie Jan 22, 2025
42b6b8b
chore: yarn build
gadenbuie Jan 22, 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
8 changes: 7 additions & 1 deletion inst/www/shared/shiny.js
View file
Original file line number Diff line number Diff line change
@@ -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) {
@@ -20622,6 +20625,9 @@
var bindingsRegistry = function() {
var bindings = /* @__PURE__ */ new Map();
function checkValidity(scope) {
if (scope instanceof Text) {
scope = scope.parentElement || document.documentElement;
}
var duplicateIds = /* @__PURE__ */ new Map();
var problems = /* @__PURE__ */ new Set();
bindings.forEach(function(idTypes, id) {
@@ -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) {
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.

21 changes: 19 additions & 2 deletions srcts/src/shiny/bind.ts
View file
Original file line number Diff line number Diff line change
@@ -12,6 +12,19 @@ import { shinyAppBindOutput, shinyAppUnbindOutput } from "./initedMethods";
import { sendImageSizeFns } from "./sendImageSize";

type BindScope = HTMLElement | JQuery<HTMLElement>;
type BindAllScope = HTMLElement | JQuery<HTMLElement> | Text;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will require some downstream changes, but it feels like, if checkValidity() is gonna support a Text type, then so should .bindAll(), etc.

Suggested change
type BindScope = HTMLElement | JQuery<HTMLElement>;
type BindAllScope = HTMLElement | JQuery<HTMLElement> | Text;
type BindScope = HTMLElement | JQuery<HTMLElement> | Text;

Copy link
Member

Choose a reason for hiding this comment

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

It's super weird that text is being passed in at all. It bothers me not knowing where that's coming from.

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 el.innerHTML || el.textContent in this code seems like a likely source

shiny/srcts/src/shiny/shinyapp.ts

Lines 1165 to 1175 in 20b2669

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);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Just above it that section I found this comment

shiny/srcts/src/shiny/shinyapp.ts

Lines 1145 to 1147 in 20b2669

// C) $divTag may be of length > 1 (e.g. navbarMenu). I also noticed text
// elements consisting of just "\n" being included in the nodeset of
// $divTag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, I refactored this to avoid adding a new type, and we just exit validity checking if it's passed something other than an HTMLElement or a jQuery instance. 33d6686

A few take-aways:

  1. Validity checks happen with each new renderContentAsync() and which nav insertion calls many times in one nav_insert(). Thus feat: De-duplicate client console messages #4177 will be helpful.
  2. There are dragons in the nav insertion logic and I think it's okay to take the easy way out and treat the symptom in checkValidity().

Copy link
Collaborator

Choose a reason for hiding this comment

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

The el.innerHTML || el.textContent in this code seems like a likely source

I don't think that's right. It's the type of el (i.e., the first argument to renderContentAsync()) that's the problem. The type checker incorrectly thinks el is always HTMLElement, when apparently it can also be Text. This can be confirmed by adding console.log(el) just before the renderContentAsync(el, el.innerHTML || el.textContent) call using the reprex you provided. And in this case, the Text nodes are just new lines. Apparently jQuery does this for HTML strings that look like this: $("<div></div>\n<div></div>"). Given that, it's really unfortunate that $divTag's type is JQuery<HTMLElement> because it really should be something more like JQuery<HTMLElement | Text>?

All that said, ideally we'd get to the bottom of the jQuery typing issues, but if that seems like a lot of work/impossible, I think I'd be OK with just adding the if check around that renderContentAsync() call (along with a comment about how el's type isn't quite right). I can't think of a scenario where el is Text that you actually want to render?

Copy link
Member Author

@gadenbuie gadenbuie Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@jcheng5 and I looked through this and came to the same conclusion as you. So I added the check to only call that renderContentAsync() for element nodes in 4a01dde.

That said, I remembered while looking at this that about a year ago I ran into issues with a web component in an inserted nav panel being initialized twice and as a result having different behavior when finally added to the DOM because the component changes some attributes on load.

What we'd really like to do is to avoid doing both $(message.divTag.html) and then later calling renderContentAsync(). If you have any advice or context around why we currently do both or how we can get out of the situation, it'd be appreciated!


/**
* 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(
@@ -78,7 +91,11 @@ const bindingsRegistry = (() => {
* @returns ShinyClientMessageEvent if current ID bindings are invalid,
* otherwise returns an ok status.
*/
function checkValidity(scope: BindScope): void {
function checkValidity(scope: BindAllScope): void {
if (scope instanceof Text) {
scope = scope.parentElement || document.documentElement;
}

type BindingCounts = { [T in BindingTypes]: number };
const duplicateIds = new Map<string, BindingCounts>();
const problems: Set<string> = new Set();
@@ -146,7 +163,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);
}