Prevent accessing null elements when operating on ajax instances using the off_canvas renderer

I have seen this problem also.

I think the nulls are created here in ajax.es6.js

 detach(context, settings, trigger) {
      if (trigger === 'unload') {
        Drupal.ajax.expired().forEach((instance) => {
          // Set this to null and allow garbage collection to reclaim
          // the memory.
          Drupal.ajax.instances[instance.instanceIndex] = null;
        });
      }
    },

Here is test only patch which is trying to show that error happens. But locally it won't fail for some reason. So maybe it has something to do with another core module being enabled?

Talked with @drpal about why there would be instances of NULL in Drupal.ajax.instances.

It seems that when the form is opened up in the dialog it creates an element in Drupal.ajax.instances because the forms submit via ajax now after #2785047: In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page. So we the dialog is closed or reloaded the code I referenced in #4 is invoked which sets the element in the array to NULL.

We are not sure why the element in Drupal.ajax.instances is not just removed from the array rather than set to NULL.

Not sure how to write a test that fails or why #4 isn't failing.

Ok I couldn't figure out how to write test to confirm this fix... but I looked that code around loop through the Drupal.ajax.instances that we are affecting with the patch and I realized there was part in there that we aren't test for now that we could be testing for

instance.options.data.dialogOptions.outsideInActiveEditableId = $(instance.element).parents('.outside-in-editable').attr('id');

This is used later to set the class outside-in-active-editable on the block that is currently being edited in the off-canvas dialog.

We don't test for this. So I thought it would be good to add a test for this in the current patch because then we confirm that we aren't messing up current functionally.

Created #2898242: Delete unused Drupal.ajax.instances on unload instead of settings to null as either follow up or a fix to the root cause.

#2898242: Delete unused Drupal.ajax.instances on unload instead of settings to null was closed, because it's working as designed — it was marked as such by @droplet who helped make it work like this. Doesn't that impact this patch too then? Nope, that just confirms that this patch makes sense.

The patch has test coverage. But its test-only patch also passes…

I think you can test it "quite easily" though:

1. have something on the page that has #ajax => something
2. trigger that AJAX, and make it replace the existing content, but ensure that the new content ALSO has some #ajax stuff (perhaps to replace the same thing once again)

Then step 1 has AJAX instance X associated. Step 2 triggered AJAX instance X, caused its associated element to be deleted (because replaced HTML), and also had a Settings command (for its new AJAX instance X+1). Executing the Settings command triggers a call to Drupal.ajax.expired(), which is what causes AJAX instance X to become null.

Knowing that, I think it's possible to write a test?

I opened a new issue and also patched the same place. You may interested:

#2901667: To optimize outside_in Drupal.behaviors.toggleEditMode

This blocks #2884601: Add a Layout Builder to core

This issue blocks two experimental modules, so promoting to major.

Re #10 and the need for tests.

If you look at #4 it does a very similar test. It opens the tray and closes multiple time in multiple different configurations. If you try this manually you can see you get the JS console error but actually functionality is broken.

So I am pretty our JavascriptTestBase test don't fail just because of JS console error. So am not sure to test this. Marking back as RTBC because don't have unit tests we actually can't write test for this type of error

If you see #2551373: contextual.js and quickedit.js should fail gracefully, with useful error messages, when Twig templates forget to print attributes it had the same problem, a JS console error was impossible to write a test from. We should have JS unit tests but I don't think we should postpone this issue on that.

#14 looks like a good idea and a completely safe change to make close to releasing 8.4-beta, so pushed it to 8.5.x and 8.4.x.

Looks like #2901667: To optimize outside_in Drupal.behaviors.toggleEditMode has some additional nice improvements, so reviews there would be great.

A follow-up for adding test coverage would be really good. Even if it needs to be postponed on some other issue, let's at least get the relevant issues filed. Thanks!

Changing to new settings_tray.module component. @drpal thanks for script help! :)