fix: Fix checking if scope is a jquery element

[gadenbuie]

Fixes rstudio/bslib#1159

Apparently there are cases where .bindAll() can be called on a text node rather than an HTMLElement or a jquery-wrapped element. The code path where this was observed was through the shiny-insert-tab message handler (see linked issue for a reprex).

I couldn't track down how that behavior arises. But it only becomes relevant for us when we want to dispatch the error console event since you can't dispatch an event on a text node.

This PR improves the check that differentiates between jQuery objects and DOM nodes and ensures that checkValidity() doesn't try to dispatch an event or call .get() on a text node.

[cpsievert]

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;

[jcheng5]

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

[gadenbuie]

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

[gadenbuie]

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.

[gadenbuie]

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().

[cpsievert]

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?

[gadenbuie]

@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!

[gadenbuie]

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.

[gadenbuie]

Closing in favor of #4179, which includes core changes from this PR and solves the underlying problem of rendering the nav HTML twice.