Refactor (if feasible) use of jquery parseHTML function to use vanillaJS

I didn't have any luck with the suggested createHTMLDocument() but was at least able to get \Drupal\FunctionalJavascriptTests\Ajax\CommandsTest::testAjaxCommands passing with this approach. Let's see if anything else fails.

There are security considerations to take care of with replacing parseHTML: https://github.com/jquery/jquery/blob/main/src/core/parseHTML.js
a straightforward replacement is unlikely to be suitable.

We might need to extract jquery's logic in vanilla js somehow

@nod_ are those considerations applicable in this one case that $.parseHTML is called? Or is it bypassed because we're passing true for the keepScripts param?

I think @mstrelan is right here, we're using the third argument to parseHTML so we're not getting any filtering anyway

Refactor looks good and applying locally doesn't seem to break anything.

Would be great if we could get confirmation from @nod_ or @justafish on this. The $.parseHtml is using jQuerys own way of handling fragments which at least provides an API for filtering HTML. We could probably manage this by publishing a CR but it would be good to get confirmation that there aren't any other considerations.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I don't understand what the bot wants me to do.

"Patch does not apply"

"This does not mean that the patch needs to be re-rolled or the MR rebased."

If both are true then this should not be "Needs work".

Anyway, I've merged 11.x in to the branch and pushed it back up. Setting back to NR. Updated remaining tasks.

@mstrelan the bot comment is there to discourage credit farming from people from doing rerolls without addressing anything else that needs to be addressed and leave the issue stalled.

Spent some time looking at the replacement, still not sure it's safe. We do keep scripts although the scripts are not executed at parse time, they're executed when we add the elements to the DOM after the parsing. Here they would be executed during the parsing. I'm not convinced it's a problem, especially since we don't add JS with that codepath anymore (since the introduction of the add_js command). So we could try to set keepScripts to false and see what happens with the testbot first. That would be a good indication this is not a concern anymore. In any case the behavior change should be in a change notice somewhere.

If we don't have scripts to take care of, next is prefiltering html as lauriii pointed out. While is not a very scientific way of looking at this, I've never seen it used and Drupal is in charge of making things 'safe' before it gets to the JS so a developer will probably reach out to some PHP before trying to figure out this jquery API.

There is some code related to text nodes parents in jQuery, that may introduce a memory leak. It's over text nodes and Drupal is hardly a single page app when used with the native ajax framework so probably not a huge deal but would be good to get some data on whether this doesn't cause issues with very large text nodes.

same here as well raised a MR for this issue but the problem is that it is failing pipeline and when fixing one test case will result in failing another test case and viceversa..

can someone please try this here?

So we could try to set keepScripts to false and see what happens with the testbot first

I have used new MR 6957, because 1251 have huge rebase with conflicts (i decided to not waste the time - that's the only reason).
Basically in my commit i was trying to resolve #17 and provide more safe variant of replacement

Looks good to me, installed and tested it locally (no issues so far).
Please have @nod_ double-check as well.

Opened a MR to test #17 and #21, we need to make sure we don't need to care about scripts in there.

ok so it's just the specific test that fails, not other functionality in core

So yeah code from !1251 was enough given the way we call this, we don't need the extra options since we always call the function with the same parameters

Security considerations I talked about in #5 don't apply here since we always call the function with the document parameter, and I haven't seen the htmlPrefilter "hook" jquery provide used in core/contrib. This is not where security-related things should be happening.

As #14 mentioned we could use a change record to explain that htmlPrefilter is not available anymore for ajax content.

Acknowledged feedbacks, not sure why pipeline failing checks, as said in #29

Reran failing javascript test and fails every time

Also was tagged for CR so moving to NW for those.

It's about the asset size change since the code changed

@nod_ All test passed...pls review

Still appears to need a CR per #14 thanks.

Not sure why you keep adding empty comments Pravesh_Poonia.

Also not sure what the CR is for. None of the other jquery removal issues have CRs, and we're not adding or changing any APIs.

leaning on @mstrelan as per #38

Believe the CR was a leftover tag actually so will remove it.

Seems all threads and feedback have been addressed and gone through a few rounds of review. Going to mark it.

Feedback from #30 was not addressed, comments in MR

Believe feedback has been addressed here.

Committed and pushed b4a0261008 to 11.x and cd700017d1 to 11.0.x and 2d482e076f to 10.4.x. Thanks!

Just leaving a note here in case someone else ran into my problem after updating to 10.4+, this breaks functionality in which an ajax response might not be a valid child of a div whereas with jquery it was working. For example if you were returning a table row into an already existing table a <tr> is not a valid child of a div so it will be stripped from the response.

I don't necessarily think this warrants a bug ticket as it's probably the right way to go in the future, but it caused me some headache so felt I should leave a note here.