Dialog drupalAutoButtons option should be respected on initial load

Good to see it's optional :)

It works for new content loaded in, amusingly enough. Just not for the initial dialog content.

We should be able to test this now.

Hi all!
I met that issue when tried to manage webform inside dialog. Smth like:

hook_link_alter():
      $data_dialog_options = [
        'drupalAutoButtons' => FALSE,
        'width' => 768,
        'height' => 'auto',
        'position' => ['my' => 'top+260', 'at' => 'top'],
      ];
      $variables['options']['attributes']['data-dialog-options'] = Json::encode($data_dialog_options);

I wanted to have submit button inside webform instead dialog one.
In html that dialog link looks ok:
data-dialog-options="{"drupalAutoButtons":false,"width":768,"height":"auto","position":{"my":"top+260","at":"top"},"dialogClass":"newsletter-form"}"
https://www.drupal.org/files/issues/core-js-dialog-autobuttons.patch failed for me. My issue was because that FALSE option appears as string not boolean. It happends after AJAX submit.
That patch helped. But that issue needs some work still.

It definitely needs tests

I'm sure better to find a cause of type conversion for settings withing drupal.ajax

+++ b/core/misc/dialog/dialog.ajax.js
@@ -130,9 +130,11 @@
+    if (!response.dialogOptions.drupalAutoButtons || response.dialogOptions.drupalAutoButtons !== 'false') {

when boolean converted to string?

Hi guys, I noticed this patch will update code into `dialog.ajax.js` file, but I also noticed that there's a comment in the very begginning of that file, "DO NOT EDIT THIS FILE." because of this:
https://www.drupal.org/node/2815083

I would like to know what's plan for this case?

@andypost Rerolled patch in latest 8.7.x DEV in the correct file.

I have aded a patch that will update both `dialog.ajax.js` and `dialog.ajax.es6.js` so it works again after patching.

@Bram Linssen see#12. Hidden #16 and #14 is the one to review. This still seems broken in latest 8.x and 9.x!

#14 didn't work for me, I guess
if (!response.dialogOptions.buttons || response.dialogOptions.buttons !== 'false') {
must be
if (!response.dialogOptions.buttons && response.dialogOptions.drupalAutoButtons !== 'false') {

Additionally it seems that the setting is even not respected if this function is triggered afterwards?

$dialog.on('dialogButtonsChange', function () {
      var buttons = Drupal.behaviors.dialog.prepareDialogButtons($dialog);
      $dialog.dialog('option', 'buttons', buttons);
    });

I think we need tests for all these cases.

Sorry I committed into the wrong branch -.- Correct Draft MR2 as cherry-pick above!

Tests are still required and missing, the MR2 now shows the planned implementation of the fix with the correct variable:
https://git.drupalcode.org/issue/drupal-2793343/-/merge_requests/2.patch

Just tested MR2 successfully in this related issue #3191418: Set 'drupalAutoButtons' => FALSE for modal to stop all buttons being moved into modal actions and can confirm it works perfectly and fixes the bug.

I also checked the point raised in #6. It's a boolean correctly!

What's left now are tests and that's it :)

I also created a HOTFIX patch here for the dialog.ajax.es6.js AND dialog.ajax.js file if someone needs it until this is fixed:
https://git.drupalcode.org/project/drupal/-/merge_requests/222.patch

I tested it successfully with 8.9.x and 9.x!

Just tested this on my end on core 8.9 and hit a small issue.
When loading a modal via ajax, it looks like docroot/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php takes in modal options via GET and returns them with the response. When this happens, the drupalAutoButtons setting gets changed from a bool false to string false. This causes the conditional check to fail. Changing this to != "false" fixed my issue. Not sure if that is a proper solution...

I can confirm #32. false is passed as "false" from the query parameters.

Also. For some reason defining a link with buttons: [], the buttons attribute gets stripped out. And I have absolutely no idea why.

        '#attributes' => [
          'class' => ['use-ajax'],
          'data-dialog-type' => 'dialog',
          // 'data-dialog-renderer' => 'wide' throws an error.
          'data-dialog-options' => json_encode([
            'width' => 700,
            'drupalAutoButtons' => FALSE,
            // @todo drupal.ajax.js does not respect drupalAutoButtons properly,
            //   pass a dummy array until https://www.drupal.org/node/2793343.
            'buttons' => [],
          ]),

EDIT: It's not for some reason. It's a jQuery thing.

This still isn't the best fix, because the following returns false positives (stringified false)

      if ($dialog.length) {
        // Remove and replace the dialog buttons with those from the new form.
        if ($dialog.dialog('option', 'drupalAutoButtons')) {

My suggestion to solve this ...

#14 works for me. Need a working patch for version 8.9.x, so I added this patch.

Drupal 8 support has been ended.

Tried to fix custom errors. Thanks.

Here is a new patch to fix CS.

Wow - sorry everybody for the complete mess in MR!1018. Rebasing locally against 9.4.x was obviously a mistake...

Attached is a patch against 9.4.x and an interdiff against #44.

The new patch includes:

• Updates recommended by @DanielKulbe to correctly respect default, true, and false settings of drupalAutoButtons.
• Fixed typos from said updates.
• Automated tests to make sure the drupalAutoButtons option is respected.

One more time... updated patch includes a correctly transpiled dialog.ajax.js file to ensure passes all automated tests.

This still needs work. Few thoughts:

• Need to fix issue breaking ModalRendererTest when "Button pane modal!" link is clicked.
• JS can probably be further simplified.
• Instead of having an entirely separate test module / class, these tests should just be included in the existing dialog_renderer_test and ModalRendererTest module and class, respectively.

I'll work on an updated patch.

Assuming the latest patch passes tests, I think this is ready for review. A few notes/assumptions:

• If the buttons dialog option is set, its contents will be used as dialog buttons regardless of the drupalAutoButtons option.
• The drupalAutoButtons option defaults to true.
• If drupalAutoButtons is true and the buttons option is empty, all form actions buttons will be moved into the jQuery UI dialog buttons area.
• If drupalAutoButtons is set to false, form action buttons will be left as-is.
• Tests for drupalAutoButtons option values (default, true, and false) have been added to the existing dialog_renderer_test module and ModalRenderTest class.

Minor issue with #51 - removed duplicate line response.dialogOptions = response.dialogOptions || {};.

One more re-roll -- missed transpiling the js file in the last one. Sorry for spamming the comments.

Thank you very much @justin2pin for looking into this, fixing things and writing the required tests. I'll remove the "Needs tests" tag now, as it now has tests!

Two things left for RTBC from my side:
1. Did you read through all the comments above like for example #6, #23, #32, #34, #36 and ensure these are fixed and have a test case?
I'm not sure if we should add tests for the stringified false and such other strange edge cases... but it might prevent us from future mistakes and misunderstandings?
From my manual tests and review, I'd say they are fixed, but this should be proofed.

2. Perhaps the following two lines should have a short inline comment explanation why this is needed or a reference to this issue? Otherwise, future developers may think this is a bug or can be cleaned up? ;)

+    } else if (response.dialogOptions.drupalAutoButtons === 'false') {
+      response.dialogOptions.drupalAutoButtons = false;
+    } else {
+      response.dialogOptions.drupalAutoButtons = !!response.dialogOptions.drupalAutoButtons;
+    }

Afterwards, this is RTBC from my perspective!
Perhaps the others here can also have a look already?

Thanks @Anybody! I just looked at those comments again, and I think those use cases are covered with #53. Because of the way Ajax requests are processed, the automated tests actually do test for stringified false (and only for stringified false as far as I can tell). I added addition comments in the latest re-roll, and am adding #3014136: Opening dialog via AJAX casts dialogOptions values to strings as a related issue. Just FYI, the yarn build script strips comments in dialog.ajax.js. See dialog.ajax.e6.js instead, as well as the interdiff for what changed.

I guess "perfect" is the right word for what you did! :) Great idea to link that issue!

Just had another look and I'd see this as RTBC! Hope for a core maintainer review and feedback soon :)

There's @todo comment mentioning this issue in \Drupal\Core\Ajax\OpenOffCanvasDialogCommand::__construct. We should either address that here or open a follow-up.

#53 looks good! Would love to see this committed :)

Looks like test failures are unrelated.

Send retest

I've used the last patch for Drupal 9 in combination with Layout Paragraphs and it works great.
But if i have an invalid (or required and empty) field after (ajax-)submit than the submit buttons displayed twice.
Is there any solution for that?

Here's a patch for 9.5

Changing to needs review to trigger automatic testing.

Thanks @beunerd #73 works against 9.5.x! But has code style issues.

Could someone please finally prepare a 10.1.x MR? Hopefully we can finally fix this long outstanding issue?
For 9.5.x we should also prepare a fix.

#76 works for Drupal 10.0 - thanks @bnjmnm! We're close here, only 2 failing tests left!

Removing the needs follow up as it seems the todo mentioned in #57 is being addressed.

Think the issue summary could use some love though.

Here's the 9.5.x patch only for the project I'm on

Here's a rerolled version for 11.x. Also updated the issue summary.

Tested this using the module provided dialog_renderer_test
Went to /dialog_renderer-test-links (like the test)
Verified Auto buttons default! and Auto buttons false! both contained .ui-dialog-buttonpane without the fix

With the fix only the first link contained .ui-dialog-buttonpane.

This basically looks good, but I see a risk of introducing an intermittent test failure. Lets get that addressed and I bet this is good to go. It's simple enough that if the person who addresses it is comfortable with the change they can self-switch this back to RTBC.

+++ b/core/modules/system/tests/src/FunctionalJavascript/ModalRendererTest.php
@@ -74,6 +74,29 @@ public function testModalRenderer() {
+    $this->clickLink('Auto buttons default!');
+    $session_assert->assertWaitOnAjaxRequest();
+    $session_assert->elementExists('css', '.ui-dialog-buttonpane .ui-dialog-buttonset .js-form-submit');
+
+    // When the drupalAutoButtons option is false, buttons SHOULD NOT be moved
+    // into the 'ui-dialog-buttonpane' container.
+    $this->drupalGet('/dialog_renderer-test-links');
+    $this->clickLink('Auto buttons false!');
+    $session_assert->assertWaitOnAjaxRequest();
+    $session_assert->elementExists('css', '.form-actions');
+    $session_assert->elementNotExists('css', '.ui-dialog-buttonpane');
+
+    // When the drupalAutoButtons option is true, buttons SHOULD be moved
+    // into the 'ui-dialog-buttonpane' container.
+    $this->drupalGet('/dialog_renderer-test-links');
+    $this->clickLink('Auto buttons true!');
+    $session_assert->assertWaitOnAjaxRequest();
+    $session_assert->elementExists('css', '.ui-dialog-buttonpane .ui-dialog-buttonset .js-form-submit');

assertWaitOnAjaxRequest to wait on a modal opening risks intermittent test failures. jQuery UI can continue to manipulate the elements after Drupal AJAX is done. For a simple use like this there isn't that much risk.. but given the number of test runs that happen daily it's best to be cautious. If this can instead waitForEementExists on whatever element needs to appear for testing to continue, then it should be fine.

Rerolled on current 11.x and switched out $session_assert->assertWaitOnAjaxRequest() in favour of $this->assertNotNull($session_assert->waitForElement() as suggested in #86.

Let's see if tests still succeed.

No idea, why my patch did not pass tests, created another branch and MR with patch #87 applied here: https://git.drupalcode.org/project/drupal/-/merge_requests/6288

Let's see what the tests from the gitlab-ci are saying.

@Grevil

https://www.drupal.org/project/drupal/issues/3401988

you need to update fork 11.x

@finnsky thanks! The fork doesn't even have 11.x haha, going to update it ASAP.

EDIT: Running again.

All green!

From #86:

It's simple enough that if the person who addresses it is comfortable with the change they can self-switch this back to RTBC.

->Done

Committed 77888cf and pushed to 11.x. Also cherry-picked to 10.2.x as a non-disruptive bug fix. Thanks!