Changes to "Manually editable HTML tags" lost if form is submitted without triggering AJAX

Ran into this earlier today. Confirming data loss/confusion! Bad for usability for sure.

The step 4 on the IS is really important. I've noticed some things:

1. The issue above also happens when removing tags previously saved and not clicking anywhere else but in the "Save configuration" button.
2. As soon as you click outside the input box right after adding/removing a tag, the box flashes out really fast. I guess it means something happens when click anywhere else (probably the system tracking the modification on the input box), that doesn't happen (or doesn't have enough time to happen) whey you click directly on the Save button right after modifying the tags.
3. I added another textarea right bellow the Manually editable HTML tags just for test, with an empty default value. The flashing mentioned on 2 doesn't happen when typing and clicking outside this.

I'd like to go deeper on this issue, but I'm still learning Drupal and have no clue on how to proceed from this point, so I'll keep following this issue to see if I can learn why this is happening and how to fix it.

I am struggling to write a failing test for this.

It looks like

    // Configure it.
    $source_editing_allowed_tags_textarea->setValue('<div data-foo>');

somehow triggers the AJAX updates anyway? 🤔

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
@@ -114,6 +116,50 @@ function (ConstraintViolation $v) {
+    $javascript = <<<JS
+      document.querySelector('[data-drupal-selector="edit-editor-settings-plugins-ckeditor5-sourceediting-allowed-tags"]').value = '<div data-foo>';
+JS;
+    $this->getSession()->executeScript($javascript);

Aha! This is the crucial difference!

Thanks @bnjmnm :)

Struggling with reproducing this. The tests fail as well for me too.

To reproduce locally I have to add a delay to the ajax calls and click twice on the submit button for the problem to show up. A single click doesn't submit the form, just gets the ajax to trigger.

Interesting, @nod_! Which browser? Firefox? I've been testing with Chrome, and that's what DrupalCI tests with.

Maybe Chrome and Firefox process a trigger of the form submit button differently… 🤔

Have not found the cause, but a few things I've narrowed down:

• Ran Xdebug while following the steps to reproduce and found:

    public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {
      // ISSUE #3265626  $form_value is "" when clicking submit before  an AJAX rebuild.
      $form_value = $form_state->getValue('allowed_tags');
      if (!is_array($form_value)) {
        $config_value = HTMLRestrictions::fromString($form_value)->toCKEditor5ElementsArray();
        $form_state->setValue('allowed_tags', $config_value);
      }
    }
  

• Removing this fixes the issue

      $source_allowed_tags['#ajax'] = [
            'callback' => '_update_ckeditor5_html_filter',
            'trigger_as' => ['name' => 'editor_configure'],
            'event' => 'change',
          ];
  

  But obviously that breaks the dynamic updating of HTML restrictions

I feel like that narrows it down considerably, but I'm not sure what to yet...

Ok to reproduce the "Limit allowed HTML tags and correct faulty HTML" checkbox needs to be checked.

Thanks ben, that helped me find the issue :)

When the submit button is clicked, the ajax callback is triggered and this piece of code is run on the textarea:

    // Disable the element that received the change to prevent user interface
    // interaction while the Ajax request is in progress. ajax.ajaxing prevents
    // the element from triggering a new request, but does not prevent the user
    // from changing its value.
    $(this.element).prop('disabled', true);

Textarea become disabled and the value is not sent as part of the form on submit. Commenting this line make things work as expected.
Don't know yet why when "Limit allowed HTML tags and correct faulty HTML" is not enabled it behaves as intended (form submission is prevented on first click on the save config button) but when the filter is enabled it just submit the form.

fixes it when manual testing. Doesn't seem to make the test pass though.

I don't particularly like the solution, just the first thing that worked, need polish.

I'm not sure the same thing is going on in the test than manually testing. It doesn't seem to have the ajax call triggered when assigning .value to the textarea and clicking on the button, but if that is the case don't know why it would fail....

Sooo, seems like the no-js version doesn't even save the value properly. I visit the page with JS disabled, add a <kbd> element to the Manually editable HTML tags field, save the form… and nothing is added to the Allowed HTML tags textarea.

It seems like _update_ckeditor5_html_filter does extra work compared to the regular form submit. It seems that $renderedField = $renderer->render($form['editor']['settings']); is the piece of code responsible for the changes actually propagating.

Wow, nice sleuthing! Seems like this one is extremely tricky 😬

Attached is an arguably hacky solution, but one that works for me locally (we will see if Drupal CI agrees). Maybe this will guide us to a more robust solution, but at the very least we now know this can be addressed somehow. Explanation of how the fix works are in the code comments.

In #20 Cspell was objecting to a word in a transpiled js file that removes cspell: ignore as part of the transpilation, so the term was added to dictionary.txt.

So … does this mean you gave up on the \Drupal\Core\Form\SubformStateInterface::getCompleteFormState() stuff in a submit handler? IIRC you said on Friday you wanted to try that, but #20 doesn't quite look like that? 🤔😅

Re #23

So … does this mean you gave up on the \Drupal\Core\Form\SubformStateInterface::getCompleteFormState() stuff in a submit handler?

Turns out it needs what I added in #21 AND a submit handler. There are two problems being addressed

1. The value of "Manually editable tags" is seen as NULL if the field value changes but there's no 'change' event (fixed with the weird hidden field in SourceEditing)
2. "Limit allowed HTML tags" does not account for the new value of "Manually editable tags" if there is no 'change' event on "Manually editable tags" (fixed by adding the submit handler that combines the field values with HTMLRestrictions)

1. 👍 The test coverage looks great!

2. +++ b/core/modules/ckeditor5/ckeditor5.module
   @@ -236,6 +237,26 @@ function _add_ajax_listeners_to_plugin_inputs(array &$plugins_config_form): void
   +function ckeditor5_filter_format_edit_form_submit(array $form, FormStateInterface $form_state) {
   +  if (isset($form['filters']['settings']['filter_html']['allowed_html'])) {
   +    // When "Manually editable tags" 👇 and "limit allowed HTML tags" 👆 are both
   +    // configured, the former informs the value of the latter. This dependent
   +    // value is typically updated via AJAX, but it's possible for "Manually
   +    // editable tags" to update without triggering the AJAX rebuild. That value
   +    // is recalculated here on save to ensure it happens even if the AJAX
   +    // rebuild doesn't happen.
   +    if ($manually_editable_tags = $form_state->getValue(['editor', 'settings', 'plugins', 'ckeditor5_sourceEditing', 'allowed_tags'])) {
   +      $manually_editable_tags_restrictions = HTMLRestrictions::fromString(implode($manually_editable_tags));
   +      $format = $form_state->get('ckeditor5_validated_pair')->getFilterFormat();
   +      $allowed_html = HTMLRestrictions::fromTextFormat($format);
   +      $combined_tags_string = $manually_editable_tags_restrictions->merge($allowed_html)->toFilterHtmlAllowedTagsString();
   +      $form_state->setValue(['filters', 'filter_html', 'settings', 'allowed_html'], $combined_tags_string);
   +    }
   +  }
   +
    }
   

   👍 This one I understand. This corresponds to #24.2.

3. But everything else (so besides the tests and the diff quoted in the previous bullet) I do not understand. I don't understand how #24.1 could happen. The test coverage also doesn't cover this AFAICT.

   It's also not the bug originally reported AFAICT, so perhaps it's worth tackling in a separate issue? But let's first verify that this really is a separate bug, because I'm really puzzled on how to reproduce it at all 🙈

So … extracted a subset of #25, that covers the original issue scope and includes the original test coverage.

+++ b/core/modules/ckeditor5/ckeditor5.module
@@ -245,6 +246,26 @@ function _add_ajax_listeners_to_plugin_inputs(array &$plugins_config_form): void
+  array_unshift($form['actions']['submit']['#submit'], 'ckeditor5_filter_format_edit_form_submit');
+}
+
+function ckeditor5_filter_format_edit_form_submit(array $form, FormStateInterface $form_state) {
...
+      $format = $form_state->get('ckeditor5_validated_pair')->getFilterFormat();

🤓 I tried removing this form-level submit handler and instead updated \Drupal\ckeditor5\Plugin\Editor\CKEditor5::getGeneratedAllowedHtmlValue(). But the end result was A) more confusing, B) could not rely on $form_state->get('ckeditor5_validated_pair')->getFilterFormat(); being available.

🙏 So all I think that remains here is adding the missing docblock and perhaps an @see _update_ckeditor5_html_filter().

Here's a video with both issues, from HEAD: https://youtu.be/osxqeWtn5Ro

If I update the Source Editing Manually editable HTML Tags field and go directly to save there are two problems

1. (this part has test coverage): The HTML Filter Allowed HTML tags does not update the with the new tag added to source editing.
2. (this one is unclear how I can test): The change to Source Editing Manually editable HTML Tags wasn't saved either! I previously thought the problem was the field value was null, but I was only testing on empty textareas. The problem is the changes not being acknowledged, not the field value being set to null.

Additional weird (interesting?) thing
I paused the video at 13 seconds, right after "Save configuration" was clicked. For a moment, the Allowed HTML tags is updated.

Additional weird (interesting?) thing
I paused the video at 13 seconds, right after "Save configuration" was clicked. For a moment, the Allowed HTML tags is updated.

I bet that's the AJAX request being fired just at the time you are clicking Save configuration (because that is what triggers the change event), and the AJAX request getting a response faster than the form submission finishes.

… which I think points to the root cause of all of this: in HEAD, it's possible to click the form-level Save configuration button whenever you want — even while A) an AJAX request still is being processed, B) an AJAX request is yet to be fired.

This is a long-standing weirdness/weakness in Drupal. Even on d.o — which runs Drupal 7 — you can get into trouble with this. Hundreds of issues/discussions/bug reports exist about this: https://www.google.com/search?q=drupal%20disable%20submit%20button%20whi...

In the case of the CKEditor 5 admin UI, the AJAX aspects are critical for it to work correctly (hence this bug report), and we're not yet doing what we can to ensure that all necessary AJAX requests + responses have ben processed.

I think this can be solved relatively simply for the CKEditor 5 admin UI (not generically):

1. Disable the Save configuration button as soon as there's any input to any of the CKEditor 5 plugin settings — not just a change event:
   

   document.querySelector('#editor-settings-wrapper').addEventListener('input', function () { document.querySelector('#edit-actions-submit').disabled = true; })
   

2. Disable the button too at the start of AJAXing, and re-enable it when you get a response. You can copy/paste the code from #3084091-3: Disable filters while processing submission via AJAX.

When I applied those two measures, I also cannot reproduce the bugs anymore! 🥳 Curious what you think!

P.S.: to test this, I think we need something like:

diff --git a/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php b/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
index 140ba1aff6..cbf9fb9861 100644
--- a/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php
@@ -144,12 +144,16 @@ public function testSourceEditingSettingsForm() {
     $javascript = <<<JS
       const allowedTags = document.querySelector('[data-drupal-selector="edit-editor-settings-plugins-ckeditor5-sourceediting-allowed-tags"]');
       allowedTags.value = '<div data-foo>';
-      allowedTags.dispatchEvent(new Event('input'));
+      allowedTags.dispatchEvent(new Event('input',  {
+        bubbles: true,
+        cancelable: true,
+      }));
 JS;
     $this->getSession()->executeScript($javascript);
 
-    // Immediately save the configuration. Intentionally do nothing that would
-    // trigger an AJAX rebuild.
+    // Save the configuration as soon as we're able to.
+    $this->assertNotEmpty($assert_session->waitForElement('css', 'input#edit-actions-submit[disabled]'));
+    $this->assertNotEmpty($assert_session->waitForElement('css', 'input#edit-actions-submit:not([disabled])'));
     $page->pressButton('Save configuration');
 
     // Verify that the configuration was saved.

IOW countless related ancient bugs:

• https://www.drupal.org/project/issues/drupal?text=prevent+submit&compone...
• https://www.drupal.org/project/issues/drupal?text=disabled&component=aja...
• https://www.drupal.org/project/issues/drupal?text=disable&component=ajax...

😭

Per #30 … I'm pretty sure this is actually related to #3262484: AJAX desync when quickly changing multiple form values in plugin settings vertical tab, and doing what I proposed in #30 would fix that too? 🤓

so #25 and #26 don't fix the no-js version of the form. I know it's a configuration for a editor that require JS but the fact that the form processing is broken in the no-js case, makes me think there is still a hidden issue somewhere. Would be needed for the solution below too.

I'm not too found of the hidden field solution. that feels like having a workaround for the fact that a disabled textarea isn't part of the submitted form, when we could easily remove the disabled attribute instead of having a copy of that field.

Disabling the save button when the change event gets triggered sounds like a good solution. I'm not sure what is achieved by disabling it on the input event (also, if we have a bunch of input events but no change event triggered (like you're typing something but then remove all changes no change event is triggered, how can we submit the form?)

The solution I like best though would be fixing the no-js form and removing the disabled attribute on the textarea if it exists when the form is submitted (even simpler than #15) with something like below would take care of this issue without having to disable the submit button.

$('form').on('submit', function (e) {
  const $form = $(this);
  $form.find('[disabled]').removeAttr('disabled');
});

Fixing the nojs form seems like it's a requirement given that there is an explicit fallback for the toobar config in a no-js situation.

fact that the form processing is broken in the no-js case

You'd mentioned this before but I'd forgotten about it. This is a really good point. I want to make this work.

I'm not sure what is achieved by disabling it on the input event

Because as soon as any typing has happened in the "Manually editable HTML tags" <textarea> (or any other input), we need to wait for AJAX processing to ensure everything is in sync by the time the form is submitted. That's why it's listening to the input event, not the change event.

You're right that this means that you cannot submit the form immediately after typing in that input, because that would not trigger the change event. And that's the very point.

It'd be better if the change event were triggered automatically before the user could submit the form, but AFAIK that just doesn't happen. But perhaps you see a way?! 🤓

The only alternative I see right now is to add an "Update" button next to the "Save configuration" button. It'd only be visible when there is not yet an in-progress AJAX request. And whenever it's visible, the "Save configuration" button would not be visible — so to the end user it'd be as if the "Save configuration" button sometimes changes to an "Update" button.

(also, if we have a bunch of input events but no change event triggered (like you're typing something but then remove all changes no change event is triggered, how can we submit the form?)

I … thought that would also trigger the change event? :O

Agreed on your recommendation for a starting point. Now that you are providing reviews here, I'll start pairing with Ben on this one. Let me give it a try before he starts his day :)

I … thought that would also trigger the change event? :O

Change event is only triggered when there is an actual change to the form element value :)

All morning went to #3231334: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss) 😬

This uses the approach suggested by @nod_ for the GHS field being empty, and retains the approach in #25 to update the allowed tags field correctly. I manually confirmed that part works even with JS disabled.

No interdiff as this is changed enough that it's easiest to classify this as a new approach.

1. +++ b/core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js
   @@ -3,9 +3,26 @@
   +      once(
   +        'ajax-conflict-prevention',
   +        context.querySelector('[data-drupal-selector="edit-actions-submit"]'),
   +      ).forEach((submit) => {
   

   I know it's similar to what's below. We can do this too:

   once('ajax-conflict-prevention', '[data-drupal-selector="edit-actions-submit"]', context)
   

2. +++ b/core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js
   @@ -3,9 +3,26 @@
   +          document
   +            .querySelectorAll(
   +              '[data-drupal-selector="filter-format-edit-form"] [disabled][data-once="drupal-ajax"], [data-drupal-selector="filter-format-add-form"] [disabled][data-once="drupal-ajax"]',
   +            )
   +            .forEach((disabledElement) => {
   

   That could be written as

               once.filter('drupal-ajax', '[data-drupal-selector="filter-format-edit-form"] [disabled], [data-drupal-selector="filter-format-add-form"] [disabled]'),
   
   

   to make use of what once provides.

3. +++ b/core/modules/ckeditor5/ckeditor5.module
   @@ -245,6 +246,28 @@ function _add_ajax_listeners_to_plugin_inputs(array &$plugins_config_form): void
   +    if ($manually_editable_tags = $form_state->getValue(['editor', 'settings', 'plugins', 'ckeditor5_sourceEditing', 'allowed_tags'])) {
   

   This doesn't work when we want to empty the "Manually editable HTML tags" field (since $manually_editable_tags is an empty array in that case). Not sure why we would want the plugin and not have anything configured, but it's unexpected. I try to add a tag, it's added properly to the allowed html, but if I try to remove it it doesn't update the allowed html field and the tag is still allowed.

only 3 is NW, 1 and 2 are more details than anything.

Source editing without extra editable html tags is actually valid, to see the generated html.

Addresses #38 - the Js suggestions are great and the removing-tags is addressed by checking is_array instead of true/false on the form input value.

except for the CI checks failing this is RTBC for me.

spoke a little too soon

1. +++ b/core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js
   @@ -3,9 +3,28 @@
   +        submit.addEventListener('click', () => {
   

   this is probably the only question, shouldn't we use the submit event? there is a related issue with the ajax framework that right click trigger the call: #2616184: Right click should not submit buttons with Ajax behaviors

2. +++ b/core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js
   @@ -3,9 +3,28 @@
   +((Drupal, once, $) => {
   

   don't think we need jQuery here.

Addresses #42

Thanks, looks good to me

I got stuck trying to work out what the 👇 and 👆 in the comment actually means. I think we should consider changing the form submission handler to be something like below. I think we can use variable names to make it easier to understand and I don't think optimising to not call $form_state->getValue() is necessary.

/**
 * Form submission handler for filter format forms.
 */
function ckeditor5_filter_format_edit_form_submit(array $form, FormStateInterface $form_state) {
  $limit_allowed_html_tags = isset($form['filters']['settings']['filter_html']['allowed_html']);
  $manually_editable_tags = $form_state->getValue(['editor', 'settings', 'plugins', 'ckeditor5_sourceEditing', 'allowed_tags']);
  if ($limit_allowed_html_tags && is_array($manually_editable_tags)) {
    // When "Manually editable tags" and "limit allowed HTML tags" are both
    // configured, the former informs the value of the latter. This dependent
    // value is typically updated via AJAX, but it's possible for "Manually
    // editable tags" to update without triggering the AJAX rebuild. That value
    // is recalculated here on save to ensure it happens even if the AJAX
    // rebuild doesn't happen.
    $manually_editable_tags_restrictions = HTMLRestrictions::fromString(implode($manually_editable_tags));
    $format = $form_state->get('ckeditor5_validated_pair')->getFilterFormat();
    $allowed_html = HTMLRestrictions::fromTextFormat($format);
    $combined_tags_string = $manually_editable_tags_restrictions->merge($allowed_html)->toFilterHtmlAllowedTagsString();
    $form_state->setValue(['filters', 'filter_html', 'settings', 'allowed_html'], $combined_tags_string);
  }
}

Ran into this issue as well (adding tag to the source editing field, and when submitting seeing briefly an error message but still submitting)

The patch in #43 fixed this for me

Address #45, good suggestion.

Confirmed that #47 addresses feedback in #45

Credited @lauriii for filing the issue and @andregp for additional issue details and myself for patch review.

Committed 9117ebf and pushed to 10.0.x. Thanks!
Committed 2d19c2f and pushed to 9.4.x. Thanks!
Committed 925fab2 and pushed to 9.3.x. Thanks!

Backported to 9.3.x resolved some fun JS build conflicts on the way.