Problem/Motivation

As part of the Views in Drupal Core initiative (VDC) this issue is to track abstracting the Views modal widget from views ui module into core. This is identified as one of the early dependencies of the VDC initiative.

Proposed resolution

Abstract the Views Modal widget/pattern from views ui module and add it to core.

Remaining tasks

Re-roll as per @quicksketch's feedback

User interface changes

Delete forms for nodes from links at admin/content/node display in modals for users with JavaScript
Delete forms for forum containers and forums display in modals for users with JavaScript

API changes

Modules wishing to display form content in modals should change their page callback to ajax_modal_form instead of drupal_get_form.
Note that the new routing system has a proposal to do format based routing so this will become more intuitive
If modules wish to trigger these callbacks via links, they should add 'modal' => TRUE to the options passed to l() when generating the link.
Modules wishing to display modal forms using the ajax api should attach an ajax behavior to their button and attach the modal library

<?php
  $form
['actions']['delete'] = array(
     
'#type' => 'submit',
     
'#value' => t('Delete'),
     
'#attached' => array(
       
// Attach the modal library.
       
'library' => array(array('system', 'drupal.ajax.modal'))
      ),
     
'#ajax' => array(
       
// Declare the ajax callback.
       
'callback' => 'forum_confirm_delete_ajax'
     
)
    );
// ..
/**
 * Ajax callback for deleting a forum.
 *
 * Calls ajax_render_modal_form passing the $form_id and the form arguments.
 */
function forum_confirm_delete_ajax($form, &$form_state) {
  return
ajax_render_modal_form('forum_confirm_delete', array(
   
$form_state['values']['tid']
  ));
}.
?>
Files: 
CommentFileSizeAuthor
#125 Diff dialog.png215.38 KBDean Reilly
#110 core-js-modal-1667742-110.patch16.56 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 48,749 pass(es).
[ View ]
#110 interdiff.txt4.6 KBeffulgentsia
#108 core-js-modal-1667742-108.patch13.38 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 48,748 pass(es).
[ View ]
#108 interdiff.txt1.23 KBjessebeach
#108 core-js-modal-1667742-node-delete-modal-example-do-not-test.txt1.81 KBjessebeach
#104 core-js-modal-1667742-104.patch13.71 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 48,745 pass(es).
[ View ]
#104 interdiff.txt1010 byteseffulgentsia
#101 core-js-modal-1667742-101.patch13.59 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 48,733 pass(es).
[ View ]
#101 interdiff.txt10.22 KBeffulgentsia
#97 core-js-modal-1667742-97.patch12.03 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 48,734 pass(es).
[ View ]
#95 core-js-modal-1667742-95.patch12.1 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 48,758 pass(es).
[ View ]
#89 core-js-modal-1667742-89.patch11.48 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 48,218 pass(es).
[ View ]
#89 interdiff.txt606 byteseffulgentsia
#88 core-js-modal-1667742-88.patch11.33 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 48,222 pass(es), 0 fail(s), and 4,019 exception(s).
[ View ]
#83 modal-83.patch11.61 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 48,231 pass(es), 0 fail(s), and 4,015 exception(s).
[ View ]
#68 views-modal-in-core-1667742.68.patch18.04 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 48,112 pass(es).
[ View ]
#68 views-modal-in-core-1667742.68.interdiff.txt7.06 KBlarowlan
#62 views-modal-in-core-1667742.62.patch15.53 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 48,082 pass(es).
[ View ]
#62 views-modal-in-core-1667742.60.interdiff.txt1.82 KBlarowlan
#60 views-modal-in-core-1667742.60.interdiff.txt1.82 KBlarowlan
#60 views-modal-in-core-1667742.60.patch19.85 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 48,065 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#57 views-modal-in-core-1667742.57.patch15.56 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 48,061 pass(es), 0 fail(s), and 7 exception(s).
[ View ]
#57 views-modal-in-core-1667742.57.interdiff.txt8.1 KBlarowlan
#47 views-modal-in-core-1667742.47.patch19.02 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 47,888 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#45 views-modal-in-core-1667742.45.interdiff.txt6.84 KBlarowlan
#45 views-modal-in-core-1667742.45.patch17.27 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 47,877 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
#44 core-js-modal-1667742-44.patch13.15 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 47,844 pass(es).
[ View ]
#40 core-js-modal-1667742-40.patch11.84 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-modal-1667742-40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#38 views-modal-in-core-1667742-38.patch16.91 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 47,587 pass(es).
[ View ]
#20 Screen Shot 2012-09-20 at 00.51.43.png17.84 KBswentel
#19 views-modal-in-core-1667742-19.interdiff.txt1013 byteslarowlan
#19 views-modal-in-core-1667742-19.patch16.95 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 41,335 pass(es).
[ View ]
#17 views-modal-in-core-1667742-17.patch16.94 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 41,331 pass(es).
[ View ]
#8 views-modal-in-core-1667742-8.patch16.79 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 37,052 pass(es), 51 fail(s), and 26 exception(s).
[ View ]

Comments

larowlan’s picture

Issue tags:+VDC

Adding tags

nod_’s picture

See what's currently missing from ctools modal #1397370: Make modal.js use jQuery dialog it's not really useful to views but it it for the modal functionality.

The abstracted part in the issue is important. Modal is not a good thing on mobile, we need to be able to change the way it work on the fly meaning we can't go blindly with jQuery UI.

(edit) Also there are two parts to this issue. The actual abstract modal API and the ajax commands. You might want to split one or the other in a follow-up issue.

nod_’s picture

larowlan’s picture

Also:
http://bugs.jqueryui.com/ticket/7861
http://bugs.jqueryui.com/ticket/7862

If this is going to fly, need to get these resolved upstream to get through the a11y gate

And:
#617730: UX: Use modal dialogs for confirmation forms.

Tagging for mobile because we need fallback on smaller devices.

nod_’s picture

Well scott gonzalez from the jquery team assured us our blocking accessibility issues will be fixed for 1.9.

larowlan’s picture

See: https://github.com/jquery/jquery-ui/pull/684, fixes tabbing issues with jquery UI Dialogs

larowlan’s picture

larowlan’s picture

Status:Active» Needs review
StatusFileSize
new16.79 KB
FAILED: [[SimpleTest]]: [MySQL] 37,052 pass(es), 51 fail(s), and 26 exception(s).
[ View ]

Patch for discussion

Status:Needs review» Needs work

The last submitted patch, views-modal-in-core-1667742-8.patch, failed testing.

effulgentsia’s picture

Issue tags:+d8ux

Perhaps it's premature, but at some point (later than now, but likely earlier than all of Views being committed to core), it will be helpful to have a more complete use case in core than a simple confirmation dialog. In #1175830-6: Update to jQuery UI 1.10.2, yched suggests Field formatter settings as a good candidate. There's also #1101600: Users need to be able to select from list when adding menu items to a menu, though some later comments in that issue argue against a modal for that. Adding d8ux tag for usability feedback for what would be an accepted use case.

tim.plunkett’s picture

Issue tags:+CTools dependency

Tagging.

Bojhan’s picture

Why don't we also move the overlay to this?

tim.plunkett’s picture

As of now, this works while in an overlay. Not sure how Inception-y we want to get.

Bojhan’s picture

@tim.plunkett I mean, the technology. The visual styling obviously has to be different, but given that this modal is accessible and overlay isn't we shouldn't have two similar technologies in core.

nod_’s picture

See #1175830-79: Update to jQuery UI 1.10.2 html5 is working on something, we should keep an eye on it.

tim.plunkett’s picture

Issue tags:-CTools dependency

This doesn't actually have any CTools dependencies.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new16.94 KB
PASSED: [[SimpleTest]]: [MySQL] 41,331 pass(es).
[ View ]

This attempt makes use of the request object's content-negotiation and does away with the need for a change to the menu callback paths.
Patch includes change to make the 'delete' links on admin/content use a modal.
With this patch, making a form display in a modal requires:
You change the menu callback from drupal_get_form to ajax_modal_form.
You change the calling link to add $options['modal'] = TRUE when calling l().

yched’s picture

Status:Needs review» Needs work

The rest of the code is way over my head, but :

+++ b/core/modules/system/system.moduleundefined
@@ -3340,6 +3340,11 @@ function confirm_form($form, $question, $path, $description = NULL, $yes = NULL,
+    '#attributes' => array(
+      'class' => array(
+        'dismiss'
+      )

This looks a little too common for a class name that's supposed to trigger some specific modal-related behavior. Could we namespace/specialize that a bit ?

6 days to next Drupal core point release.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new16.95 KB
PASSED: [[SimpleTest]]: [MySQL] 41,335 pass(es).
[ View ]
new1013 bytes

Fixes suggestion from #18.

swentel’s picture

StatusFileSize
new17.84 KB

Gave it a quick spin, looks nice overall. The only weird thing is that there's a border around the delete button.
Chrome Version 21.0.1180.89

Screen Shot 2012-09-20 at 00.51.43.png

- edit - seems like it's some sort of autofocus on it - clicking somewhere removes that.

larowlan’s picture

The border is because the delete button gets the focus, making this an accessible modal. Yay for jquery.ui.

nod_’s picture

Status:Needs review» Needs work

Nice start. The JS needs some work. Lots of JSHint issues to solve and a few other things that are not following current core practice. Like the $('selector', context) that should be $(context).find('selector'). Missing a docbloc for the dismissModal JS command.

There seems to be a lot of JS considering we're using jquery ui to do it for us. Care to have a look at #1397370: Make modal.js use jQuery dialog and see if there is something that can be reused from there?

Also it kinda doesn't work well with overlay, probably an easy fix.

effulgentsia’s picture

Priority:Normal» Major

This is required for VDC, so raising to major.

tsvenson’s picture

As pointed out in #10 by @effulgentsia:

Perhaps it's premature, but at some point (later than now, but likely earlier than all of Views being committed to core), it will be helpful to have a more complete use case in core than a simple confirmation dialog.

I'm also curious to learn how simple/advanced this one is planned to be. The modal Page manager uses, which comes from CTools I believe does much more and would be so much more useful to have in core.

Is there plans to allow the modal to contain more complex forms, AJAX and multistep wizards even? Or, is that better suited for a more advanced variant...

I can think of tons of use cases all over core where this would vastly improve usability. Such as adding/editing fields in the Field UI in a modal instead of replacing the field list tab as now is the case.

Sorry if this is a bit off-topic, but I feel it is at least to some extent relevant.

larowlan’s picture

Status:Needs work» Needs review

Can I get a review of the php/form api stuff too?
Will re-roll once for both js/php.

seutje’s picture

+++ b/core/misc/ajax.jsundefined
@@ -465,6 +465,101 @@ Drupal.ajax.prototype.error = function (response, uri) {
+
+  var maxWidth = parseInt($(window).width() * .85); // 85% of window
+  var minWidth = parseInt($(window).width() * .6); // 60% of window
...
+  // Don't let the window get more than 80% of the display high.
+  var maxHeight = parseInt($(window).height() * .8);
+  var minHeight = 200;
...
+
+  scrollHeight += parseInt($scroll.css('padding-top'));
+  scrollHeight += parseInt($scroll.css('padding-bottom'));
...
+  difference += parseInt($scroll.css('padding-top'));
+  difference += parseInt($scroll.css('padding-bottom'));
+  // @todo is any of this relevant?

Always provide a radix for parseInt.

And I don't think those padding values could possibly change in between those 2 calls, so it'd prolly be nice to cache the return value of those css() calls
Other stuff, like === vs == should be flagged by JSHint.

effulgentsia’s picture

Sorry I haven't gotten around to reviewing this yet. I plan to soon. Reviews from others, of course, are still very much welcome.

webchick’s picture

Issue tags:+Spark

Tentatively tagging Spark. We might need this for the layout/block UI.

User Advocate’s picture

@webchick – glad to see you add that connection to Spark. I think a core modal form capability can be very relevant to usage contexts such as the ones that Spark is addressing. In fact it can potentially have a profound change on the UX for all of Drupal. It’s worth taking some time to consider the dimensions of this so we don’t miss the opportunity.

Bringing a modal dialog capability into core is consistent with the general shift in UX strategy we’re going through for D8 - and that reflects a change going on across the entire web. That change comes from an increase in user expectations for web applications. That is to say, web apps are more and more going to have to provide rich user experiences that were previously only possible in desktop applications. In that regard, this simple confirmation modal dialog is the tip of a very big iceberg.

@effulgentsia and @tsvenson indicated that it would be useful to consider more use cases. I think that’s a good idea but I want to mention that doing so could be a long process. To begin that process I want to just go over a few use cases that apply to even a simple confirmation dialog and share a few key concepts that come from UI design standards for desktop applications.

A confirmation dialog should be based on a base class (or equivalent) that can be configured for different purposes. In the Windows API (which I’m most familiar with) this is done using a ‘Message Box’ window. When one is created, the specific type of dialog can be controlled by flags passed into the create function. These flags will create the UI buttons that the given dialog type requires to carry out its specific use cases. These bitwise flags of course are defined as semantic constants. Here are some examples taken from the Win API:

  • MB_ABORTRETRYIGNORE - The message box contains three push buttons: Abort, Retry, and Ignore.
  • MB_CANCELTRYCONTINUE -The message box contains three push buttons: Cancel, Try Again, Continue. Use this message box type instead of MB_ABORTRETRYIGNORE.
  • MB_HELP - Adds a Help button to the message box. When the user clicks the Help button or presses F1, the system sends a WM_HELP message to the owner.
  • MB_OK - The message box contains one push button: OK. This is the default.
  • MB_OKCANCEL - The message box contains two push buttons: OK and Cancel.
  • MB_RETRYCANCEL - The message box contains two push buttons: Retry and Cancel.
  • MB_YESNO - The message box contains two push buttons: Yes and No.
  • MB_YESNOCANCEL - The message box contains three push buttons: Yes, No, and Cancel.

So it’s all about being able to control from a high level what the meaning of the dialog box should be for a given use case.

There are other aspects to consider as well, such as the proper use of the ‘caption bar’ titles. In the example shown in comment #20, this area contains the primary prompt which asks the question “Are you sure you want to delete testing?” The Windows (and other OS) convention is to use this space to identify something higher level through a title such as ‘Confirmation Required’ or ‘Warning’ etc. Keep in mind also that many users will not be expecting to find the primary prompt in that location.

There are other factors to consider such as the optional inclusion of a graphic component (e.g. an exclamation mark for warning dialogs). These can also be determined by flags or perhaps optional arguments to the builder function. Use of these icons can help the user rapidly understand what task they are to carry out in the modal.

More significant in the evolution of the Drupal user experience are the use cases where more complex tasks can be carried out through a modal form. There are three key UX benefits from this strategy:

  1. by definition, the modal dialog allows (forces) the user to temporarily focus on a specific task before proceeding
  2. offloading these subtasks to the temporary real estate of the modal allows the main screen to be simpler
  3. the overall usage context (the base screen) is preserved thus significantly enhancing the user’s ability to stay oriented in the workflow

If we could achieve these benefits throughout Drupal we would be light years ahead of the current user experience. For now, it is probably practical to shoot for a fully functional confirmation dialog API. In the next round maybe we can tackle the more complex sub tasks.

Jelle_S’s picture

+++ b/core/includes/ajax.incundefined
@@ -1140,3 +1172,205 @@ function ajax_command_add_css($styles) {
+  // Some forms may have the title built in, if so set the title here (for non
+  // javascript users):

Should be a '.' at the end, I think.

+++ b/core/includes/ajax.incundefined
@@ -1140,3 +1172,205 @@ function ajax_command_add_css($styles) {
+    // @todo document hook

Todo

+++ b/core/includes/ajax.incundefined
@@ -1140,3 +1172,205 @@ function ajax_command_add_css($styles) {
+    // @todo document hook

Todo

+++ b/core/includes/ajax.incundefined
@@ -1140,3 +1172,205 @@ function ajax_command_add_css($styles) {
+  // Non javascript invocation - degrade to standard drupal_build_form
+  // behaviour. Forms can have the title built in, if so set the title here:

Should be a '.' at the end, I think.

+++ b/core/includes/ajax.incundefined
@@ -1140,3 +1172,205 @@ function ajax_command_add_css($styles) {
+ * @return mixed
+ *  If request Content-Type is ajax, returns array of ajax commands, otherwise
+ *  returns markup from drupal_get_form()

Punctuation

+++ b/core/includes/ajax.incundefined
@@ -1140,3 +1172,205 @@ function ajax_command_add_css($styles) {
+ * @see l().

No '.' at the end of @see statements.

+++ b/core/includes/ajax.incundefined
@@ -1140,3 +1172,205 @@ function ajax_command_add_css($styles) {
+  return ($type  == 'ajax') ? array('#type' => 'ajax', '#commands' => $output) : $output;

Double space between $type and ==.

+++ b/core/includes/ajax.incundefined
@@ -1140,3 +1172,205 @@ function ajax_command_add_css($styles) {
+    drupal_add_js('core/misc/ajax.js');

use drupal_add_library('system', 'drupal.ajax');

+++ b/core/includes/ajax.incundefined
@@ -1140,3 +1172,205 @@ function ajax_command_add_css($styles) {
+/**
+ * Utililty to add libraries and JavaScript required for ajax modal forms.
+ *
+ * Adds jquery.ui.dialog, jquery.form, ajax.js and required settings to the
+ * page.
+*/
+function ajax_modal_form_prepare() {
+  $added = drupal_static(__FUNCTION__);
+  if (!$added) {
+    drupal_add_js('core/misc/ajax.js');
+    drupal_add_library('system', 'jquery.ui.dialog');
+    drupal_add_library('system', 'jquery.form');
+    drupal_add_js(array(
+      'ajaxForm' => array(
+        'popup' => 'ajax-modal-popup',
+        'body' => 'ajax-modal-body'
+      )
+    ), 'setting');
+    $added = TRUE;
+  }

Also, this feels strange to mee. Looks like this could be a library (defined in hook_library_info) on its own.

+++ b/core/includes/common.incundefined
@@ -2300,6 +2303,14 @@ function l($text, $path, array $options = array()) {
+      ajax_modal_form_prepare();

Same here, if it's a library, you can call drupal_add_library('system', 'drupal.modal'); or something similar.

+++ b/core/misc/ajax.jsundefined
@@ -465,6 +465,101 @@ Drupal.ajax.prototype.error = function (response, uri) {
+  // @todo is any of this relevant?
+  /*difference += $('.views-override').outerHeight(true);
+  difference += $('.views-messages').outerHeight(true);
+  difference += $('#views-ajax-title').outerHeight(true);
+  difference += $('.views-add-form-selected').outerHeight(true);
+  difference += $('.form-buttons', $modal).outerHeight(true);*/

Todo & commented out code.

+++ b/core/modules/system/system.moduleundefined
@@ -3340,6 +3340,11 @@ function confirm_form($form, $question, $path, $description = NULL, $yes = NULL,
+    '#attributes' => array(
+      'class' => array(
+        'modal-dismiss'
+      )
+    ),

Not 100% sure if it's a coding standard or personal preference... I'd have to check. But I always put a comma after every element in an array (including the last one).

Edit: seems like it is a coding standard after all.

Sorry for nitpicking :-)

Jelle_S’s picture

Status:Needs review» Needs work

Forgot the status.

MustangGB’s picture

#5: Just noting that jquery ui 1.9 has landed, but the accessibly fixes were not included.

scott.gonzalez’s picture

Just noting that jquery ui 1.9 has landed, but the accessibly fixes were not included.

Correct; #5 was either a miscommunication or a typo. After talking to nod_, I asked my team if they were ok with splitting the 1.10 release into 1.10 and 1.11 specifically so that dialog would be released in time for Drupal 8. That day we shuffled stuff around and came up with our new roadmap. Jörn Zaefferer, the other development lead for jQuery UI, is actually at my house right now (visiting from Germany) and we'll be focusing on dialog all week. The goal is to have a beta with all of the dialog changes available before your feature freeze and a stable release before your code freeze.

mgifford’s picture

Thanks Scott. This is encouraging.

larowlan’s picture

hoping to get back to this once I've finished with #731724: Convert comment settings into a field to make them work with CMI and non-node entities which is getting close...

dasjo’s picture

i'd like to link this video of the newly created Drupal SiteBuilding Usability initiative:

Comprehensive Modal Dialogs System in Drupal
video link: http://youtu.be/lOXiqYu50pE
announcement link: http://groups.drupal.org/sbui-announcement

larowlan’s picture

We have been asked to review http://view.jqueryui.com/dialog/tests/visual/dialog/form.html by the jquery.ui team they've continued to work on a11y issues and have https://github.com/jquery/jquery-ui/pull/794/files ready to go - just needs testers.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new16.91 KB
PASSED: [[SimpleTest]]: [MySQL] 47,587 pass(es).
[ View ]

Re-roll against latest head, just seeing how tests go, no interdiff - only merges from 8.x branch

nod_’s picture

Status:Needs review» Needs work

At BADCamp working on it for a couple hours.

First, it works, awesome :D

But it's too mushed together. We need to have :

  1. a API to bring up a "modal" as a standalone thing,
  2. A way to put forms in modal to have any use for them in Drupal.

The actual JS looks very much like the ctools code, and it's not really good, jQuery ui has a position utility that could help us out here.

I'll work on it for a while and send an updated patch.

nod_’s picture

StatusFileSize
new11.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-modal-1667742-40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Still a WIP but to show a simpler approach for this.

There is no setForm/dismissForm thing anymore. There is just 2 commands to open and close a modal. The content of the modal is handled by the existing AJAX insert command (since that's what we want it to do).

The replacement of the callback in node.module makes me think we'd want to change the drupal_get_form method to make it possible to have any form in a modal (or at least all form will give you back ajax commands).

A few things might be missing from the previous patch, let me know what and I'll be happy to put it back.

Haven't looked to much into the PHP, might need some simplification. Since I changed the way it works there are some naming inconsistencies introduced.

larowlan’s picture

Status:Needs work» Needs review

go bot go!

Status:Needs review» Needs work

The last submitted patch, core-js-modal-1667742-40.patch, failed testing.

larowlan’s picture

nod_ patch doesn't apply, missing file core/misc/ajax.modal.js - please ping me on irc so we can coordinate efforts, I've a branch in my sandbox for this and can give you commit access. I'm working on porting the views code in views/include/ajax.inc to the generic api.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new13.15 KB
PASSED: [[SimpleTest]]: [MySQL] 47,844 pass(es).
[ View ]

Updated. Needs the patch in #1833678: Ajax is messing with overlay to work properly with the overlay.

larowlan’s picture

StatusFileSize
new17.27 KB
FAILED: [[SimpleTest]]: [MySQL] 47,877 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
new6.84 KB

This one adds image style effects using modals.
Screencast here: http://www.youtube.com/watch?v=VySgJjlkutU&feature=youtu.be

quicksketch’s picture

OoooOOOo. Nice @larowlan. I like how the use of modals in image styles actually increases consistency. You're right that some actions (i.e. desaturate) do not have settings. The use of modals to add other actions without a redirect is a nice pairing that keeps the user on the page all the time.

That said, maybe integrations like this should be done a-la-carte in separate issues? The image action modal could be cleaned up by styling the interior forms to take up less vertical space (thus fitting on the screen more easily). Considering modals are using iframes and this viewport is passed forward to the child page, we should even be able to add CSS media queries to make the interior forms have more appropriate layouts when the modal is displayed at different sizes, such as on a mobile device.

Similar to that, it looks like the modal pushes down the page but the underlay doesn't stretch with it. Could we make sure that overlay scrolls with the page, or (probably better) position the modal in such a way that it will use a higher position on the screen if needed? Ideally, the modal would attempt to use more vertical space if it were available, up to a certain limit. For example if you're on a mobile device, the modal would likely fill the whole screen. On screens larger than 768 wide (or thereabouts), I'd expect the modal to be centered to fit, up to 80% of the page height, then scroll the modal (not the window) when needed, a la Views. Then the underlayment isn't a problem because you're never scrolling the page.

Accessibility-wise, last I heard is that we're okay with putting in modals now and working under the assumption that jQuery UI 1.10 will fix the remaining issues before D8 is released. Is that still the case that we can push this forward and assume accessibility will be worked out before release?

Great work so far! Generic modals open a huge door of possibility for us.

larowlan’s picture

StatusFileSize
new19.02 KB
FAILED: [[SimpleTest]]: [MySQL] 47,888 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

and I fooed up with a dsm() in the patch, same as 45 less that.
@quicksketch the plan is to get this in then decide on places where it should be used.

tstoeckler’s picture

Awesome!!!

scott.gonzalez’s picture

it looks like the modal pushes down the page but the underlay doesn't stretch with it

jQuery UI 1.10 will use position: fixed; height: 100%; width: 100% for the overlay, so whatever is causing that issue should no longer be a problem when you upgrade. Hooray for no longer supporting IE6.

quicksketch’s picture

Considering modals are using iframes and this viewport is passed forward to the child page, we should even be able to add CSS media queries to make the interior forms have more appropriate layouts when the modal is displayed at different sizes, such as on a mobile device.

I'm an idiot. These aren't iFrames at all (at least in current implementations). Crazy! I'll try actual code reviews before I post more feedback.

quicksketch’s picture

.

Status:Needs review» Needs work

The last submitted patch, views-modal-in-core-1667742.47.patch, failed testing.

sun’s picture

Glad to see progress here.

+++ b/core/includes/ajax.inc
@@ -1140,3 +1172,183 @@ function ajax_command_add_css($styles) {
+function ajax_form_wrapper($form_id, &$form_state) {

I think this should be renamed to ajax_render_modal_form().

+++ b/core/includes/ajax.inc
@@ -1140,3 +1172,183 @@ function ajax_command_add_css($styles) {
+  if (!empty($form_state['ajax']) && (empty($form_state['executed']) || !empty($form_state['rerender']))) {
...
+  elseif ($form_state['submitted'] && empty($form_state['rerender'])) {

It's not clear to me why the first condition checks for 'executed' but the second checks for 'submitted'. Normally, all code external to Form API should check for 'executed' only.

+++ b/core/includes/ajax.inc
@@ -1140,3 +1172,183 @@ function ajax_command_add_css($styles) {
+    if ($messages = theme(array('status_messages', 'status_messages__ajax', 'status_messages__ajax__'. $form_id))) {

Just the last value without an array performs exactly the same thing in a much simpler way. :)

+++ b/core/includes/ajax.inc
@@ -1140,3 +1172,183 @@ function ajax_command_add_css($styles) {
+ *   $items['node/%node/delete'] = array(
+ *    'title' => 'Delete',
+ *    'page callback' => 'ajax_modal_form',
+ *    'page arguments' => array('node_delete_confirm', 1),

I'm relatively sure that this is not the right approach. I'll have to dive a bit deeper into the code to verify this.

+++ b/core/modules/image/image.admin.inc
@@ -181,11 +192,44 @@ function image_style_form_add_submit($form, &$form_state) {
+    // Set sensible defaults if they don't already exist.
+    $form_state = array(
+      'rerender' => FALSE,
+      'ajax' => TRUE,
+      'no_redirect' => TRUE,
+      'build_info' => array(
+        'args' => array(
+          $style,
+          $effect
+        )
+      )
+    );

This somewhat confirms my suspicion — most of the properties being specified here should be considered internal to Form API.

Bojhan’s picture

I would like us to separate the technical implementation from where we apply this pattern. As suggested by quicksketch, and like we did with dropbutton. Confirmation forms, should be the primary place where we do this.

I noticed most of the styling is still pretty off, something that needs work but could also be split off.

larowlan’s picture

I think this should be renamed to ajax_render_modal_form().

Sounds good

It's not clear to me why the first condition checks for 'executed' but the second checks for 'submitted'. Normally, all code external to Form API should check for 'executed' only.

This is largely borrowed from views, might need to ask @timplunkett et al about why they did this

Just the last value without an array performs exactly the same thing in a much simpler way. :)

Yes, but there is some specific stuff views needs here which I've removed from the generic implementation, the plan being to port views to use this instead of it's own - so it will need these additional suggestions here to know when to react.

I'm relatively sure that this is not the right approach. I'll have to dive a bit deeper into the code to verify this.
This somewhat confirms my suspicion — most of the properties being specified here should be considered internal to Form API.

Happy to know if this can be handled cleaner so we don't need something similar for every implementation. Basically the code as it stands is an ajax equivalent of drupal_get_form, perhaps we need another util function in the middle.

@bohjan, so I'm assuming you want to see the patch at #44 with sun's suggestions from #53 and a meta for decide where to use modals with the image formatter code attached with a .do-not-test so others can see how to implement it?

Please let me know

Lee

Bojhan’s picture

@larowlan Yes, I would remove all cases where you use this except the "Are you sure" forms. We are 100% sure we want to use them there.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new8.1 KB
new15.56 KB
FAILED: [[SimpleTest]]: [MySQL] 48,061 pass(es), 0 fail(s), and 7 exception(s).
[ View ]

So this patch

  • Renames ajax_form_wrapper() to ajax_render_modal_form()
  • Removes the image style modals
  • Adds a modal for the forum delete confirmations (which are form based)
  • Retains the modal for the node delete links in admin/content (demonstrates the link method)
  • Simplifies the theme('messsages', ... as per sun's comment (took me three reads to understand what you meant there, thanks)
  • Moved the construction of $form_state to ajax_render_modal_form() (from ajax_modal_form()) allowing this logic to be reused in submit handlers - addressing @sun's final comment - ie keeping the $form_state internal to ajax_render_modal_form() - the forum delete example is hence far simpler than the image style one was
  • Renames the ajax commands to ajax_set_modal() and ajax_dismiss_modal() instead of ajax_set_form() and ajax_dismiss_form() to reflect the changes made to the JavaScript

From here I see two follow ups

  • [Meta] Decide on where we should use modals
  • [Meta] Port remaining confirm forms to use modals

What do we think?

Status:Needs review» Needs work

The last submitted patch, views-modal-in-core-1667742.57.patch, failed testing.

yched’s picture

Issue tags:+JavaScript

Tagging JS too

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new19.85 KB
FAILED: [[SimpleTest]]: [MySQL] 48,065 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new1.82 KB

Yep, that wasn't going to work.
More cleanup of putting the $form_state logic out of arms reach.

Status:Needs review» Needs work

The last submitted patch, views-modal-in-core-1667742.60.patch, failed testing.

larowlan’s picture

StatusFileSize
new1.82 KB
new15.53 KB
PASSED: [[SimpleTest]]: [MySQL] 48,082 pass(es).
[ View ]

damn wrong branch, interdiff above is right, patch should have been this one:

larowlan’s picture

Status:Needs work» Needs review
larowlan’s picture

ok, green again
follow-ups as per #57
what else needs to be done?

quicksketch’s picture

Great work @larowlan! I'm really looking forward to this excellent feature. I'm hinging on it for modal dialogs for WYSIWYG image and link editing. Though a different use-case, this still looks like it's going to provide some needed infrastructure.

Okay here come the nitpicks (sorry!):

+ * The 'redirect' command instructs the browser to redirect to the give url.

give => given

+ *   An internal drupal path.

drupal => Drupal

+  // Similar logic to drupal_goto.

drupal_goto => drupal_goto()

+  // Some forms may have the title built in, if so set the title here (for non
+  // javascript users):

non javascript => non-JavaScript

+    // @todo document hook
+    drupal_alter(array('ajax_form_commands', 'ajax_form_commands_' . $form_id), $commands, $form_state);

Document the hook.

+    // Degrade to non-javascript behaviour.

non-javascript => non-JavaScript

+  // Non javascript invocation - degrade to standard drupal_build_form
+  // behaviour. Forms can have the title built in, if so set the title here:

Non javascript => Non-JavaScript.
behaviour => behavior (Damn Americans! ;)

+ * Modules should use this as a menu callback to handle displaying forms in
+ * modals. Example hook_menu () item

hook_menu () => hook_menu()

+ *  @code
+ *   $items['node/%node/delete'] = array(
+ *    'title' => 'Delete',
+ *    'page callback' => 'ajax_modal_form',
+ *    'page arguments' => array('node_delete_confirm', 1),
+ *    'access callback' => 'node_access',
+ *    'access arguments' => array('delete', 1),
+ *    'weight' => 1,
+ *    'type' => MENU_LOCAL_TASK,
+ *    'context' => MENU_CONTEXT_INLINE,
+ *    'file' => 'node.pages.inc',
+ *   );
+ * @endcode

This makes me want to use a more familiar function name that is easier to find. If the function were named "drupal_get_form_modal" then it would be easy to find via the api.drupal.org site and remember, because I just need to add the word "modal" to the function I would usually call. However this makes ajax.inc not have its nice little "every function starts with ajax_" pattern. IMO, DX is more important here than making our .inc file have 100% consistent prefixes.

larowlan’s picture

thanks for the review @quicksketch, I agree with you on the name of the function

Fabianx’s picture

Looks really good.

larowlan’s picture

StatusFileSize
new7.06 KB
new18.04 KB
PASSED: [[SimpleTest]]: [MySQL] 48,112 pass(es).
[ View ]

addresses issues identified by @quicksketch

larowlan’s picture

Ok, so green again, @quicksketch and @sun's feedback resolved.
Follow-ups created as per instruction from @bojhan
#1842040: [meta] Decide on where to use modal dialogs
#1842036: [META] Convert all confirm forms already converted to new routing system to use modal dialog

What's next? I think this is very close, plus as it's a major task - getting this in helps with thresholds :)

yoroy’s picture

Status:Needs review» Reviewed & tested by the community

With reviews from sun and quicksketch & bojhan's UX guidance in the bag I think the current state deserves an rtbc. You know, that version of rtbc where we leave it for a couple of days for any final reviews and nitpicks before actually committing :-) Great work all.

sun’s picture

Status:Reviewed & tested by the community» Needs work

Sorry, but I only reviewed this once and never signed it off.

The code looks slightly better now, but still not ready from an architectural and DX perspective.

+++ b/core/includes/ajax.api.php
@@ -0,0 +1,79 @@
+function hook_ajax_form_title_alter(&$title, $form_state) {
...
+function hook_ajax_form_title_FORM_ID_alter(&$title, $form_state) {
...
+function hook_ajax_form_commands_alter(&$commands, $form_state) {
...
+function hook_ajax_form_commands_FORM_ID_alter(&$commands, $form_state) {

All of these should not exist.

+++ b/core/includes/ajax.inc
@@ -1140,3 +1171,181 @@ function ajax_command_add_css($styles) {
+function ajax_render_modal_form($form_id, $args) {

90% of the extremely complex logic in this function are undocumented. It will take me some time to study it.

For now, all I can say is that the amount of custom processing going on there indicates that something is architecturally wrong.

+++ b/core/modules/forum/forum.admin.inc
@@ -75,7 +75,16 @@ function forum_form_forum($form, &$form_state, $edit = array()) {
-    $form['actions']['delete'] = array('#type' => 'submit', '#value' => t('Delete'));
+    $form['actions']['delete'] = array(
+      '#type' => 'submit',
+      '#value' => t('Delete'),
+      '#attached' => array(
+        'library' => array(array('system', 'drupal.ajax.modal'))
+      ),
+      '#ajax' => array(
+        'callback' => 'forum_confirm_delete_ajax'
+      )
+    );

@@ -190,7 +199,13 @@ function forum_form_container($form, &$form_state, $edit = array()) {
-    $form['actions']['delete'] = array('#type' => 'submit', '#value' => t('Delete'));
+    $form['actions']['delete'] = array(
+      '#type' => 'submit',
+      '#value' => t('Delete'),
+      '#ajax' => array(
+        'callback' => 'forum_confirm_delete_ajax'
+      )
+    );

This still looks too much for me.

We might need to postpone this on #1238484: [docs update] Ability to mark the form buttons to be of a specific type (so that they can be styled differently) or find another, similar, but ultimately much more automated way to indicate that a certain form button or link should open a modal.

+++ b/core/modules/forum/forum.admin.inc
@@ -200,6 +215,15 @@ function forum_form_container($form, &$form_state, $edit = array()) {
+function forum_confirm_delete_ajax($form, &$form_state) {
+  return ajax_render_modal_form('forum_confirm_delete', array(
+    $form_state['values']['tid']
+  ));
+}

Why is this a separate callback?

+++ b/core/modules/forum/forum.admin.inc
@@ -209,6 +233,7 @@ function forum_form_container($form, &$form_state, $edit = array()) {
function forum_confirm_delete($form, &$form_state, $tid) {
+  $form['#action'] = url('admin/structure/forum/edit/forum/' . $tid);

Why is this needed?

larowlan’s picture

All of these should not exist.

These are only here so we can use this api with views, views ui does extra stuff with it's modals with titles and highlighting sections. So the plan being to port views ui modals to use this api. Without these we have two near identical systems. (see views/include/ajax.inc)

Why is this a separate callback?

We have to pass the form_state['values']['tid'] as an argument to the forum_confirm_delete form builder.

Why is this needed?

Because drupal_build_form will set #action to the current url, which because this is ajax is system/ajax, so the modal ends up with

<form action="/system/ajax">...</form>

without this.

Please advise, happy to discuss on irc if needed.

Fabianx’s picture

We can maybe learn something from ctools_automodal for how to trigger things. Especially the github fork() has an interesting approach.

effulgentsia’s picture

Great to see such progress here.

For now, all I can say is that the amount of custom processing going on [in ajax_render_modal_form()] indicates that something is architecturally wrong.

IMO, what's wrong is that we have a ajax_command_set_modal() command that is different from a ajax_command_insert() command. This requires the PHP code to care whether something is to be shown in a modal vs. any other kind of AJAX insertion. Whereas it seems to me that this could be made entirely client-side logic, since all Drupal.ajax.prototype.commands.modalOpen() does is run some JS code and invoke the insert command anyway, on the #drupal-modal wrapper. I'm guessing we can instead make a client-side 'modal' behavior that can be applied to links and buttons and that can wrap the Drupal.ajax invocation with the equivalent stuff, allowing the PHP code to not have to do anything different for a normal AJAX response vs. a "AJAX to be shown in a modal" response.

Meanwhile, for normal AJAX responses, we already have Drupal\Core\EventSubscriber\ViewSubscriber::onAjax() handling them. Quite possibly, we haven't covered the "return an entire form" as an AJAX response use case yet, and that some of the code in this patch is required to handle that, but it would be nice to move those fixes directly into the Form API or the ViewSubscriber::onAjax() pipeline, and not be so tied up with modals.

If we do that, we should be able to remove all the stuff mentioned in #71.

The above refactoring might take some time to hash out though, as it involves the intersection of JavaScript, Form API, and Drupal's server-side AJAX system. And to complicate matters, the server-side AJAX system is being significantly refactored in #1812866: Rebuild the server side AJAX API, and the client-side portion is being discussed in #1533366: Simplify and optimize Drupal.ajax() instantiation and implementation. Meanwhile, though, basic modal functionality is wanted for WYSIWYG and Layout UI work before feature freeze.

So, I wonder if we can add a few todos to #68, but otherwise expedite it, and then work on cleaning up the PHP side in follow ups. I can help write some of those todos if sun and others here agree to that suggestion.

sun’s picture

That's a very far stretch to ask for. I assume it is caused by feature freeze. What constitutes a feature is:

  1. A new feature is introduced that enables new functionality or changes existing in a significant way.
  2. The new feature impacts how I should or need to write my module's or theme's code.

Post feature-freeze, that's extended by:

  1. The newly introduced feature might be factored and revamped entirely to be architecturally "compatible" or make more sense.
  2. It might be polished and improved in detailed areas that do not influence existing implementations.
  3. API changes must be avoided at all costs and may only happen if absolutely required until code freeze. Rien ne va plus after that.

In light of that:

What I'm currently missing is a clear definition of "the feature"; i.e., what is explicitly supposed to be supported and what is not. I.e., when, exactly, are modals supported, what is the user interface pattern for that, where is it documented, and perhaps most importantly, which (possible) usage of modals is explicitly not supported?

Furthermore, as I already alluded to in my review above, the feature being introduced here ought to be much easier for module developers.

So in terms of @todos and a (borderline) "acceptable" initial feature, I'd much rather expect hefty hacks and ugly workarounds within Form API and Ajax API internals, along with corresponding @todos, but in completely different locations, which can then be cleaned up and resolved post feature freeze.

And implementation-wise, I'd expect literally nothing more than this:

-    $form['actions']['delete'] = array('#type' => 'submit', '#value' => t('Delete'));
+    $form['actions']['delete'] = array('#type' => 'submit', '#value' => t('Delete'), '#modal' => TRUE);

In other words, circling back into what I just mentioned above:

I don't care at all how Views or whichever contrib module implemented this for D7. Product features need to be designed in a proper way; i.e., defining how users (here: developers) are going to benefit from a feature and how they are going to use it. If a feature is too complex to use, it just simply won't get used, or it won't be used properly. Consequently, completely failing on its goal.

This is further manifested by the fact that modal dialogs imply a user interface pattern, and user interface patterns must be used in a consistent fashion, so as to not horribly confuse actual end-users. Thus, if this is too complex to implement, only fraction of modules will do it, leaving end-users behind with a very inconsistent user interface. In that case, the end result would actually be worse compared to what we have today, so I'd strongly object to that.

I hope this provides sufficient reasoning and a clear path forward.

Bojhan’s picture

Can we get some actual todo's? This seems to be going down the exact same route as all the other modal issues, because people are keen on re architecting everything every time for each UI issue. @larowlan addressed some of @sun's concerns, and sun mentioned we need to add todo's in the accurate places - so lets do that, and then push this in.

@sun Your comment on features, is completely unrelated to this issue. Yes it is a problem, that our whole "code freeze" is largely undefined. But lets not try to solve that here. Also I want to keep "how to use it" discussion out of this issue, as it totally side-tracks correct implementation. We will similar to dorpbutton open an appropriate followup.

rvilar’s picture

I want to help on #1842036: [META] Convert all confirm forms already converted to new routing system to use modal dialog, but I see that I can't start on that until this issue is closed. Can I help on this?

falcon03’s picture

@sun#75: I can understand your important concerns. But we must strongly keep in mind that we need modals in core to enhance views and other features' accessibility.

If feature freeze can block this issue, can we ask for an exception to core maintainers to let us work on this issue until code freeze?

effulgentsia’s picture

Assigned:larowlan» effulgentsia

I'm gonna work on cleaning up the PHP part of this patch today.

effulgentsia’s picture

I got distracted by some other issues, so apologies for not getting to this yet. Will try to this weekend.

Dries’s picture

Sorry, but I only reviewed this once and never signed it off.

@sun: I've noticed a trend in your comments across different issues, and it is time for me to speak up about it. You appear to increasingly bash ideas or people. I love your passion for Drupal and respect your technical depth, but your approach and communication style turns people off and doesn't foster a collaborative culture. The truth is people don't need formal sign off from you. More importantly, you'll want to change your style and create an environment were people look forward to a "sun review" (remember that time?) versus creating an environment where people loath a sun review because it makes them feel like crap. Good leaders don't make other people feel like crap. It's not right. You can't be proud of yourself if you do that. Good leaders respect and motiviate people, build consensus, and empower them to shine. Again, I love your passion and your technical depth, but I really need you to be a better leader here. Thanks!

larowlan’s picture

@sun, apologies if I've misinterpreted you're comments here.
Given that we were down to nitpicks at #65 I genuinely thought that I'd addressed your concerns and this was close to ready.
Clearly this was not the case.
I too respect your technical knowledge and passion for Drupal, a passion I share.
Given #1812866: Rebuild the server side AJAX API completely refactors the way ajax responses are built, there is some significant re-working of this patch required as is, so at the same time I feel it would be an ideal opportunity to address your concerns.
Can you articulate further what you consider to be the way forward here, as it's not completely clear to me. Are you proposing building this functionality into the form api at the ground level via process callbacks to attach required logic/libraries and content-type handling logic in drupal_build_form? I don't really see any other way to implement the functionality you've described (eg the #modal => TRUE) in a generic way without such low-level integration. But that said, I could be missing something obvious. Please let me know.
As well as this being a killer feature for Drupal 8, I've invested a fair bit of time in it and hence have a vested interest in seeing it make the cut, in whatever form that may take.
Thanks
Lee

effulgentsia’s picture

Assigned:effulgentsia» larowlan
Status:Needs work» Needs review
StatusFileSize
new11.61 KB
FAILED: [[SimpleTest]]: [MySQL] 48,231 pass(es), 0 fail(s), and 4,015 exception(s).
[ View ]

Ok, here's a minimal solution. I replaced the forum.module use case with the Delete button on the node edit form, because forum_form_main() is a mess that needs to be refactored to use Form API correctly.

There are some problems with modal submissions busting the base page out of the Overlay, but I think that can be fixed by someone with better JS skills than I have.

@larowlan: the basic architectural difference with your patch is that this allows modal submissions to happen to the same URL as if it weren't in a modal. In D8, we have proper content negotiation, so some of the D6 and D7 CTools workarounds related to submitting to a different URL aren't needed, and that simplifies things a lot.

However, this patch does have a @todo related to content negotiation not working on redirects from IframeUpload submissions.

@larowlan: since this was assigned to you before #79, I'm assigning it back to you, in case you're using assignment to track your issues. I'm happy to continue helping on this though until we get it in.

effulgentsia’s picture

Given #1812866: Rebuild the server side AJAX API completely refactors the way ajax responses are built

By the way, turns out it doesn't. That patch added some nice new classes, but didn't change Drupal to use them yet. There are follow up issues listed on that issue to do so.

Status:Needs review» Needs work

The last submitted patch, modal-83.patch, failed testing.

effulgentsia’s picture

The truth is people don't need formal sign off from you.

In sun's defense, the first sentence of #71 was a direct response to him being used as one of the factors in #70's RTBC. However, I still agree with Dries's feedback in general.

and create an environment where people look forward to a "sun review" (remember that time?)

I just want to attest to this not being taken lightly. I alone have received hundreds of reviews from sun over the past 3 years, and have learned a ton from many of them. For Drupal to succeed, we need contributors continually improving their skills, and reviews from sun have done this for me and many other contributors. But, sun, that doesn't happen when the tone is too antagonistic, condescending, or controlling. It's not always easy to walk the line between pointing out problems constructively vs. destructively, and to be honest, I don't even know if #71 crossed it, but some of your comments on other issues have. My hunch is that it's a symptom of burnout. If I'm right, then, please, please, take a little time to unwind and take care of yourself. Drupal will survive while you're resting, and be better off in the long term if you get the rest you need.

larowlan’s picture

Thanks @effulgentsia, (I think) this clarifies what @sun was looking for above and confirms my speculation in #82 was closer to the mark than I thought.
In @sun's defence, I don't think a line was crossed at #71, however I think perhaps the tone at #75 could have been more constructive; I can't speak to other issues.
Along with @effulgentsia I have learnt a lot from @sun's reviews, both of my work and of others and Drupal is definitely the better for it.
In fact, I was stoked when @sun chimed in at #53 and (I felt) the changes required were minimal, it gave me confidence that this was close to being accepted.
I think everyone is feeling the pressure at the moment given the looming feature freeze and I hope we have a way forward with this issue.
Thanks again

nod_’s picture

StatusFileSize
new11.33 KB
FAILED: [[SimpleTest]]: [MySQL] 48,222 pass(es), 0 fail(s), and 4,019 exception(s).
[ View ]

Updated patch, there was a little problem in the $.extend of the modal file, wasn't extending the commands object. Overlay doesn't close when clicking the dismiss link but still closes when the form is submitted. Apparently the behaviors on the child page aren't triggered properly.

Haven't fixed the PHP warning either.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new606 bytes
new11.48 KB
PASSED: [[SimpleTest]]: [MySQL] 48,218 pass(es).
[ View ]

This fixes the PHP warning. I like the JS cleanup in #88 except:

+++ b/core/misc/ajax.js
@@ -477,6 +513,9 @@ Drupal.ajax.prototype.commands = {
+      ajax.commands.modalOpen(response.title);
...
+++ b/core/misc/ajax.modal.js
@@ -0,0 +1,61 @@
+$.extend(Drupal.ajax.prototype.commands, {
+  modalOpen: function (title) {

Per #74, I had actually intentionally changed that in #83 to extend Drupal.ajax, not Drupal.ajax.prototype.commands. I don't consider modalOpen to be a command for PHP to send back. PHP should only send back an insert command, and the JS code decides under what conditions to open a modal. So, can we change this back to not be a command? I didn't do so in this patch, cause wanting nod_'s feedback on that first.

nod_’s picture

Oh ok, sorry, didn't get it.

If that's the case, better make a Drupal.modal object and not tie it to the Ajax framework at all. I changed it back since the patch didn't work for me. On thing we should discuss then is if we allow several dialogs open on the page? (since dialogs don't have to be modals, can just be popins).

I'll mess around with that for a bit and I'll update the patch after dinner.

xjm’s picture

Title:Add abstracted views modal to core» Add abstracted views modal to core (resolves accessibility bug)
Priority:Major» Critical

#1806308: Review Views JavaScript + generic modals for accessibility is a critical bug that is probably resolved by this, so escalating the priority of this issue. (The other may be a duplicate, but it is postponed until this is resolved, one way or the other.)

mgifford’s picture

@xjm I was trying to see how it resolves the accessibility bug or who did the testing for that. Can you give me a bit more information on this.

Not sure how it's tied, but would love to get it fixed. Modals aren't easy.

larowlan’s picture

Hi Mike
This doesn't solve issues with views with the current patch (no views ported in this issue), but could.
Nor does it solve accessibility in general until jquery.ui 1.10 is released, which are the github pull requests you and falcon03 have been testing.
That's where the accessibility review is taking place, but in a general nature - not specifically of views modals.
Hope that helps.

Lee

mgifford’s picture

Thanks for the clarification Lee.

nod_’s picture

Title:Add abstracted dialog to core (resolves accessibility bug)» Add abstracted views dialog to core (resolves accessibility bug)
StatusFileSize
new12.1 KB
PASSED: [[SimpleTest]]: [MySQL] 48,758 pass(es).
[ View ]

All right after my 6 day dinner updated patch.

Dialog thing not dependent on Ajax commands. Works well. I pretty much implemented http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.htm... saving the position stuff.

Renamed modal-dismiss by modal-cancel. No reason to introduce different terms here. to use the dialog API:

var myDialog  = Drupal.dialog(SomeDOMElement, settings);

myDialog.show();
myDialog.showModal();
myDialog.close('a return value of some sort');

console.log(myDialog.returnValue);

There are 4 events fired on window to allow contrib to change settings, DOMElement or whatever. The ajax file is using a couple of events to bind the cancel link closing so that we keep that out of the modal.js file.

nod_’s picture

Title:Add abstracted views modal to core (resolves accessibility bug)» Add abstracted dialog to core (resolves accessibility bug)
Assigned:larowlan» Unassigned
nod_’s picture

Title:Add abstracted views dialog to core (resolves accessibility bug)» Add abstracted dialog to core (resolves accessibility bug)
StatusFileSize
new12.03 KB
PASSED: [[SimpleTest]]: [MySQL] 48,734 pass(es).
[ View ]

modal.js should be named dialog.js really, updated patch.

jessebeach’s picture

The code in #97 looks good. I applied it, read through it and understood how I would use a modal in other contexts.

This issue has a lot of history. Based on #97 I would RTBC. If someone else feels so as well, let's do it.

sun’s picture

The latest patch looks way better and achieves a deep integration into the various subsystems. I'm not even able to spot any major subsystem hacks in it, so it's even better that we don't even need any workarounds to make this possible.

Overall, I'm not sure whether the processing/handling of #ajax][modal] shouldn't be converted into an Ajax command, but that's the exact kind of internal refactoring that we can happily perform post feature freeze, so there's no reason to discuss this detail in this issue and hold up this feature for it.

The essential feature and API that we're adding to D8 is this:

  $element['delete']['#ajax']['modal'] = TRUE;

Whereas the expectation is that this works for any form button, and also, for any kind of link (though using a different syntax for a link that goes through theme_links().)

There do not seem to be any new tests to verify the expected behavior and functionality, but we generally lack tests on the Ajax front, so I guess that confirmations based on manual testing will be sufficient for this issue. We can investigate whether it is possible to add more sophisticated/automated test coverage in a separate follow-up issue.

Thus, we're down to final reviews. Here's mine:

+++ b/core/includes/ajax.inc
@@ -474,7 +474,7 @@ function ajax_prepare_response($page_callback_result) {
     // #ajax['method']. The default method is 'replaceWith', which completely
     // replaces the old wrapper element and its content with the new HTML.
     $html = is_string($page_callback_result) ? $page_callback_result : drupal_render($page_callback_result);
-    $commands[] = ajax_command_insert(NULL, $html);
+    $commands[] = ajax_command_insert(NULL, $html) + array('title' => drupal_get_title());

Can we clarify why we're unconditionally adding a page title here? (in code)

+++ b/core/includes/ajax.inc
@@ -589,12 +589,15 @@ function ajax_pre_render_element($element) {
     // Assign default settings.
     $settings += array(
-      'path' => 'system/ajax',
+      'path' => isset($settings['callback']) ? 'system/ajax' : NULL,
       'options' => array(),
     );

Can we clarify why the path should only default to 'system/ajax' when there is a callback? (and not always, like previously?)

+++ b/core/includes/theme.inc
@@ -1739,8 +1739,24 @@ function theme_links($variables) {
-        // Pass in $link as $options, they share the same keys.
-        $item = l($link['title'], $link['href'], $link);
+        // @todo Reconcile Views usage of 'ajax' as a boolean with the rest of
+        //   core's usage of it as an array.
+        if (isset($link['ajax']) && is_array($link['ajax'])) {
+          // To attach Ajax behavior, render a link element, rather than just
+          // call l().

Wouldn't it make sense to just check for $link['ajax'] and conditionally execute this instead?

ajax_pre_render_element(array('#type' => 'link', '#ajax' => $link['ajax']));
+++ b/core/misc/ajax.js
@@ -78,6 +78,37 @@ Drupal.behaviors.AJAX = {
+Drupal.behaviors.AJAXModalDialog = {
...
+    if (!$('#drupal-modal').length) {
...
+    $element.on('click.ajaxmodal', '.modal-cancel', function (e) {
+      dialog.close('cancel');
...
+  $element.off('.ajaxmodal');

+++ b/core/modules/system/system.module
@@ -1171,6 +1171,21 @@ function system_library_info() {
+  $libraries['drupal.dialog'] = array(

@@ -3350,6 +3365,9 @@ function confirm_form($form, $question, $path, $description = NULL, $yes = NULL,
+    '#attributes' => array(
+      'class' => array('modal-cancel'),
+    ),

The naming of classes, behaviors, libraries, dependencies, and functions appears suboptimal to me.

Wouldn't it make sense to

1) rename all instances of "modal" into "dialog"

2) rename all instances of "drupal[.-]modal" into "ajax[.-]dialog"

?

Ideally, I'd think we should try to stay as close to jQuery Dialog as possible — while we're implementing a modal dialog here, I can't see a reason for why there should not be a direct follow-up issue to this that asks for non-modal dialog support. Thus, by renaming things to "dialog", we keep all doors wide open to that. A modal is really just a subset of a dialog, and that's a semantic that most dialog libraries (including jQuery Dialog) follow and implement very well.

That said, the HTML ID #drupal-modal should probably be just #ajax-modal and not #ajax-dialog, since there can only be a single modal by design.

--

Lastly, I think that some of the earlier comments were unfair, off-topic, and inappropriate. I fundamentally disagree with some of them. But that's off-topic for this issue. Let's make sure to get this feature into core instead. It looks like we're close. :)

effulgentsia’s picture

Assigned:Unassigned» effulgentsia

I'm working on implementing sun's feedback.

effulgentsia’s picture

Assigned:effulgentsia» Unassigned
StatusFileSize
new10.22 KB
new13.59 KB
PASSED: [[SimpleTest]]: [MySQL] 48,733 pass(es).
[ View ]

This addresses #99, and in the process moves some functions from ajax.js to dialog.js and changes indentation in dialog.js, so the interdiff appears bulkier than it really is.

Wouldn't it make sense to just check for $link['ajax'] and conditionally execute [ajax_pre_render_element()] instead?

It would need to be drupal_pre_render_link() in order to get the id attribute in there, but I fail to see how that makes more sense than calling drupal_render(). I actually think we should make it always call drupal_render(), and not have the else part, but we first need to fix Views to not set 'ajax' on links with different semantics. There's a todo comment about that already in the patch.

nod_’s picture

Not having any behavior or event listener in dialog.js was on purpose. I want to be able to use dialog.js without any assumption, it doesn't need to create a drupal-modal element to work, the ajax framework does. That's not a biggie, I'll move them to their own file later on.

effulgentsia’s picture

Own file would be fine, but no need for it to be in ajax.js. Non-AJAX use cases of #drupal-modal and .dialog-cancel are also possible (e.g., wysiwyg).

effulgentsia’s picture

StatusFileSize
new1010 bytes
new13.71 KB
PASSED: [[SimpleTest]]: [MySQL] 48,745 pass(es).
[ View ]
+++ b/core/misc/ajax.js
@@ -587,8 +558,15 @@
-      Drupal.dialog(wrapper, {title: response.title}).showModal();
+      Drupal.dialog(wrapper, {title: response.title}).show(ajax.dialog);

I thought I was being clever here, but I subsequently realized that this violates nod_'s intent of sticking to the API of http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.htm.... This corrects that.

Bojhan’s picture

Status:Needs review» Reviewed & tested by the community

It looks like @sun his concerns are addressed and it was his final review - it looks like we are in good shape for getting this into core. Given that its a huge change, which always lead to follow ups and we need to get this major thing in this week. I am going to go ahead and mark it RTBC.

For A11y followers, this does not actually resolve the accessibility bug of the modal. The jQuery UI team is working intensely on that, and we expect to update to that version as soon as its released - see https://github.com/jquery/jquery-ui/pull/794. Although its not common practive to commit before resolving a bug like this, we are tied to code freeze and this is the third release we try to get a modal into core and its a great intimidate step till jQuery UI updates are released.

catch’s picture

Just a note that since this blocks a critical bug it's not necessarily blocked by feature freeze at all.

I'm not sure why the node delete is included in the patch, is that just to have an implementation for testing? If so can we drop it from the patch and add it in a new issue? It seems like it'd be either worth doing for all entities or not at all, and I'm not sure about introducing that new interface pattern along with the rest of the patch.

Bojhan’s picture

@catch We can drop it from the patch.

jessebeach’s picture

StatusFileSize
new1.81 KB
new1.23 KB
new13.38 KB
PASSED: [[SimpleTest]]: [MySQL] 48,748 pass(es).
[ View ]

I pulled out the node delete operation code and put that in its own file for posterity (core-js-modal-1667742-node-delete-modal-example-do-not-test.txt).

I agree with nod_ that dialog.js should be void of implementation code (the attached events), but looking at it, I can't figure the appropriate place to move the event attachments to right now. I'm reluctantly willing to let it go as something we can improve later since no better options present.

I agree with sun that tests are lacking. This is debt we knowingly take on as part of the test writing work that looms on our nearing horizon for most front end code.

jessebeach’s picture

Status:Reviewed & tested by the community» Needs review

Setting to needs review to get this into people's queues. Should be set back to RTBC once the small change to remove the node delete operation code is verified and accepted.

effulgentsia’s picture

StatusFileSize
new4.6 KB
new16.56 KB
PASSED: [[SimpleTest]]: [MySQL] 48,749 pass(es).
[ View ]

Ripped out one other hunk missed in #108, and added demonstration use cases to ajax_test.module. I also added a starter DialogTest, though it's thin at the moment, because all the interesting stuff happens client-side.

effulgentsia’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC. I think catch is a sufficient reviewer for the interdiffs in 108 and 110.

sun’s picture

This looks great now.

I'm not sure I understand why this is not perceived as a feature, but anyway, would be great to get this in before feature freeze.

Thanks all!

nod_’s picture

Views modals are not accessible, this will in time replace all views modal and is aimed at being accessible, hence not really a feature. But anyway it's ready now so no pressure :)

jessebeach’s picture

Here's the followup to convert confirm forms to modals, just to get it at the bottom of the issue:

#1842036: [META] Convert all confirm forms already converted to new routing system to use modal dialog

webchick’s picture

Title:Add abstracted dialog to core (resolves accessibility bug)» Change notice: Add abstracted dialog to core (resolves accessibility bug)
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Yeah, this feels a little borderline between feature/bug (a "fug"? ;)) but the accessibility work done here is important for resolving critical Views accessibility problems, and is also coming up over and over as a fundamental UX element in various other major/critical UX patches (CMI UI, Entity Translation UI, etc.). Looks like it's been pretty well picked over by several smart folks, so that's great.

I'm not sure if all of the use cases outlined by User Advocate are covered here... looks like there's an option to show / show as a modal as well as an arbitrary class that can be added to any form button to enact "cancel" type behaviour, so that's a good start. We might want to expand this patch in a follow-up to create some nice wrapper functions / properties / something for creating standardized "OK" / "Yes / No" / "OK / Cancel" type of dialogs so that we don't end up with inconsistencies in the way various modules implement these patterns. But most likely, the easiest way to spot these will be to convert various forms to use it and such patterns will likely naturally emerge as a result.

Committed and pushed to 8.x. Great work on this, folks! :D

Let's get a change notice with some instructions on how people can use this in their modules.

quicksketch’s picture

Woohoo! Yay great work all.

xjm’s picture

MustangGB’s picture

Has this been implemented anywhere yet?

nod_’s picture

nope.

Change notice, I covered the JS part, someone up for the PHP side? https://drupal.org/node/1852224

nod_’s picture

Status:Active» Needs review
effulgentsia’s picture

I edited the JS change notice and added a #ajax one.

nod_’s picture

thanks! Looks good to me, may want a confirmation that there is enough information to understand from someone else though.

tstoeckler’s picture

Priority:Critical» Normal

The change notice looks good. With its help I just tried manually replacing the node delete button by a modal link. Awesome!!!
One thing that might be worth mentioning is what the 'modal' => TRUE/FALSE actually does, specifically what the difference is if you *don't* set 'modal' to TRUE. (I would have edited myself, but I don't actually know the answert to that question.) That is by no means critical, though so leaving open, but marking normal for now. Should be back to 'critical' when it's marked fixed.

nod_’s picture

Dean Reilly’s picture

StatusFileSize
new215.38 KB

How would I go about setting the size of a dialog window? I'm currently working on adding a diff to the configuration synchronization page #1821548: Add a "diff" of some kind to the CMI UI and noticed the default dimensions are too small (see the attached screen shot).

jibran’s picture

Title:Change notice: Add abstracted dialog to core (resolves accessibility bug)» Add abstracted dialog to core (resolves accessibility bug)
jibran’s picture

Issue tags:-Needs change record
+++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/DialogTest.phpundefined
@@ -0,0 +1,32 @@
+  /**
+   * Ensure elements with #ajax['dialog'] render correctly.
+   */
+  function testDialog() {
+    // Ensure the elements render without notices or exceptions.
+    $this->drupalGet('ajax-test/dialog');
+
+    // @todo What else should we assert?
+  }
+
+}

Created a follow up issue for this @todo #1884530: Complete modal dialog tests.

webchick’s picture

Dear 71 people following this issue, #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases is apparently blocking anything in core from actually using this nice new dialog API, so if you could take a look, that'd be swell. :)

Also, would someone please be able to give a final review of the change notice so we can mark this puppy fixed?

larowlan’s picture

Status:Needs review» Fixed

Change notice from #121 looks good to me

Status:Fixed» Closed (fixed)

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

jibran’s picture

Issue tags:+modal dialog

Tagging.

andypost’s picture

Does this code could be possible to backport to D7?

klonos’s picture

The way I see it from reading the "API changes" section of the issue summary, there has been API addition - not an actual change (as in "alteration") per se. I think if that's true and we don't break things, we should consider backporting.

effulgentsia’s picture

#1870764: Add an ajax command which makes it easy to use the dialog API in complex cases changed the implementation of this issue considerably, so once a change notice is written there, let's use that issue to discuss/do backporting.

effulgentsia’s picture

Version:8.x-dev» 7.x-dev
Assigned:Unassigned» David_Rothstein
Status:Closed (fixed)» Active
Issue tags:+Needs committer feedback

Actually, one thing that might be worth discussing here though is just a question of whether a backport is even on the table, considering that D7 uses jQuery UI 1.8, and we need jQuery UI 1.10 for the dialog to be accessible.

webchick’s picture

I don't understand how a D7 backport that includes an upgrade of jQuery UI to 1.10 could possibly be on the table? We know jQuery UI 1.8 => 1.10 broke at least Edit module in core.

scott.gonzalez’s picture

For the D7 backport, dialog would be the least of your concerns. 1.10 removed a lot of 1.8 APIs: http://jqueryui.com/upgrade-guide/1.10/ Backporting would not be a good idea.

effulgentsia’s picture

Right. So the question is: is a backport of the dialog API on the table, if we don't use it in D7 core, but just provide the API for contrib modules to use. Then contrib modules can then decide to either make jQuery Update a dependency, or accept a dialog with accessibility problems.

My gut feeling at this time is that we should not do that, and leave it to http://drupal.org/project/dialog or some other contrib module to backport what's been done for D8.

quicksketch’s picture

Does this code could be possible to backport to D7?

Including it in CTools is probably the best bet. Here's the issue for switching CTools to jQuery UI: #1397370: Make modal.js use jQuery dialog.

effulgentsia’s picture

Version:7.x-dev» 8.x-dev
Assigned:David_Rothstein» Unassigned
Status:Active» Closed (fixed)
Issue tags:-Needs committer feedback

Given #137 and #139, restoring issue attributes to pre #135. If someone feels strongly about this being backported to core, you can reopen.

effulgentsia’s picture

Issue summary:View changes

summary update

jason.fisher’s picture

FYI, I have updated the CTools JQuery Dialog patch here against latest 7.x-1.x-dev release: https://drupal.org/node/1397370