Modal form actions broken if subform has validation errors or certain AJAX callbacks

Created on 22 February 2022, almost 3 years ago
Updated 19 April 2024, 10 months ago

Problem/Motivation

As soon as a layout paragraph to be added has a validation error, it's no more possible to save the layout paragraph modal.

Steps to reproduce

This can be easily reproduced with a text paragraph:

  1. Create a paragraph with a text field. Set the textfield to be required.
  2. Add this text paragraph to a node with layout paragraphs, but don't fill any text and click "Save". A validation error pops up correctly, saying the value is required.
  3. Now fill in text to fix the error and try to click "Save" again. Nothing happens.

Proposed resolution

The modal action buttons have to be updated on content validation errors.

Remaining tasks

User interface changes

API changes

Data model changes

🐛 Bug report
Status

Fixed

Version

2.0

Component

Code

Created by

🇩🇪Germany Anybody Porta Westfalica

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Any Updates on this issue? I also still see the action button twice with the Patch #6

  • #6 allows the actions to be clickable again. However, as mentioned, the duplicated buttons persist.

    Here is another path to reproduce this issue. If a form element has an AJAX callback which alters the entire form, then its actions will be duplicated as well. A workaround is using a more specific wrapper element than the <form> itself. I mention this because AJAX seems to be the general cause of this issue, not specific to validation errors:

    $form['field_example']['#ajax'] = [
      'callback' => 'my_ajax_callback',
      'trigger' => 'change',
      'wrapper' => $form['#id'],
    ];
    
    function my_ajax_callback(&$form, FormStateInterface $form_state): array {
      return $form;
    }
    

    My first thought was that the patch could simply add a line to remove the new actions from the DOM. But then I considered a use case where one might alter those actions in an AJAX callback for whatever reason. So might we replace the old actions with the new ones instead? Unfortunately I can't speak to the complexity of that solution.

  • nicross-com changed the visibility of the branch 3265794-modal-form-actions to hidden.

  • nicross-com changed the visibility of the branch 3265794-modal-form-actions to active.

  • Here's my solution in builder.js which moves code from the dialog:aftercreate event handler into a reusable updateDialogButtons() function. We also call it within the behavior attachment so it's called after AJAX complete:

    Drupal.behaviors.layoutParagraphsBuilder = {
      // ...
      updateDialogButtons();
    }
    
    var $lpDialog;
    
    $(window).on('dialog:aftercreate', function (event, dialog, $dialog) {
      if ($dialog.attr('id').indexOf('lpb-dialog-') === 0) {
        $lpDialog = $dialog;
        updateDialogButtons();
      }
    });
    
    $(window).on('dialog:afterclose', function () {
      $lpDialog = undefined;
    });
    
    function updateDialogButtons() {
      if (!$lpDialog) {
        return;
      }
      var buttons = [];
      var $buttons = $lpDialog.find('.layout-paragraphs-component-form > .form-actions input[type=submit], .layout-paragraphs-component-form > .form-actions a.button');
      if ($buttons.length == 0) {
        return;
      }
      $buttons.each(function (_i, el) {
        var $originalButton = $(el).css({
          display: 'none'
        });
        buttons.push({
          text: $originalButton.html() || $originalButton.attr('value'),
          class: $originalButton.attr('class'),
          click: function click(e) {
            if ($originalButton.is('a')) {
              $originalButton[0].click();
            } else {
              $originalButton.trigger('mousedown').trigger('mouseup').trigger('click');
              e.preventDefault();
            }
          }
        });
      });
      if (buttons.length) {
        $lpDialog.dialog('option', 'buttons', buttons);
      }
    }
    

    Sorry about misclicking the hide button on the old issue fork. I'll try moving this into a new issue fork with ES6 as well. Please feel free to clean this up or fix edge cases. I'm still learning the Drupal JS standards and how to contribute to this project.

  • Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update about 1 year ago
    Not currently mergeable.
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update about 1 year ago
    7 pass, 32 fail
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update about 1 year ago
    7 pass, 32 fail
  • Pipeline finished with Canceled
    about 1 year ago
    #82056
  • Pipeline finished with Failed
    about 1 year ago
    #82061
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update about 1 year ago
    5 pass, 34 fail
  • Pipeline finished with Pending
    about 1 year ago
    #82070
  • I think I've taken this as far as I can for now. Note that the patch for MR151 is failing to apply for me with composer on the latest dev. I think it's related to that last line of builder.js in my first commit. In the dev branch we're passing once, but that's curiously omitted from the patch as if it's based on an earlier branch. Let me know if there's a way to fix that without starting over.

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update about 1 year ago
    55 pass
  • Pipeline finished with Running
    about 1 year ago
    #82094
  • 🇺🇸United States NathanDanielsonCOM

    Our team has been doing in-depth testing of this patch with a wide range of our paragraphs, some very simple and some complex, without running into the duplicated Save/Cancel bug. We've tried various scenarios without seeing any unintended issues and are planning to take this patch into a large production site. Does anyone else have any testing feedback?

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update 11 months ago
    55 pass
  • Composer does not like the patch file for Merge Request 151 so I've recreated it. #151

  • First commit to issue fork.
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update 11 months ago
    55 pass
  • 🇺🇸United States sethhill

    The above approach by @nicross-com of updating the dialog buttons works as expected. In MR156 I refactored that approach to avoid setting the global variable to track the dialog, and addressed some ESLint errors that were reported in the builder.

  • Pipeline finished with Success
    11 months ago
    Total: 10202s
    #135623
  • Pipeline finished with Skipped
    11 months ago
    #136652
  • First commit to issue fork.
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update 11 months ago
    55 pass
  • Status changed to Fixed 11 months ago
  • 🇺🇸United States justin2pin

    Merged MR156 – Thanks all!

  • Automatically closed - issue fixed for 2 weeks with no activity.

  • 🇩🇪Germany berliner

    The patches have been a bit greedy though. Instead of only moving the form action submit buttons and button links into the dialogs button pane, it now moves all submit buttons and button links there.
    So it's now doing exactly what it was meant not to do, according to this comment: https://git.drupalcode.org/project/layout_paragraphs/-/commit/9e6ce29756...

    I have raised 🐛 Dialog form button logic is too greedy Active as a follow-up issue to fix this.

Production build 0.71.5 2024