It would be AWESOME if the form actions (buttons) in modal forms were moved to Bootstrap's .modal-footer div. This would make long modal forms much more intuitive (as I often encounter with Panels and Fieldable Panels Panes), as the form actions would always be visible.

CommentFileSizeAuthor
#8 bootstrap-modal-forms-form-actions-2637314-8.patch8.57 KBjoelstein
#7 bootstrap-modal-forms-form-actions-2637314-7.patch5.12 KBjoelstein
#5 bootstrap-modal-forms-form-actions-2637314-5.patch4.58 KBjoelstein
#2 bootstrap-modal-forms-form-actions-2637314-2.patch4.57 KBjoelstein
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelstein created an issue. See original summary.

joelstein’s picture

joelstein CreditAttribution: joelstein commented
Status: Active » Needs review
FileSize
bootstrap-modal-forms-form-actions-2637314-2.patch4.57 KB

Here's a first stab at making this work. It overrides Ctools' modal_display() function to essentially manipulate the modal if the ajax response includes a form. In that case, it will extract .form-actions DIV and move it to a new .modal-footer, and then wrap the modal header, body, and footer inside the original form tag. Anything else included in the ajax response will also be placed in the modal, such as status messages. And if the ajax response doesn't contain a form that we can work with, it unwraps the modal parts and removes the footer (essentially restoring the old behavior).

To make this work without having to touch the overall modal positioning, the patch adds padding to the bottom of .modal-body and negative margin to the top of .modal-footer. The supplementary styling is includes in overrides.less (wasn't sure how to compile this for the base theme).

There's only one potential gotcha: I had to move the #modal-content ID up from .modal-body to .ctools-modal-dialog, so that Ctools would attach the ajax form behaviors. If a site depended on this ID (which it shouldn't), then it might experience problems until the CSS is updated.

Let me know what you think! I've been wanting this for awhile. :)

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Status: Needs review » Needs work 
+++ b/js/modules/ctools/js/modal.js
@@ -116,13 +116,13 @@
-    html += '  <div class="ctools-modal-dialog modal-dialog">';
+    html += '  <div id="modal-content" class="ctools-modal-dialog modal-dialog">';
     html += '    <div class="modal-content">';

Shouldn't this ID be added the div below (where the "modal-content" class already exists?

---

On a side note, I'm fairly busy with #2609316: Port Bootstrap to Drupal 8, so I'm not entirely sure when I'll be able to get to fully reviewing/testing this. Also, I'm a little apprehensive of touching the CTools JS file as we've had many issues with it in the past. If others can get on board with it and can RTBC it, then I'll be more likely to commit.

joelstein’s picture

joelstein CreditAttribution: joelstein commented

Thanks for the review, Mark, and for all your hard work on this theme. :)

Ctools looks for #modal-content form in Drupal.behaviors.ZZCToolsModal, so I had to move the ID up so it would be a parent of the form, allowing Ctools to attach the ajax handlers to the form submit.

It would be better if Ctools changed its selector to #modalContent form, which would support existing functionality while also supporting what we're doing here. But without getting that changed, the patch I submitted will work (with my caveats, of course).

joelstein’s picture

joelstein CreditAttribution: joelstein commented
FileSize
bootstrap-modal-forms-form-actions-2637314-5.patch4.58 KB

Here's an updated patch which adds a z-index to the modal footer, so it appears above input groups, which give their child form elements a z-index.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Status: Needs work » Postponed (maintainer needs more info)

I'm a little cautious about committing this, as is.

If this is cloning elements, what happens if the elements have click behaviors or JS that expect to be in the same form? How is it handling the proper click events?

Also, what is the potential BC breakage of existing sites? Should this just be left as a patch here for those that are seeking this functionality?

joelstein’s picture

joelstein CreditAttribution: joelstein commented
Status: Postponed (maintainer needs more info) » Needs review
FileSize
bootstrap-modal-forms-form-actions-2637314-7.patch5.12 KB

Here's an updated patch which solves an issue we saw in Firefox based on the placement of the footer and Firefox's weird behavior not rendering padding-bottom on elements with overflow. Instead, the modal footer is displayed inline and the modal body's max height is reduced to make room for the footer. Works great and requires no hacky CSS.

I also fixed a typo in the documentation and resolved jshint complaints.

If this is cloning elements, what happens if the elements have click behaviors or JS that expect to be in the same form? How is it handling the proper click events?

I updated the patch to deep clone data and events, but even before I did that, the ajax behaviors attached to the form actions were preserved, so I think we're good here.

Also, what is the potential BC breakage of existing sites? Should this just be left as a patch here for those that are seeking this functionality?

Great question. This patch is working fantastically on dozens of sites for us, and it should work as-is. But perhaps we should add a configurable option within Bootstrap to disable this if folks run into problems?

joelstein’s picture

joelstein CreditAttribution: joelstein commented
FileSize
bootstrap-modal-forms-form-actions-2637314-8.patch8.57 KB

I found another small issue where sometimes the modal wouldn't be positioned correctly after adding the modal footer with the form actions.

This patch moves the modal resizing to it's own function at Drupal.Bootstrap.CtoolsModalResize, and adjusts the modal body's max height depending on the presence of the modal footer.

Works like a charm!

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Status: Needs review » Postponed

@joelstein, considering that you're the only one that has provided this and is using it. I'm very hesitant to move forward on this issue. CTools has been a PITA on several issues over the years and I don't want to risk breaking any existing sites, even if accidentally.

Until this has been reviewed by a few other people who use CTools, I'm going to mark this as postponed for now.

Homotechsual’s picture

Homotechsual CreditAttribution: Homotechsual commented

After looking for this for a while I stumbled upon this issue, unfortunately applying a patch to a contrib module isn't generally something we do for the long-term, so we've settled on a little javascript hack!

jQuery(document).ready(
  function($) {$("#login-modal #edit-actions").prependTo("#login-modal .modal-footer");
});

Small, quick and it works a treat!

Note: We're using the bootstrap_login_modal module for this on this site.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Status: Postponed » Closed (won't fix)

Considering how complicated modals/dialogs are, I'm not comfortable adding this to 7.x-3.x, given how many sites use this base theme.

FWIW, this will be apart of 8.x though #2831237: Bootstrap modal does not work well with jQuery UI dialog.