Whenever you end up with the window smaller than the modal, the modal has a tendency of hiding its top part. Usually the close button is there so it makes the situation rather unclear to the end user.
I suggest using Math.max() so the top of the modal remains always visible.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mojzis’s picture

Simple patch with a fixed value. Could be probably provided as an option in settings.

mojzis’s picture

Status: Active » Needs review
dooglet’s picture

Version: 7.x-1.x-dev » 7.x-1.2

I've been working with ctools modal recently to create a user registration form. The form has quite a lot of fields however and I've been running into the problem described above.

Spent a bit of time working on this and come up with a poor solution i'm not particularly proud of but the core of the issue here doesn't seem to be related to the values obtained for the wt variable (which the patch above addresses).

Rather, by looking through the modal.js file and with judicious use of console logging I found that the css values for the top attribute of the div with id modalContent are different before and after the call to Drupal.attachBehaviors();

  /**
   * AJAX responder command to place HTML within the modal.
   */
  Drupal.CTools.Modal.modal_display = function(ajax, response, status) {
    if ($('#modalContent').length == 0) {
      Drupal.CTools.Modal.show(Drupal.CTools.Modal.getSettings(ajax.element));
    }
    $('#modal-title').html(response.title);
    // Simulate an actual page load by scrolling to the top after adding the
    // content. This is helpful for allowing users to see error messages at the
    // top of a form, etc.
    $('#modal-content').html(response.output).scrollTop(0);
    Drupal.attachBehaviors();

    // hack
    if(parseFloat($('#modalContent').css('top')) < 50) {
      $('#modalContent').css('top','50px');
    }

  }

So, to TL;DR it:

$('#modalContent').css('top') has a normal value (~240px in my case) before the call to Drupal.attachBehaviors() at which point my huge form knocks it to -307px and the top of the form is offscreen at the top of the window. My guess is there's some positional code going on somewhere deeper but I've no idea where. Yet. To the mystery machine!

litzastark’s picture

Issue summary: View changes

Just wanted to note that the code in #3 worked great for me on a D6 site! Thanks for posting, dooglet.

tlilleberg’s picture

In d7 (possibly others), you can use the fix in #3 without hacking the module itself.

Simply create a js file with these contents: (example file is myModule/js/myModule-modal.js

// myModule/js/myModule-modal.js
$(document).ajaxComplete(function() {
    if(parseFloat($('#modalContent').css('top')) < 50) {
      $('#modalContent').css('top','50px');
    }
});

And attach it in your form array:

function myModule_form($form, &$form_state) {
  $form = array(
    //The usual stuff
  )
  $form['#attached']['js'] = array(ctools_attach_js('myModule', 'myModule-modal', 'js'));
}

You can probably just add the file with ctools_add_js in the callback function itself, though that I haven't tested.

One downside to this is that the code will run on any ajaxRequest, not just the modal form finishing, but I think it's a good trade-off for leaving the ctools code alone.

-- Travis Lilleberg
Digital Loom
www.digital-loom.com

mojzis’s picture

@dooglet, sorry i didn't notice your input sooner ... just a short explanation: the patch doesn't address the wt variable, but the mdcTop variable, which in turn is used later on to set the css top - the same thing you are addressing :) While its elegant to solve the problem without changing the original code, i still have a feeling that my patch does slightly improve ctools :).

DamienMcKenna’s picture

Version: 7.x-1.2 » 7.x-1.x-dev

Status: Needs review » Needs work

The last submitted patch, 1: modal_max_for_top-1678562.patch, failed testing.

mojzis’s picture

reroll ;)

mojzis’s picture

Status: Needs work » Active
mojzis’s picture

Status: Active » Needs work

trigger test ...

AlexKirienko’s picture

Status: Needs work » Needs review
FileSize
1.37 KB

I have experienced same problem with big modals.

In ctools 1.4 I have fixed it with

if (mdcTop < 0) { 
  mdcTop = 0; 
}

I have placed fixed Drupal.CTools.Modal.modalContent in my theme JS.

But this is bug. And it will be better for all of us to fix it in ctools.

Thank you for patch, mojzis. But it works only for modals opened when window located at the top of the page. If you scroll page down and open modal - modal will be located at the top of the page, not top of window.

This happens cos you include wt in Math.max. wt - represent window position on page and should be outside of Math.max.

Also I think that big modal windows must be positioned at the top of window. There is no top 20 px needed.
Different themes can have their own top-padding for modal window. For example bootstrap have its own top-padding which will be different in different conditions. We using bootstrap on project and it's critical for us.

Please check fixed patch.

Chris Matthews’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The 4 year old patch in #13 to modal.js does not apply to the latest ctools 7.x-1.x-dev and if still applicable needs to be re-rolled.

Checking patch js/modal.js...
error: while searching for:

    // Create our content div, get the dimensions, and hide it
    var modalContent = $('#modalContent').css('top','-1000px');
    var mdcTop = wt + ( winHeight / 2 ) - (  modalContent.outerHeight() / 2);
    var mdcLeft = ( winWidth / 2 ) - ( modalContent.outerWidth() / 2);
    $('#modalBackdrop').css(css).css('top', 0).css('height', docHeight + 'px').css('width', docWidth + 'px').show();
    modalContent.css({top: mdcTop + 'px', left: mdcLeft + 'px'}).hide()[animation](speed);

error: patch failed: js/modal.js:438
error: js/modal.js: patch does not apply
adr_p’s picture

Chris Matthews’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

  • joelpittet committed 8ca1f9b on 7.x-1.x authored by mojzis
    Issue #1678562 by mojzis, AlexKirienko, adr_p, Chris Matthews, dooglet,...
joelpittet’s picture

Status: Needs review » Fixed

Thanks the latest fix looks reasonable I've committed it to the 1.x dev branch.

DamienMcKenna’s picture

Status: Fixed » Closed (fixed)

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