ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs

+1 We don't need to solve bugs of browsers nobody uses any more.

+1 - marked #818670: Can we remove the wrapping div added by ajax 'insert' commands ? as duplicate

Well I agree. And google recently announced dropping the support for chrome 3.0 so chrome is definitely safe. And I couldn't find any recent browser statistics about safari 3.0, so that version must be dead by now :)

After some testing, var new_content = $(response.data); does not work when response.data is some HTML with a text node at the toplevel :

- response.data = '<span>foo</span>bar'
--> only <span>foo</span> gets inserted

- response.data = '<span>foo</span>bar<span>baz</span>'
--> JS execution stops during new_content.hide(); a couple lines below.

It does seem like a wrapping tag is needed. In this case, we should make sure it is removed when we're done inserting and applying effects.

Attached patch does that.

recategorize

At a glance, and without having tested it at all, it looks like this will cause problems when Drupal.attachBehaviors is called. As its first parameter it's expecting a jQuery wrapped set to use as the context, but it could end up getting something like <span>foo</span>bar<span>baz</span>. If I'm right, then you'll need to defer removing the temporary wrapper until after Drupal.attachBehaviors is called. I'll try and test if I get a chance later today...

is there any reason we can't retain the wrapper and make it's display property match the parent's?

I was under the assumption that top-level text nodes were fixed/supported in jQuery 1.4 now, but it seems I got that wrong.

If I'm not mistaken, then we have another issue regarding #wrapper in the queue that might be closely related.

@seutje : there are contexts where the mere presence of the div messes your layout :

<table>
  <tr>
    <td>...</td>
    <div><td>...</td></div> <!-- FAIL -->
    <td>...</td>
  </tr>
  <div> <!-- FAIL -->
    <tr>
      ..
    </tr> 
  </div>
</table>

It can also cause CSS rules to break.

This div just means you don't control the HTML you're outputting, which is really embarrassing.

@katbailey #6:

If I'm right, then you'll need to defer removing the temporary wrapper until after Drupal.attachBehaviors is called.

Hm. Animations being asynchronous, removing the div after both animations and behaviours attachment are done might be tricky.
Is the current order (animate, attach behaviors) a requirement, or could we attach behaviors before showing the new content ?

I think the whole Drupal.ajax.prototype.commands.insert() function is weird. Can't we split it up into the following?

insert() // add response.data into wrapper using .html()
empty() // remove children of wrapper
replace() // replace wrapper using .replaceWith()
remove() // already exists

the method .replaceAll() seems useless in this context; it allows to replace/move the wrapper to response.data (expected to be a selector string)

I'm using the ajax link example from ajax_example module to test this, and the issue of context being passed to Drupal.attachBehaviors is proving difficult to test due to this weirdness: #825318: Drupal.attachBehaviors() is called an extra time, unnecessarily and for document context, when there is no form.
@yched, are you testing this with ajax forms?

@katbailey: Yes, testing with a form. I have to say I didn't really look closely at the behaviors nor noticed they got fired twice.
I'm not sure it's really related to the current issue, but it does make things a little confusing as to how to properly fix the extra div issue :-/.

From #4:

After some testing, var new_content = $(response.data); does not work when response.data is some HTML with a text node at the toplevel :

That's a shame, but if it's true, then I don't think a temporary wrapper that gets removed is the right answer, because of the issue in #6. What about adding an AJAX command setting (or another property attached to the AJAX command object being returned) for letting people turn off the wrapper, so people can do so when they know they're returning a well-formed fragment (like something already in a 'td')?

What about this?

Minor regression is that if there are no ".ajax-new-content" elements and an effect is being used, text nodes at top-level will be show immediately (without the effect).

@eff #14

I don't think a temporary wrapper that gets removed is the right answer, because of the issue in #6 [behaviors]

What about the following workflow ?
- attach behaviours (done as a last step currently)
- fire animations
- remove the wrapping div in a callback at the end of the animations

Animations are asynchronous anyway, so even in current HEAD, behaviours get attached on dom elements while they're being animated. I don't think it would make a difference for behaviors if they were attached *before* the animations ?

Behaviors might check for visible elements only...

+++ misc/ajax.js	12 Jun 2010 10:58:53 -0000
@@ -321,13 +321,22 @@
+    new_content = new_content.contents();

I'm not enough of a JS / jQuery expert to know if this gets us into the trouble that katbailey warns about in #6 when new_content is then used as the context in the attachBehaviors() that follows. How can we find out?

Re #16:
A common use-case for attaching behaviors is wysiwyg editors, which use IFRAMEs. DOM manipulation (like removing a container DIV) of elements containing IFRAMEs causes the IFRAME to reload which messes up the editor, so we should absolutely not do any DOM manipulation after behaviors have been attached.

65 critical left. Go review some!

#19 doesn't answer effulgentsia's remark in #18 : won't behaviors end up with context being a mix of tags and top level text nodes, as pointed in #6 ? - although this hasn't yet been proved to be an actual issue

A couple thoughts :
- would it be valid to pass the 'target' as context for behaviors in all cases ?
(currently, it's the target if method is 'replaceWith' / 'replaceAll', and the content of the target if method is 'html')
- could a solution lie around the idea of adding a wrapper div only if response.data doesn't contain a wrapping tag itself (i.e <div>a</div><div>b</div> or <div>a</div>b<div>c</div>) ?

@yched:

could a solution lie around the idea of adding a wrapper div only if response.data doesn't contain a wrapping tag itself

great idea!

@casey: great implementation!

So how about making this issue just that, as in this patch, and putting the effects code cleanup of #21 into a separate issue for further discussion?

- I'm wondering about the case where content.data has several top-level tags (i.e <div>a</div><div>b</div>). For those, #21 and #22 remove the wrapper as well, and animations and behaviors are attached to a jq object with n elements, instead of currently on the elements as a whole. Shouldn't we also keep the div in those cases ?

- minor : is there a way we could create new_content without the div and add it if needed, rather than adding it and removing it just after that ?
+ when there are no text nodes, children() might be more explicit than content() ?
from http://api.jquery.com/contents :
"The .contents() and .children() methods are similar, except that the former includes text nodes as well as HTML elements in the resulting jQuery object."

With #23.

Actually, I'm not sure if #24 works right (see #4).

Tested it and #24 does not work. This goes back to #22 but adds the consideration from #23 that our effects code currently assumes a single element (at least when there isn't an ajax-new-content class in the content).

is there a way we could create new_content without the div and add it if needed, rather than adding it and removing it just after that ?

Not that I can think of. I added a comment in the patch that explains why not. But according to http://api.jquery.com/jQuery/#creating-new-elements, jQuery itself often implements $(HTML_STRING) as creating a temporary DIV and removing it.

when there are no text nodes, children() might be more explicit than content() ?

Well, the if statement must use .contents() since it doesn't know yet if there are text nodes. So while inside the if could be done with .children(), it seems to me that sticking with .contents() makes more sense.

I think the code comments in #26 makes this clear, but just wanting to address this question from #23 head on:

and animations and behaviors are attached to a jq object with n elements, instead of currently on the elements as a whole. Shouldn't we also keep the div in those cases ?

This should not be a problem for behaviors (it's possible there are broken behaviors in contrib that rely on the context to be a single element, but that's a broken behavior). But it is a problem for the effects code as it is currently written. We could try to make the effects code more robust, as per #21, but I continue to think that should be a separate issue. #26 takes into account the currently delicate effects code while still solving the primary use-case this issue started with.

Agreed on the remarks in #26 / #27. Patch looks good to me.
Casey, what do you think ?

Yes looks good to me too; nice and clear comments. One thing though; I'd add contents() to a var:

    var new_content_contents = new_content.contents();
    if (new_content_contents.length == 1 && new_content_contents.get(0).nodeType == 1) {
      new_content = new_content_contents;
    }

Yay.

Still applies.

this patch helps a lot when working with the ajax system (e.g. for appending another row to a table) and still applies.

Is this meant to be a permanent solution? It looks like it is a bit of a hack. Maybe we should add a @todo for Drupal 8?

AFAICT, this is a permanent solution, as long as AJAX accepts any kind of returned value, string, or fully-fledged HTML structure, and as long as the corresponding server-side calling code does not preemptively specify the nature/structure of the delivered content upfront. It is hard to tell whether this could be improved in any way.

http://drupal.org/node/919994 is a dup of this. FYI, this functionality is critical to the CTools D7 port.

#30: ajax-commandinsert-736066-30.patch queued for re-testing.

Committed to CVS HEAD. Thanks.

A note to googlers or those who followed the link from misc/ajax.js:501 and that have some trouble inserting several table rows in a HTML table, like this:

$html = '
<tr>
  <td>foo</td>
</tr>
<tr>
  <td>foo</td>
</tr>';

$commands[] = ajax_command_after('table#products-table tbody', $html);  

You will need only one top-level dom element in $html otherwise the rows will be wrapped into a <div> element.

Having several <tbody> is OK, so you can do something like this:

$html = '
<tbody>
  <tr>
    <td>foo</td>
  </tr>
  <tr>
    <td>foo</td>
  </tr>
</tbody>';

$commands[] = ajax_command_after('table#products-table tbody:last', $html);  

Well, aren't we care about inserting DIVs into HEAD?

When a stylesheet is returned with the 'insert' command with data like:

"<style type="text/css" media="all">@import url("http://.../sites/all/themes/mytheme/layouts/mylayout/mylayout.css?m5cq3m");</style>
"

this results to wrapping inside the 'div' and placing it into the 'head' element because of this code:

if (new_content.length != 1 || new_content.get(0).nodeType != 1) {
      new_content = new_content_wrapped;
    }

The reason is:

1) In the above example, the style element for some reason is suffixed with a newline, which creates two elements in the new_content variable: style element and text node, so making the if-expression to trigger on .length check (which = 2)
2) If we were get more then 1 stylesheet from a server, then amount of elements are simply more then 1, working out the same condition.

I don't know how bad it is to have divs in head, but I personally dislike it.
I propose not to use DIVs at all, when the wrapper is set to 'head':

if ((new_content.length != 1 || new_content.get(0).nodeType != 1) && wrapper.get(0).nodeName !== 'HEAD') {
    new_content = new_content_wrapped;
  } 

I think the general problem with this code is that it possibly produces invalid HTML.

if ((new_content.length != 1 || new_content.get(0).nodeType != 1)) {
    new_content = new_content_wrapped;
} 

E.g. if your PHP code produces a bunch of <li>-elements to be inserted into an existing <ul>, the <li>'s will get wrapped inside a <div> which is not allowed at top-level in <ol>/<ul>, following the W3-definition

Ideally it should be possible to pass a flag to indicate whether you want your content wrapped or not. That flag could be added to the the response object in PHP and checked for in ajax.js:

PHP:

$append = ajax_command_append('.selector', $html);
$append['no_wrap'] = TRUE;
$commands[] = $append;

ajax_deliver(array('#type' => 'ajax', '#commands' => $commands));

And in ajax.js, line 489:

var new_content_wrapped;
if(response.no_wrap) {
  new_content_wrapped= $(response.data);
} else {
  new_content_wrapped = $('<div></div>').html(response.data);
}
var new_content = new_content_wrapped.contents();

Note: This might be too much of a hacky solution and it should rather be seen as inspiration...

Here's a jQuery work around to remove the invalid divs from a given #selector without affecting the contents.

http://stackoverflow.com/questions/6428254/removing-only-the-parent-elem...

$(document).ajaxComplete(function() {
  var div = $("#selector div");
  var tmp = div.children().clone();
  var parent = div.parent();
  div.remove();
  tmp.appendTo(parent);
}

A side effect of the committed patch of #30 is that the inserted wrapping DIV's can get nested endlessly.

In our case we use the Form API's managed file field in a custom form. When uploading or removing a file, file_ajax_upload() returns multiple HTML elements on each AJAX call, these elements get wrapped and the nesting begins.

EDIT1: I think the File module probably should not append extra elements outside the AJAX wrapper, see #2302011: Managed file elements get wrapped + nested on each AJAX file upload.

EDIT2: Why not add ie. the .ajax-new-content-wrapper class to the new_content_wrapped element so it is easier to identify?

The code added by #30 still exists in 8.x. If there is an issue with it then it will have to be fixed there first.

no patch for 8.0.x

Raising to Major, because multiple issues are running into different flavors of this same underlying bug, and because per #43, producing invalid HTML is bad.

Adding the "JavaScript" tag for obvious reasons :)

Also updated the summary.

Many false positives on this check:

    if (new_content.length != 1 || new_content.get(0).nodeType != 1) {
      new_content = new_content_wrapped;
    }

can be circumvented by simply str.trim()'ing the response.data to ensure that jQuery does not do weird things in .html() resulting in the creation of empty text nodes (I think this occurs when there is a "\n" at the end or beginning of the response data).

This solved it for me:

    var new_content_wrapped = $('<div></div>').html(response.data.trim());
    var new_content = new_content_wrapped.contents();

it doesn't really fix the underlying problem though...

Not sure that trim() is a nice way to clean-up response that could be big enough.
I still think the batter to make that at the server side

The fact that I can't inject multiple javascript files via ajax into a page makes me sad :-(
In commet #42 this side-effect is explained already, here my use case for JavaScript files:

In a ajax callback I add multiple javascript files via drupal_add_js(), which results in a ajax-insert-command with multplie <script> tags. Those are then added to <head>, wrapped in a <div>, and that for not interpreted by my browser.

I'll be happy if any solution makes it into 7.x upstream.

I've to take back my critisism a little: even tough my scripts are wrapped by a <div> they get interpreted by my browser. Hence not so sad anymore :-|

Just wanted to "push" this issue as nothing has been done to fix this yet. It is highly annoying for me to work with D8 Ajax while theming. As there will be an RC out there soon I'd request at least a workaround e.g. have a constructor parameter for new AjaxCommand(..., $wrapper = FALSE) where $wrapper would decided about doing wild div stuff.

I don't see how it affects Usability (which is the tag we use, not UX). Fixing the DX tag.

In my terms UX => User experience because

So an empty div is always being appended now since all render function seem to be returning an \n.

I haven't tracked this down fully but for now he's a hacked up patch.

I'm undecided if the new line should be fixed here or in the theme. This JS is too sensitive right now which is why I'm doing it here. It might also be fair to just removed the need to check if there's multiple elements on the root of the doc. I'll look into this further.

Posting patch for now to move forward on my issue.

@ericduran thanx for bumping and idea about trimming with right way!
Please elaborate on

the need to check if there's multiple elements on the root of the doc

That's now obvious that trimming should be done after render, maybe that's cheaper to do at the backend, because some requests could be big.

Thank you very much!

Also it doesn't work when twig.config.debug is enabled.

This is most likely also causing problems for BigPipe, because BigPipe just reuses the AJAX system: #2654386: [PP-1] BigPipe uses AJAX system to insert new content, AJAX system adds a wrapping <div>, breaks theme.

Now effectively confirmed. Which means this potentially blocks BigPipe from being added to Drupal 8.1, see #2469431-204: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts.

What about this solution?

It allows the code to behave the same if nothing changes. But on top of that allows the developer to set another wrapper or force not using a wrapper at all.

This should fix the tests

Patch #58 works fine, but it needs backport to D7

I talked with @nod_ on IRC and took a look on jQuery to see what changed since the launch of D7 when was the last time I checked this issue.

Looking at http://api.jquery.com/Types/#htmlString we have

When a string is passed as an argument to a manipulation method such as .append(), it is always considered to be HTML since jQuery's other common interpretation of a string (CSS selectors) does not apply in those contexts.

For explicit parsing of a string to HTML, the $.parseHTML() method is available as of jQuery 1.8.

// Syntax error, unrecognized expression: bye<b>hello</b>
$( "bye<b>hello</b>" ).appendTo( "body" );
 
// Appends bye<b>hello</b>:
$( $.parseHTML( "bye<b>hello</b>" ) ).appendTo( "body" );

Which means we could remove all the code with new_content and new_content_wrapped and do

      // Add the new content to the page.
      new_content = response.data;
      wrapper[method](response.data);

But the issue then is on the following code, not the actual insert. new_content.find('.ajax-new-content') will throw an error on text nodes.

If we do var new_content = $($.parseHTML(response.data)); then Drupal.attachBehaviors(new_content.get(0), settings); will run only for the first node and throw an error if the first node is a text node. We can fix that with:

  new_content.each(function() {
    if (this.nodeName != '#text') {
      Drupal.attachBehaviors(this, settings);
    }
  });

I imagine this can raise some performance concerns. But if you want to insert multiple table rows or list items now you need to send multiple AJAX Commands which would be even slower.

To fix this we have decide what path we take.

My opinion is that we should not wrap in a div. The developer should have full control of what the result of the command is. Of course with freedom should come responsibility too.

Also note that using $.parseHTML() will not be possible in D7.

something like this:
bye<b>hello</b>
the developer can wrap in a <div> anyway. But it sounds like this:

<td>new row</td>
<td>2nd new row</td>

shouldn't be wrapped in a div (because that would be invalid HTML) Drupal AJAX insert doesn't currently allow it to be not wrapped, but should. However if the <td>'s are not wrapped in a div, I imagine that would that mean that the jQuery effect would happen to each <td> instead of the whole block of newly inserted <td>'s? Although according to #41, you can get around this by wrapping the <td>'s in a <tbody>.

Something like bye<b>hello</b> could be inserted in a span. But that can be wrapped by the developer too.

You can wrap tr in a tbody, not td. And as I said, now the workaround for inserting 2 td's like in your example is to call InsertCommand twice which means everything is run for each td anyway, including the attachment of behaviors.

Can't it ?

EDIT: Ouch, skip this patch

correct patch:

I investigated the new line issue and after I found why it happens I searched d.o and saw that the issues is actually related to this one. #1237012: An empty status message in the AJAX response is added to the commands[] array and added to the DOM as an empty div tag

Regarding #77, that solution can work for D8 too. The "drawback" is that it can expose any badly written behavior implementation.

HTML output in core

HTML output with patch in #77

Patch #68 tested.

Before ajax:

After ajax:

But i have a question: we really need wrap content in div?

A patch using the parent approach. Like I said previously this may make the context become "body" so run for everything on the page exposing behaviors that didn't use .once().

I upload it for testing and review. The comment would need changes for a commit.

I haven't manually tested #81 yet, but #73 says:

new_content.find('.ajax-new-content') will throw an error on text nodes.

Is that true? If so, wouldn't #81 trigger that, since that's exactly the code that's running between the removed hunk and added hunk?

In #81 for multiple elements or when the first is a text it sends the parent. Which means instead of creating the div parent we send whatever the parent of the new content is. In the worst case it's "body". Which means if we send 2 table rows the parent will be tbody instead of wrapping them in a div. In 73# I was talking about sending the new content as is.

But prior to the patch's new_content = new_content.parent(); line, there's existing code in ajax.js that does:

// Determine which effect to use and what content will receive the
// effect, then show the new content.
if (new_content.find('.ajax-new-content').length > 0) {
  new_content.find('.ajax-new-content').hide();
  new_content.show();
  new_content.find('.ajax-new-content')[effect.showEffect](effect.showSpeed);
}
else if (effect.showEffect !== 'show') {
  new_content[effect.showEffect](effect.showSpeed);
}

So those find()s happen on the sent content, not the parent. Also, if we enter the else block, then we're running an effect on potentially multiple nodes and/or text nodes, and is that ok?

The issue about new_content.find() was when I was doing

  // Add the new content to the page.
  new_content = response.data;
  wrapper[method](response.data);

If I was doing new_content = $($.parseHTML(response.data)) that was not a problem. While I don't parse the HTML in this patch, I also don't remove this part which does the parsing now:

  var new_content_wrapped = $('<div></div>').html(response.data);
  var new_content = new_content_wrapped.contents();

The patch works, making body the context sometimes is fine. Now when using the file upload field before and after we still have leftovers <span class="ajax-new-content"> but not they're not nesting infinitely in divs, they just add up at the same level.

Why do we tiptoe around edge cases to begin with? Do we have an exemple of a module that needs whitespaces and textnodes+domelements on the root of the response, and that can't be replaced with something better? I don't see why it's not ok to just say "don't do it anymore" and make a 'text' ajax command that would insert text if needed.

Blast from the past where people had to comment to subscribe :þ Turns out I agreed with tic2000 already #1031600-10: AJAX requests cause duplicate, invalid CSS includes and multi-nested response data output, so I'm obviously ok with this solution.

I just got affected by this issue. It took me a long time to understand what was wrong.
Thanks all for your work. I did the manual testing.

@nod_ I think it's too late to break BC, even on frontend, so let's commit this fix then open another issue to discuss about changing how everything works (for D9, maybe).

#87/#89 if there are no real examples relying on that behaviour, I think it's OK to change it in a minor release. i.e. the concrete bug in #87 is worth fixing compared to the theoretical regression that might be introduced for code we don't know about.

What is described in #87 can not be solved by this patch in any way. What is described in #87 is most we can do. There the issue is that we have DOM A, we add to it DOM B, and replace DOM A with A + B. Repeat over and over, so we end up with multiple B.

But I took a look at #2588013: <span class="ajax-new-content" style="display:inline-block;"> causes unwanted whitespace and provided a fix there too. There was another solution already provided, so there is even a choice in what approach we take there.

DuaelFR was that an RTBC for #68 or #81?

Sorry. #81 patch is good to me, according to my tests and the reading of #87

OK in that case as #81 says the comment needs updating for the new approach.

Sorry! I reviewed it again. I think #85 is a problem for current sites. Anime is changed. Do it in 8.1 is more suitable.
#68 has similar problem but developer can make decision for unexpected anime during development.

What if adding a `unwrap()` as callback ?

** I leaving the debug message there. So if you go to `admin/structure/types/manage/article/form-display` and click config. You can see better ajax result.

- Can we do refactor and break BC for Drupal.AjaxCommands.insert API in D8.1?
- Any popular modules output wired code now?

Added unwrap if we have node type 1, if we have other node types we have wrapper, and if we have nothing we just return nothing , this might be a good approach to this issue.

Patch in #81 needed a reroll. (there was merge conflicts I hope I did it right).

Previous reroll had errors ($ missing).

Previous patch was throwing some errors.
Uploading the new patch.

this approach (#103) raised by me and rejected by myself. Sorry :(.

Now we have few approaches
1. @droplet #81 (#103)
Problem: Anime effect is changed. After patching, it's apply anime to each div without single parent root

2. @tic2000 #68
Problem: Similiar to #1, But developers know it's problem on coding

3. @droplet #95
Problem: ( no known issues until this momont )

Posting reroll patch for ericduran's 736066-56 patch. Disregard this.

Because BigPipe uses the AJAX system, this also affects BigPipe: #2738685: BigPipe leaves wrapper <div>s in place, because AJAX system leaves them — this can cause some CSS to break.

Testing the previous patch and @VinayLondhe you didn't follow the file name standards.

This also affect views with AJAX exposed filters. Because of the extra div, the views contextual links (for quick edition) are broken after your "Apply" your filters once and does not show anymore.

Sorry, after test of #106: it appears that the div is no more included, although the "contextual links" does not appear anyway.
So this is RTBC, and the contextual links will go to another issue.

I suggest we start with a functional JS test first to get this issue back on track.

Then we can much more easily see which solutions fix the problem and which don't.

Re-roll for ajax_js_insert_command-736066-101.patch.

+ if ($new_content.length !== 1 || $new_content.get(0).nodeType !== 1) {

This logic doesn't work when theme debugging is enabled, or in other use cases where comments and whitespace are returned alongside a single Node Element. The debug comments will make $new_content.length greater than 1, and $new_content.get(0).nodeType is either Node.TEXT_NODE (newline) or Node.COMMENT_NODE. This makes development difficult as I have to turn off theme debugging while working on any AJAX logic.

I think what we actually want to check for is that there is only one Node.ELEMENT_NODE type element in $new_content, although this logic would require a loop that makes sure that the other siblings are not significant to the user (text nodes with whitespace and comments are probably OK, Node.DOCUMENT* and Node.PROCESSING_INSTRUCTION_NODE are not).

I leave a patch with a different approach. It still continues adding the extra DIV, but after the first request it recognizes the previous created DIV and replace it avoiding infinite nested layers.

I forget why we need a DIV. Why couldn't we just add a class to all the new elements being added? Is it because of something like this:
bye<b>hello</b>

// For legacy reasons, the effects processing code assumes that new_content
// consists of a single top-level element. Also, it has not been

In other words, what legacy reason?

This blocks #2738685: BigPipe leaves wrapper <div>s in place, because AJAX system leaves them — this can cause some CSS to break. Tagging.

Ok. so here is patch that just adds tests. Write now the tests pass without any changes to ajax.js but that is because I am not sure what desired behavior is.

I didn't write the tests using the dialog system but used a direct call to Drupal.ajax().

The 3 tests cases I thought are response of:

1. <div>text</div>
2. not wrapped text
3. outside-text<div>text</div>

What other test cases should be test for?

Also what should be desired behavior of each?

@tedbow Based on my comment in #118, there are (at least) two cases that the committed fix did not cover:

1. <!-- Theme debug comment --><div>Test</div>
2.     <div>leading whitespace (including newlines)</div>

In both cases, with the current logic, a wrapping <div> is added, even though the HTML string only contains one top-level element that is already wrapped. This is explained a bit more in #118.

@tedbow Thanks for the tests. This, I think, will address the test cases and wrap things correctly.

@samuel.mortenson thanks for the clarification.
Do you think the test I added is good way to test the functionality generally or should be just use use-ajax dialog links?

I can add the additional test cases you mentioned.

Some review:

1. +++ b/core/misc/ajax.js
   @@ -1041,9 +1039,14 @@
   +      var $responseDataWrapped = $('<div></div>').html(response.data);
   +      var $new_content = $responseDataWrapped.contents()
   +        .filter(function (index, value) {
   +          return value.nodeType === 1;
   +        }).length === 1 && $responseDataWrapped.contents().length === 1 ?
   +          $responseDataWrapped.children() :
   

   if "response.data" included a text node and a dom element, like "test<b>test</b>", then the returned length from the filter function would be "1", and the response would not be wrapped. I'm not sure what's expected here, but that's an edge case I thought of.

2. +++ b/core/misc/ajax.js
   @@ -1041,9 +1039,14 @@
   +          $('<div></div>').html(response.data);
   

   I think this could just be $reponseDataWrapped, instead of wrapping the response in another call.

3. +++ b/core/modules/system/tests/modules/ajax_test/js/insert-ajax.js
   @@ -0,0 +1,24 @@
   +  Drupal.behaviors.insertTest = {
   

   Testing with use-ajax links would be a bit simpler than this, I think.

It would be great if we could get test coverage for leading comments and whitespace as well. Thanks for taking this task on!

#129.1 is incorrect, I missed the $responseDataWrapped.contents().length call.

@samuel.mortenson Thanks for the review. U+1F44D

I want to possibly circle back to #120 and ask again, why we are even wrapping the response? It feels like we are going to get caught up in a bunch of really complicated logic that will never accurately handle all the possible edge cases that can happen with AJAX'd data.

At a high level this will sequentially add non-element nodes (text, comment, etc) from an HTML string until an element node (div, p, etc) is found. At that point, the resulting wrapped non-element nodes and the the element nodes are added to $new_content. If there are subsequent after the first element node is encountered, the process continues.

+++ b/core/misc/ajax.js
@@ -1041,9 +1039,30 @@
+      var wrapper = null;
+      $responseDataWrapped.contents().each(function (index, value) {
+        if (wrapper && value.nodeType !== 1) {
+          wrapper.append(value);
+        }
+        if (!wrapper && value.nodeType !== 1) {
+          wrapper = createWrapper();
+          wrapper.append(value);
+        }
+        if (wrapper && value.nodeType === 1) {
+          $new_content.append(wrapper, value);
+          wrapper = null;
+        }
+      });
+
+      $new_content = $new_content.children();

If $responseDataWrapped.contents() returns a single value with nodeType === 1, $new_content will be empty as "wrapper" is NULL. This should be testable with response like <div>hello</div>.

With #77 applied in D7, it breaks the admin UI for fields on entities.

@samuel.mortenson

Yeah. I didn't update the tests to account for the new behavior.

@GuyPaddock

Yeah. I have not been testing this at all in D7. I assume some additional work will need to take place to backport this.

Fixes a few edge case issues and updates the tests.

The error I was seeing in D7 was:

Uncaught TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.
    at Ga (jquery.js?v=1.4.4:130)
    at Function.css (jquery.js?v=1.4.4:128)
    at $.fn.init.hide (jquery.js?v=1.4.4:147)
    at Object.insert (ajax.js?v=7.54:536)
    at Drupal.ajax.success (ajax.js?v=7.54:428)
    at Object.success (ajax.js?v=7.54:189)
    at Object.a.success (jquery.form.js?v=2.52:12)
    at Function.handleSuccess (jquery.js?v=1.4.4:143)
    at XMLHttpRequest.w.onreadystatechange (jquery.js?v=1.4.4:142)

This affects selecting Media in the media browser for an entity that has no images yet, as well as when trying to rearrange fields in Fields UI. It appears that the case it's running into is a case where the content being added ends up being text DOM nodes, which makes the window the target.

The attached patch is a variant of 77 that works around the issue, but seems to break effects.

Test for failure, this code currently causes the issue described, with the ajax wrapper breaking a select list.

    foreach ($options as $key => $option) {
      $html .= '<option value="' . $key . '">' . $option . '</option>';
    }

    $response = new AjaxResponse();
    $response->addCommand(new HtmlCommand($element_class, $html));
    return $response;

Un-commenting the optgroup tag fixes the issue, but then causes problems when using the select list with the chosen module.

Post Patch Tests:
With wrapper already set:

    $html ='<optgroup>';
    foreach ($options as $key => $option) {
      $html .='<option value="' . $key . '">' . $option . '</option>';
    }
    $html .='</optgroup>';
    $response = new AjaxResponse();
    $response->addCommand(new HtmlCommand($element_class, $html));
    return $response;

Return without wrapper:

    foreach ($options as $key => $option) {
      $html .='<option value="' . $key . '">' . $option . '</option>';
    }
    $response = new AjaxResponse();
    $response->addCommand(new HtmlCommand($element_class, $html));
    return $response;

Return string:

    $html = 'Returning string should be wrapped';

    $response = new AjaxResponse();
    $response->addCommand(new HtmlCommand($element_class, $html));
    return $response;

Patch works with the expected behaviour for the three scenarios above. Will set this to review and tested, but realise there might be tests required that i might have missed outside of my use case, happy to take further guidance and test more scenarios as needed.

Having a quick review. It looks very similar / same as #103 but with extra redundant conditions?

Anyway, the content will be lost if data returned something like #95

var new_content_wrapped = $('<div class="test"></div>').html('BEGIN<div style="height: 100px; font-size: 30px">test</div><div style="height: 100px; font-size: 30px">test</div>END');

Missing END

Also, the same anime issue as I summarized in #104 ?

(If we allowed let it break in D8.3/4, is #103 ready to go?)

#139

Patch works with the expected behaviour for the three scenarios above. Will set this to review and tested, but realise there might be tests required that i might have missed outside of my use case, happy to take further guidance and test more scenarios as needed.

Was this tested using the patch from #137?

#140

Anyway, the content will be lost if data returned something like

var new_content_wrapped = $('<div class="test"></div>').html('BEGIN<div style="height: 100px; font-size: 30px">test</div><div style="height: 100px; font-size: 30px">test</div>END');

I believe that the patch from #137 would account for this case and not drop the END text.

Hey @drpal, yes tests in #139 were made against your patch #137.

I've tested #137 in combination with the Refreshless module. Now I get an empty divs before the inserted content instead of a wrapping div.

The problem stems from indentation. If the response.data contains for example a space before the a div, we get an extra empty div.
We should probably skip text containing only whitespace or linebreaks.

Something like this works in my case (would need test).

Edit: argggg typ0, working on version with tests

Here is a better version, it also has an extra test for the leading white space case.
Note, I rewrote the test a bit so that it runs faster (less page requests).

As mentioned earlier, we still haven't figured out what to do with theme debug comments. These get wrapped in a div now.
It could also be a follow up IMHO.

one leading or trailing SPACE is validated we cannot trim it?

@droplet, what do you mean?
Are you saying we should allow leading and trailing spaces?
I think it causes too many problems, as can be seen in the Refreshless module. If someone really wants a leading or trailing space in an unwrapped response, he/she could add  .

New patch: Removed debug line.

Fixes a minor nit with checking for length from #151. Additionally, I think it's probably a good to handle leading whitespace in this way. U+1F44D

Hmm. What behaviors do we expect and what will change in the current patch?

For example:

In patch #153

BEGIN <div>testing</div> END

Became:

<div>BEGIN </div><div>testing</div>

`BEGIN ` was an INLINE element, now this is BLOCK level. And DIV can be styled with CSS.

Comment in the current code:

we check if the new content satisfies the requirement of a single top-level element

Is it still true?
Assumed this is FALSE, then we need not do anything with `response.data`?

@droplet,
So what I understood about wrapping the TEXT_NODE / COMMENT_NODE in a div is that we do that because it is needed for animations. My preference would be that we don't wrap (we don't actually need it for comment nodes), but..

We have a bit of a deadlock here:
1. We don't want to change the DOM structure when replacing content, because that can break CSS (.some-class > .my-div). And can lead to heavy divitis (for example with the Refreshless module).
2. We don't want to really wrap text nodes and comment nodes, however it is needed for animation.
3. Currently when the return data consists out of multiple nodes, animation can be weird. For example if you use the slide animation, multiple items will slide instead of one (the later is the current behavior).

We also can't just temporarily wrap multiple nodes so that we have a single animation, and then unwrap after the animation, because styling could in that case break during animation.

My preference would be that we don't try to compensate for multiple animations, in the cases that for example a single slideToggle is needed the returned data should just be wrapped in a single block element. That is up to the implementer.

Even so, the last patch is still not complete.
The code below is responsible for attaching behaviors to the new content.

        Drupal.attachBehaviors($new_content.get(0), settings);

This only works for a single item.

So when, for example (with the latest patch) we turn theme debugging on, we only attach behaviors for the newly inserted theme debug comment. Ouch!

@dmsmidt

Would it be possible for you to update the tests to provide an example where this fails?

@drpal sure, here is a patch with an additional test on top of what we already have.

So that failed as expected, working on a patch fix.

Attached patch fixes the 'context' problem mentioned above. And tries to fix earlier mentioned problems as well.

So, to answer some questions: text nodes should not be passed to attachBehaviors since jQuery could interpret them as selectors, so we should still wrap those (sadly enough).
Currently we wrap them in a <div>, this can be destructive for a design since we return a block level item. Returning a should be less destructive to designs, and still gives the necessary design possibilities on inserted elements for themers. This has been changed in the given patch. I think this is not a BC break in practice. I also updated the tests to reflect this, and changed some typ0's.

Furthermore, comments should never be wrapped. We also don't need them in 'context' when attaching behaviors. Not wrapping them has huge benefits for when theme debugging is turned on. I changed the patch accordingly and added a test case for that as well.
The wrapping logic has been optimized in the same go. Instead of an integer I use the 'Node.COMMENT_NODE' constant for type checking. This is more readable and works in IE9+.

Lastly I also made the test a bit stricter, now it is checked that the target div is directly wrapping the new data (to ensure no other divs/spans are added).

@dmsmidt

Nice work. Thanks for handling that.

+++ b/core/misc/ajax.js
@@ -1027,22 +1027,52 @@
+      var elementsReturned = $responseDataWrapped.contents().length;

Just a slight concern about the variable name here since it's just the number of elements.

@drpal, improved var naming.

Also I found that this wasn't enough when we insert new elements with the replaceWith method.
We should test the different insert techniques that we support.
This version at least tries to make it work with 'replaceWith' next to 'html'.

However.. I don't know what will happen if we use 'replaceWith' with multiple elements.. Needs some work.

Scary that those tests pass, since I have some test sites that break hard. Thus adding 'need tests'.
At least the following patch works on an AJAX heavy test site (Refreshless + AJAX views).

I probably don't have time in the next days to work on the needed tests.
The currently added tests in this patch work with the 'html' method. We need to check that context is alright for the different inserting techniques ('replaceWith', 'append', 'prepend', 'before', 'after', or 'html').
Of those I think 'replaceWith' is the most important. The others probably work like 'html'.

last patch test, work perfect.
after apply patch, on custom block where get rendered View and commerce variante where after ajaxReplace appear empty div.

Thanks @dmsmidt

Refactored tests, adding support for replaceWith.

I like the rewrite. One discussion point:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxTest.php
@@ -82,4 +95,70 @@ public function testDrupalSettingsCachingRegression() {
+    foreach (['html', 'replaceWith'] as $method) {
+      foreach ($test_cases as $test_case) {
+        $this->drupalGet('ajax-test/insert');

This possibly makes testing slower, I'm not sure how significant it is, but we could easily skip it for the 'html' method. For 'replaceWith' it is necessary since the #wrapper will be gone after the first replace.
We could also generate different wrappers for each test type and method. Than we just need to request only a page once.
Any thoughts?

@dmsmidt

Thanks. I don't really have an opinion other than it's nice to have more complete coverage.

needs JavaScript review was done by @drpal. Test coverage is present, and looks rather comprehensive.

So… is this then RTBC again?

Hmm. with the new behavior, I'm interested what I'm missed in below patch or the test cases don't catch.

(Patch should be failed. I didn't trim the SPACE and remove SPAN in tests)

EDIT:
Just realized the code around isAllElementNode conditional can be simplified. (But with the same logic. I will update if it's right approach)

With #169,

we able to do further and wrap "NODE_ELEMENT only" response with DIV.

Any "NODE_ELEMENT only" response matches:

DIV, SPAN, UL, A... 
// (few more I think, will find it out later)
// whitelist better than blacklist I think..

It must be safe to ADD A WRAPPER. doesn't it?

- Fixed failure tests
- Enhanced the tests
- It seems big_pipe will sent out empty responses.
- In this patch, I introduced a little idea to keep inline element. But the result is the dimension of parent SPAN will be wrong and little semantic problem. However, it will keep single-parent-element result.

+++ b/core/misc/ajax.js
@@ -1031,21 +1035,26 @@
+        if (responseDataWrappedContents.length === 1 || ($wrapper.css('display') !== 'block')) {
+          $new_content = $('<span class="ajax-inline"></span>').html(response.data);

@droplet I fear you want to over-optimize, and the big rewrites are a bit hard to follow without a bit more explanation.
We have a working patch in #165, could you tell what your problems are with that approach? Did you actually find problems with that patch?
I'm up for improvements, but it would be great if you could explain them a bit more elaborate so that I can follow.

In #160 I explained the considerations for the last changes before your version. The 'inline-element' styling trick you introduce in #172 I find a bit too much, and out of scope. We already swapped

@dmsmidt,

To my understanding, your patches does not fulfill the deadlock your told in #155. But for some reason (technical?), it changed the way to render things.

#165 changed:
1. Adding SPAN to TEXT_NODE
2. Anime effect is changed.
3. No more single-parent root.

#172 fixed the bugs and keeps no (lesser) BC layer changes:
1. the dimension of parent SPAN will be wrong and little semantic problem. (Note: rendering is correct.)
2. (Basically, anime is same as now)
3. Still no single-parent root with some cases but we could do it. @see #170

with for text nodes only, this way the layout flow isn't as destructive anymore as previously.

Could you update the test case to make it fail. Thanks.

(Anyway, both patches have their Pros and Cons. I really want to find out if we give it up because of the technical issue or we really want things working that way.)

@droplet, while making the patch of #165 I took the 'deadlock' issues into consideration, and thought about what would be the best way forward.

1. We don't want to change the DOM structure when replacing content, because that can break CSS (.some-class > .my-div). And can lead to heavy divitis (for example with the Refreshless module).

The patch ensured minimal DOM structure changes, this is the biggest win! Indeed only for root text nodes a SPAN is added. The span is less destructive than DIVS for the layout. We need a wrapper, otherwise we can't pass it as behavior context.

2. We don't want to really wrap text nodes and comment nodes, however it is needed for animation.
3. Currently when the return data consists out of multiple nodes, animation can be weird. For example if you use the slide animation, multiple items will slide instead of one (the later is the current behavior).

See above, we do need to wrap text nodes, also for context. Animations I'm not that concerned with anymore. If someone wants a consistent single animation, he/she should wrap the return data in a single node element.

Currently AJAX inserts/replaces break the DOM structure. This is more problematic than the rare cases that only root text nodes are returned or the combination of multiple root elements in the return data with a slide effect. I haven't really encountered those edge cases in the wild, but in case they do, functionality doesn't break, only the animation is less pretty.

My questions about #172:

1. the dimension of parent SPAN will be wrong and little semantic problem.

We probably have a language barrier between us, because I don't really understand this ;-) Could you help me?

2. (Basically, anime is same as now)

How can this be? If we want to keep animation as how Drupal works now, we always need a single root element.
One single parent root element is just the things we want to prevent in this patch.
For example, if someone is appending items to a list of items via AJAX, every time we append, there will be an extra nesting (in case we use a single root parent element. This is problematic.

3. Still no single-parent root with some cases but we could do it. @see #170

See the above.

Does this answer your question about finding out why certain approaches where used?

This was RTBC in #140, on March 4, more than 2 months ago.

Can we please continue here? This has been causing strange bugs for years, and has been causing problems for BigPipe users in particular too: #2738685: BigPipe leaves wrapper <div>s in place, because AJAX system leaves them — this can cause some CSS to break.

#165 was the last completely functional patch as fas as I know. #140 didn't take care of multiple things (the discussion after that is not completely useless). Both @drpal and me are happy with #165, and thought it RTBC worthy. @droplet, then did a major rewrite, but never got passing tests.
Still waiting for his reply for over a month. I'd like to have this fixed a.s.a.p., but I guess we need another person to review the two versions and have an opinion about it.

reviewing

\o/ Thank you, @nod_!

Both patches had issues, but droplet had the simplest one. The approach was pretty much the same since parseHTML was doing what the previous patch tried to.

Tests pass and more importantly it fixes the issue with the image upload widget.

In any case there are some strange things going on in testing: the test ajax command return 2 command, the 'html' or 'replace' command and a second one with 'prepend'. Don't know where that second one comes from but it's strange.

I did cheat on one test, since the response is trimmed one got rid of one space in the expected data in the test.

Put back the detachbehavior on the wrapper, no idea why that got changed along the way.

My questions about #172:

1. the dimension of parent SPAN will be wrong and little semantic problem.

We probably have a language barrier between us, because I don't really understand this ;-) Could you help me?

He means it's possible for <div> to end up inside of <span> like with the inline exemple.

Fixes the tests from #181, since we don't return a div anymore.

Tested drpal's patch, +1 for RTBC

Some nits and questions.

1. +++ b/core/misc/ajax.js
   @@ -1027,22 +1027,42 @@
   +      var $new_content;
   

   Please use lowerCamelCased.

2. +++ b/core/misc/ajax.js
   @@ -1027,22 +1027,42 @@
   +      // If only NODE_ELEMENT or COMMENT_NODE elements are returned skip wrap processing.
   

   Exceeds 80 chars.

3. +++ b/core/modules/system/tests/modules/ajax_test/js/insert-ajax.js
   @@ -0,0 +1,40 @@
   + * Provides method to test ajax requests.
   

   Can we be a bit more specific with the description? Also let ppl know we are testing that attaching Behaviors work, this was a simple but important addition I think!

4. +++ b/core/modules/system/tests/modules/ajax_test/js/insert-ajax.js
   @@ -0,0 +1,40 @@
   +      $('.ajax-insert').once('ajax-insert').on('click', function (event) {
   ...
   +      $('.ajax-insert-inline').once('ajax-insert').on('click', function (event) {
   

   We have some code repetition here (only the wrapper changes).

5. +++ b/core/modules/system/tests/modules/ajax_test/js/insert-ajax.js
   @@ -0,0 +1,40 @@
   +      $(context).addClass('processed');
   

   Add a comment to let others know this is for testing if behaviours work well.

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxTest.php
   @@ -82,4 +95,76 @@ public function testDrupalSettingsCachingRegression() {
   +      $assert->assertWaitOnAjaxRequest();
   ...
   +      $assert->assertWaitOnAjaxRequest();
   

   Shouldn't we be replacing these for the assertions that waits for an element?

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxTest.php
   @@ -82,4 +95,76 @@ public function testDrupalSettingsCachingRegression() {
   +      // Extra span added by a second prepend command on the ajax requests.
   +      $assert->responseContains($this->wrapAjaxTarget('<span class="processed"></span>' . $test_case['expected']));
   

   Help, where and why does this happen again?

From #185

• 1 - Fixed
• 2 - Fixed
• 3 - Fixed, although we aren't explicitly testing behaviors here.
• 4 - I think it's probably better to have some more readable code, even if it is somewhat duplicated.
• 5 - Again, not actually testing behaviors.
• 6 - We'd probably be dependent on having an expected locator, which might match the previous test case giving us a false positive.
• 7 - We don't actually need the span, removed.

Thanks again.

4: ok!

Concerning 3&5: now it is not clear at all why we add the 'processed' class.
I added it because before this patch we just had one node element passed to attachBehaviors, but now we can have multiple. And we want to be sure that they all have been the 'context' for attachBehaviors.
Somehow we should make that clear at least.
I think it is currently the closest thing to behaviors testing we have (although still very simplistic).

#186 does not work for 8.3.1 . Although the file 8.4.x ajax.js and the 8.3.x/1 ajax.js are identical... The patch could not be applied.

yes, this is the usual process. We fix the bug in the dev version, which is 8.4, and if we think it's possible or relevant to backport we do that after it it fixed in the dev version. For now we focus on 8.4.

Just for the record, #1 is scope creep, this is not a clean-up patch.

We seem to be missing a kind of test:

<div>top-level node #1</div><div>top-level node #2</div>

About 3&5 it's less about testing behaviors than making sure attach behaviors are called when ajax content gets inserted. We're already assuming they work. And since we had one type of test missing it didn't make as much sense since we were only dealing with one top-level element in all the other cases.

I'm so very tempted to RTBC this, this seems so very close! Leaving that to those who've been closely involved lately though. Just wanted to say I'm cheering from the sidelines to see this very tricky, nasty bug nearing its final breath!

Did a bit or refactor to simplify the code a little. We have tests so it's fine, don't worry about it :þ

Remove the 'hack' for the mixed content where the leading whitespace was removed, inserted as-is now.

Another edge case, make sure that whitespaces in the middle of top-level-only nodes wrap the content:

<div>top-level node #1</div>      <div>top-level node #2</div>

I think we covered everything important?

Reroll for ES6.

Oh, we almost patched the whole insert function now. Here's few suggestions (can do in follow-up also):

1. Use ES6 directly in insert function. no var, use const...etc

2.
settings = response.settings || ajax.settings || drupalSettings;
move 2 settings to top.

3. caching $newContent.find('.ajax-new-content')

4. ideally, new JS file should be ES6 ready \core\modules\system\tests\modules\ajax_test\js\insert-ajax.es6.js

Minor:

-    $green_div = $this->assertSession()->waitForElement('css', "#ajax_selected_color div:contains('green')");
+    $green_div = $this->assertSession()->waitForElement('css', "#ajax_selected_color span:contains('green')");
 
... 

-    $red_div = $this->assertSession()->waitForElement('css', "#ajax_selected_color div:contains('red')");
+    $red_div = $this->assertSession()->waitForElement('css', "#ajax_selected_color span:contains('red')");

... etc

If we have an span instead of a div the var name should not be: $green_div, $red_div, etc.

#195: let's do that in a follow-up indeed. Let's first finally land this! :)

#196: nice nitpicks!

Let me reroll it for real quick. Actually, the ES6 give us a clean overview on scopes.

- Reroll
- ES6 syntax on insert and new file
- Fixed #195
- Fixed #196
- Fixed typo.

Same as #198 but remove a non relevant file

1. +++ b/core/misc/ajax.es6.js
   @@ -1011,38 +1011,69 @@
   +      const onlyElementNodes = responseDataNodes.every(function (element) {
   

   =>

2. +++ b/core/misc/ajax.es6.js
   @@ -1052,36 +1083,39 @@
   +        $newContent.each(function () {
   

   =>

Some minor nits about arrow functions.