"Add another item" field button is displayed as a modal action

I'm seeing the same thing with Claro. On a normal node form with two multi-value entity reference fields the "Add Another" buttons get pulled into the modal footer. This is a Claro subtheme in the screenshot but I've experienced the same behavior using Claro directly.

UPDATE: While this does help with some buttons in dialogs it breaks the buttons in Views UI modals in some cases.

This seems to be coming from core dialog.ajax.js

prepareDialogButtons gathers up the buttons to move to the footer using the selectors
'.form-actions input[type=submit], .layout-region-node-actions .form-actions a.button'

This is wide enough to pick up the add more buttons in the multivalue fields.

Right now I'm getting around this by:

• copying core/misc/dialog/dialog.ajax.js to my subtheme js folder
• renaming to mytheme_dialog.ajax.js
• changing the selector used in var $buttons = $dialog.find to something more narrow using a selector from my theme
• using an override

This is the override I'm using in my theme info.yml file:

libraries-override:
  core/drupal.dialog.ajax:
    js:
      misc/dialog/dialog.ajax.js: /themes/custom/mytheme/js/mytheme_dialog.ajax.js

Works well for me on the default node page:

I installed Layout Builder modal and it doesn't have Admin theme option as Claro, only Seven while Claro is set as default admin and Seven is actually uninstalled.

And I can only see issue if I set Claro as front-end default - then the issue is there - but I don't think Claro is intended to be used as non-admin and also not sure that Layout Builder Modal module is intended at the moment to be used with Claro since that wasn't an option for admin themes in modules settings

Oh should probably have mentioned versions:
Drupal 9.0.1
And Layout Builder Modal: 8.x-1.1

The issue summary mentions two separate bugs:

1. The 'Add another item' field button is displayed as a modal action
2. The form submit button is NOT displayed as a modal action

I suggest we fix #1 here, and fix #2 in a follow-up issue.

The above patch fixes: "Add another item" field button is displayed as a modal action
The buttons(s) are not visible as a modal action anymore.

After this change, there isn't sufficient margin between the button and description text.

Steps To Reproduce

1. Go to “home >Administration>Structure>Block layout ”

Note : Enable the “layout ” module under extend

2. Select the “Claro“ theme under “Block Layout”
3. Click “Custom block library” & then click “Blocks” option
4. Click “Add Custom Block” button & add the values under the relevant fields
5. Click “Save” button & “Block Types“ option
6. Now go to created block & Click “Manage Fields” option & add the desired “Fields”
7. Open the created customize block layout & observe the “Add another item ” button

This seems be working fine as expected without applying the patch for Drupal 8.9.x . Please find the attachment for the issue.

In our case, it was the Media Library modal which revealed this problem for us. The patch in #11 worked for us too, and no problems with spacing that I could see.

I can confirm this issue happens Media Library, as described in #13, and the patch from #11 works to fix it.

Thanks everyone for working on this.

This is a bug and the patch in #11 is against 8.9.x, it needs to be for 9.1.x. I've started a test on 9.1.x. The issue summary should include the proposed resolution, adding the standard template would help there. And having before and after screenshots in the issue summary helps any reviewer and the committer.

In #12, janmejaig reports that they were unable to reproduce the issue. That discrepancy should be explained as well.

Does this need a test?

Looks like the patch passes for 9.1.x-dev, so I'm putting back to RTBC. Updated the summary to use the template best I could. Feel free to improve :)

Screenshots were in #11 but didn't really do it justice, so here is a before:

And an after:

As for @janmejaig report, the steps/screenshot don't seem to be relevant as it's not something in a modal, which is all this issue is concerned with, but rather something on a standard form page.

+++ b/core/themes/claro/css/components/form.pcss.css
@@ -97,7 +97,7 @@ tr .form-item,
 .form-item__error-message {
...
-  margin-bottom: calc(6rem / 16); /* 6px */
+  margin-bottom: var(--space-m); /* 16px */

This increases spacing in other use cases than field cardinality. Maybe we could just wrap the button with element that uses other class than form-actions, and we would apply the same styles for that element?

Thanks for this works guys, I reckon we're almost there but I can still see an issue.

While the patch fixes the issue 1 & 2, it doesn't fix the issue 3: the main form actions buttons, like the node/taxonomy "Save"/"Delete", are inconsistent with the original behaviour.

In Seven, they display correctly in the modal footer, while in Claro only the "Save" appears correctly, with "Delete" remaining in the original place.

After a quick inspection I noticed that in the standalone versions of the form (in both admin themes), case A1 & B1, "Save" is a <input> with class="button", while "Delete" is an anchor (<a>) with class="button" in Seven and class="action-link" in Claro.
In Seven's modal (A2) the markup gets re-arranged somewhere and both links become a <button>, but in Claro only the first gets picked up while the second is ignored.

I haven't tested if the class change is enough, but if it is, I wonder what would be the consequences in changing that class for styling and coding standards.

I will try and come with a patch soon.

Edit: I can confirm that the class change does the trick.
claro.theme@366

  // Make entity forms delete link use the action-link component.
  if (isset($form['actions']['delete']['#type']) && $form['actions']['delete']['#type'] === 'link' && !empty($build_info['callback_object']) && $build_info['callback_object'] instanceof EntityForm) {
    $form['actions']['delete'] = _claro_convert_link_to_action_link($form['actions']['delete'], 'trash', 'default', 'danger');
  }

Commenting the snippet above fixes the modal, but obviously changes the style of the button (class="button button--danger"), becoming fully red.

Any suggestion on the course of actions?
Maybe we could change the modifier class in order to have:

• button--icon-trash to add the trash icon
• button--action-link (or similar) in order to simulate the "hollow" link look.

Another solution to the above problem could be leaving the action links untouched and change the function that generates the <button> in the modal footer: instead of selecting the .button class, it could select any input, button, anchor inside the .form-actions wrapper.

#15 The issue summary contains a 'Proposed resolution'. #16 contains before/after screenshot.

#21 is a different bug: buttons that should be in the modal actions are not in the modal actions

The original issue is the inverse problem: a button is included as a modal action, but it should not be.

I suggest filing a separate issue for #21

As recommended by @idebr I opened a different issue for case #21, inspired by @jimmynash #4 comment.

On the current issue: I agree with @laurii in #19, to maintain the wrapper with a dedicated class in order to keep the original padding/margin without altering the component in different contexts.
Any recommendations about the naming of the wrapper class? I would propose .subform-actions or .multiple-value-form-actions.
The class should have the same properties of .form-actions, so we could change the selector like this:

.form-actions,
.multiple-value-form-actions { /* Or the preferred name */
  /* Existing style [...] */
}

+1 for patch #11.
The padding/margin should still be looked at, as stated by #19 and #25.

I'm not a FEFM but in my opinion, the issue here is with Drupal.behaviors.dialog.prepareDialogButtons which is using classes that are intended for styling for attaching JS behaviour (yeah I put a u in behaviour, fight me).

In my opinion a better fix would be to change that code to look for .js-form-actions and update \Drupal\Core\Render\Element\Actions::processActions to add both the .form-actions class for the styling and the .js-form-actions class for the JS functionality.

I don't think that would represent a BC risk, but happy to be wrong.

Tagging for JS to see if JS maintainers agree with me

I need this change for a site, so protoyped the changes that larowlan suggested. It's solved the issue for me using the Claro theme.

Tests? What would need to assert? The position of the "add more" button?

Used patch #34 on 9.3.12 and it solved the problem.

Patch #34 works for me as well for Drupal core 9.3.13. Thanks

We discovered that too and I asked about it on slack (https://drupal.slack.com/archives/C1BMUQ9U6/p1666967652891399), where @larowlan pointed me to this issue.

IMHO, changing the selector here seems problematic, I'm not sure if that's not an API change or not as we can't rely on existing markup using the js class. The Actions class seems like a perfect example for that. I proposed a different fix in slack:

the latest patches are doing a very different thing than the issue summary proposes, this didn't work for paragraphs out of the box. I think a better fix would be to change the current claro class to something else that has the same effect as form-actions instead of changing the class that the existing and generic dialog JS looks for
would love some feedback from claro maintainers on that

@laurii confirmed that:

Yeah I agree that field-multiple-value-form.html.twig looks like a misuse of form-actions . I think we could remove that. However, something like changing the form-actions to js-form-actions could make sense as a follow-up.

He also confirmed that this should be allowed as a change to Claro in a patch(?)/minor release.

We plan to work on a patch for that in the next days.

Edit/Update: I missed #25, which suggested exactly the same and even also discussed with @laurii.

I went back to the comment #25 and implemented that. #34 did not work for us, and #11 had the issue that some css rules depending on the class .form-actions were missing.

Following #25 suggestion, I did duplicate all the css rules for .form-actions, for the new class. I am a bit worried that some or many of them are not actually necessary, but without a reliable way to determine which cases are happening and which are not, I made the decision to apply that everywhere.

Applied #34, which went fine. Didn't have the chance to test #42. Thanks!

Note that this breaks the paragraphs add above feature (again), as we foolishly relied on that form-actions class (to be fair, there isn't really an alternative and every theme does it differently). A patch to fix that is in #3268122: Button "Add above" is missing with Gin theme enabled

Verified and applied patch #42 on Drupal 10.1.x-dev.
The patch applied cleanly.
Followed the steps mentioned in the IS and was able to reproduce the issue.

Result:
The issue got fixed after applying the patch.
The "Add another item" button is now displaying where it belongs (in this case below the 'Text' multi-valued field)

Attaching the screenshots for the reference
Before patch:

After patch:

RTBC +1

Working fine without applying any patch.

Looks good and works fine. (#42)

We still need tests, and it would be good to get a +1 from @lauriii

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I just applied the patch in #42 and the issue still occurs for me using Claro and Gin.

In my case, I had an actions structure within a fieldset in a form. My fix was to just remove the button from that inner actions. Then it corrected itself. Is that expected, or is there a way to prevent that scenario too?

I'm not sure what button that is, but it's not the regular field widget one I think. The fix here is only about the default button in field widgets by changing the class to not use form-actions. form-actions is expected to be the primary action of a form I think and should be used for others.

Yeah that is coming from a form alter (mine) where I had action buttons within a fieldset (also mine). I just removed the actions part, it moved to the right place. But I saw having more than one actions referenced here:

https://git.drupalcode.org/project/examples/-/blob/4.0.x/modules/form_ap...

I suppose one question is, shouldnt 'Add Block' be down in the action area?

Posted into #frontend channel but was previously tagged for tests which still need to happen.

I have been working on a test for this issue. I am uploading the test alone, and the test together with patch #65

phpcs fixes.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroModalDisplayTest.php
   @@ -0,0 +1,93 @@
   +/**
   + * Tests that buttons in modals are not in the footer.
   

   footer is probably not the right term for this, but I'm not exactly sure what is. The class for it is ui-dialog-buttonpan, so maybe "button pane"?

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroModalDisplayTest.php
   @@ -0,0 +1,93 @@
   +  /**
   +   * Tests that uploads in the 'media_library_widget' works as expected.
   +   */
   +  public function testWidgetUpload() {
   

   description and test method hasn't been updated from the copied example. maybe testModalAddAnother()?

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroModalDisplayTest.php
   @@ -0,0 +1,93 @@
   +    // A file needs to be added for the unlimited field to appear in the form.
   +    $this->addMediaFileToField('Add files', $this->container->get('file_system')->realpath($jpg_image->uri));
   +
   +    $selector = '.js-media-library-add-form-added-media';
   +    $this->assertJsCondition('jQuery("' . $selector . '").is(":focus")');
   

   maybe add a comment to the assert, that we wait for the upload to finish and refer where it's copied from?

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroModalDisplayTest.php
   @@ -0,0 +1,93 @@
   +
   +    // Assert that the 'add another item' button is not in the dialog footer.
   +    $assert_session->elementNotExists('css', '.ui-dialog-buttonset .field-add-more-submit');
   

   can you add a positive assert that it is where we expect it to be?

   Only having not asserts means that if for example the class changes, it will still pass but no longer test what we want it to. So add a very similar elementExists() and then that will fail and we have a reminder to update.

5. +++ b/core/themes/claro/css/components/action-link.css
   @@ -105,7 +105,8 @@
    
   -.form-actions .action-link {
   +.form-actions .action-link,
   +.multiple-value-form-actions .action-link {
      margin-inline: 0 var(--space-s);
    }
   

   I have no idea if we really need to change all places, but it is probably the safest option.

Thanks @berdir, I addressed all your comments.

phpcs fix

+++ b/core/themes/claro/templates/form/field-multiple-value-form.html.twig
@@ -42,7 +42,7 @@
+      <div class="multiple-value-form-actions">{{ button }}</div>

I think it would be nice to name this a bit more generically.. Maybe something like field-actions could work?

Updated the class as per #72, attached interdiff for same

Removing "Needs frontend framework manager review" tag because I don't think this actually needs a framework manager sign-off and I have reviewed this as a maintainer of Claro. Also removing "Needs tests" because there are tests in the patch, and test only patch in #67.

Solution seems simple enough 👍 Removing @nest from action-link CSS is a bit of scope creep but seems at least fine by me. Claro is internal so we could probably backport this to 10.1.x as a non-disruptive bug fix.

The patch in #74 does not apply. Can we have a reroll?

Simple reroll with a conflict in core/themes/claro/css/components/form.pcss.css.

Sorry for the noise.. I broke HEAD in other issue... 🤦‍♂️

Attached is patch 77 rolled for 10.1.x

Updated patches based on #77:
- 11.x: re-rolled for latest changes to 11.x
- 10.1.x: code cleanup

Looks like another random fail.