Problem/Motivation

There are multiple places in drupal core where you cannot use the easy to use #ajax element in forms to do your custom logic, but you
need actual support by custom javascript OR an ajax command. Some examples are a page manager (#1841584: Add and configure master displays)or the views edit interface.

Proposed resolution

Add an openDialogCommand/closeDialogCommand (bikeshed the name) that takes a title, the content and potentially even the dialog settings.
Additional to the PHP level you need on the JS level the actual commands and additional binding for the form element.

Remaining tasks

Postponed on this

Related

CommentFileSizeAuthor
#102 interdiff.txt1.21 KBquicksketch
#102 ajax_dialogs-1870764-102.patch38.97 KBquicksketch
#94 ajax-dialog-1870764.92.wip_.interdiff.txt7.52 KBlarowlan
#86 interdiff.txt2.83 KBquicksketch
#86 ajax-dialog-1870764-86.patch38.94 KBquicksketch
#81 interdiff.txt3.97 KBquicksketch
#81 ajax-dialog-1870764-81.patch35.97 KBquicksketch
#79 core-js-dialog-ajax-1870764-79.patch36.51 KBnod_
#79 interdiff.txt6.57 KBnod_
#75 interdiff.txt8.93 KBquicksketch
#75 ajax-dialog-1870764.75.diff36.71 KBquicksketch
#72 ajax-dialog-1870764.72.diff35.98 KBquicksketch
#72 interdiff.txt7.2 KBquicksketch
#69 interdiff.txt2.27 KBquicksketch
#69 ajax-dialog-1870764.69.diff35.59 KBquicksketch
#68 interdiff.txt8.48 KBquicksketch
#68 ajax-dialog-1870764.67.diff35.57 KBquicksketch
#65 interdiff.txt3.99 KBquicksketch
#65 ajax-dialog-1870764.65.diff29.54 KBquicksketch
#64 ajax-dialog-1870764.64.interdiff.txt14.2 KBquicksketch
#64 ajax-dialog-1870764.64.diff29.56 KBquicksketch
#61 ajax-dialog-1870764.61.interdiff.txt1.56 KBlarowlan
#61 ajax-dialog-1870764.61.patch37.07 KBlarowlan
#60 ajax-dialog-1870764.60.interdiff.txt2.91 KBlarowlan
#60 ajax-dialog-1870764.60.patch37.58 KBlarowlan
#57 ajax_dialogs-1870764-51.diff36.13 KBquicksketch
#48 interdiff.txt653 bytesfrega
#48 drupal-1870764-48.patch28.12 KBfrega
#44 drupal-1870764-44.patch28.14 KBfrega
#44 interdiff.txt4.08 KBfrega
#29 interdiff.txt2.42 KBdawehner
#29 drupal-1870764-29.patch28.72 KBdawehner
#25 drupal-1870764-25.patch27.9 KBfrega
#25 interdiff.txt16.21 KBfrega
#23 drupal-1870764-23.patch26.45 KBdawehner
#22 drupal-1870764-22.patch19.11 KBdawehner
#21 drupal-1870764-21.patch12.04 KBdawehner
#15 1870764-15.patch26.64 KBfrega
#15 interdiff.txt19.85 KBfrega
#13 1870764-13.patch17.76 KBfrega
#4 drupal-1870764-4.patch6.79 KBdawehner
#3 drupal-1870764-3.patch6.5 KBdawehner
#2 drupal-1870764-2.patch2.76 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner’s picture

Status: Active » Needs work
FileSize
2.76 KB

This still certainly needs more ideas copied from the original code at http://privatepaste.com/e1bbddc718

dawehner’s picture

FileSize
6.5 KB

Tracking some work.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.79 KB

So this fixes the closing dialog.

gdd’s picture

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,54 @@
+    // @todo Should it be able to control the dialog settings?

I definitely think it would be useful to add this. There's a ton of use cases for various dialog settings that are difficult to use now (for instance we could use this on #1821548: Add a "diff" of some kind to the CMI UI)

nod_’s picture

Category: feature » task
Priority: Normal » Major

Holds up a major Views and CMI task .

dawehner’s picture

@nod_
Did you made progress here? You told something in IRC.

effulgentsia’s picture

Priority: Major » Critical

#1851414: Convert Views to use the abstracted dialog modal is a critical task postponed on this, so raising this to critical.

effulgentsia’s picture

Add an openDialogCommand/closeDialogCommand (bikeshed the name)

#4 uses these names, but hard-codes functionality to just modal dialogs. So I think the names should be openModalCommand/closeModalCommand or the currently named commands should be abstracted to support non-modal dialogs.

I definitely think it would be useful to add this. There's a ton of use cases for various dialog settings that are difficult to use now

I agree about it being useful, but I think a challenge is that dialog.js is currently modeled on http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.htm..., so that it is abstracted from jQuery UI (leaving open the possibility to remove that dependency), and AFAICT, there's no spec there for settings. @nod_: any thoughts on this?

Crell’s picture

I think the HTML5-ish way to handle settings would be data-* attributes on whatever the main element is, no?

sun’s picture

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.php
@@ -0,0 +1,54 @@
+class OpenDialogCommand extends InsertCommand {
...
+      'selector' => '#drupal-modal',

+++ b/core/misc/dialog.ajax.js
@@ -0,0 +1,43 @@
+  Drupal.ajax.prototype.commands.openDialog = function (ajax, response, status) {
+    var $modal = $('#drupal-modal');
...
+      element_settings.dialog = {
+        modal: true
+      };
...
+  Drupal.ajax.prototype.commands.closeDialog = function (ajax, response, status) {
+    var $modal = $("#drupal-modal");

We have a mismatch here — a modal is a specific incarnation of a dialog.

If we call the Ajax commands "Dialog", then I expect to be able to use them for dialogs; i.e., potentially multiple on the same page.

The sub-facility of being able to deliver modal functionality should either be an optional parameter/setting for the Ajax commands, or we should introduce two separate Ajax command sets for dialogs and modals.

+++ b/core/misc/dialog.ajax.js
@@ -0,0 +1,43 @@
+(function ($, Drupal, drupalSettings) {
...
+})(jQuery, Drupal, drupalSettings);

drupalSettings isn't used here.

YesCT’s picture

@sun gave a review in #11
So, should this be needs work to address those things?
Or, if it's still at needs review because it needs more review, what kind of review is additionally needed, or are there a couple people (@nod_?) in mind folks are hoping will review this specifically?

frega’s picture

FileSize
17.76 KB

Please find a patch attached. Caveat: it is *not* a direct improvement/descendant of #4 (I've conferred w/ @dawehner about this). Instead it tries to bring together the different strands and issues. It is still WIP and more of a prototype as it cobbles together bits and pieces from dawehner's code in comment #4 above, #1667742: Add abstracted dialog to core (resolves accessibility bug) (in particular larowlan's patch in http://drupal.org/node/1667742#comment-6734556) and also some notions from ctools_modal-7.x. If the general direction is ok, I would refactor the code (proper AjaxCommands-objects).

The patch *deliberately* adds a module to modules/dialog_example/ in order to allow for *easy* testing and speccing of this functionality. Please Install the the dialog_example module after applying the patch add visit the dialog_example/ to see the various "use cases".

dawehner’s picture

I appreciate the fact that we have commands for both pure dialogs and modals.
You better expose the full functionality or people will start to get inconsistent again.

Just in general: I'm wonderning whether and how we can explain in documentation the difference
between a dialog and a modal.

+++ b/core/includes/ajax.incundefined
@@ -1160,3 +1160,156 @@ function ajax_command_add_css($styles) {
+function ajax_dialog_form_wrapper($form_id, &$form_state, $dialog_settings=array(), $dialog_selector=FALSE) {
+  // This won't override settings already in.
+  $form_state += array(
+    'rerender' => FALSE,
+    'ajax' => TRUE,
+    'no_redirect' => TRUE,
+  );

It seems to be that this could be somehow moved into fapi directly if we can already know whether the request asks for ajax?
Totally not sure whether it makes sense to put such specific functionality deeply into the system.

+++ b/core/includes/ajax.incundefined
@@ -1160,3 +1160,156 @@ function ajax_command_add_css($styles) {
+  if (is_array($output)) {
+    $output = drupal_render($output);
+  }

I guess we can skip that, drupal_build_form will always return a render array.

+++ b/core/includes/ajax.incundefined
@@ -1160,3 +1160,156 @@ function ajax_command_add_css($styles) {
+  if ($messages = theme('status_messages')) {
+    $output = $messages . $output;
+  }

Should we put that into the AjaxRender class, as it seems to be something fundamental like css/js?

+++ b/core/includes/ajax.incundefined
@@ -1160,3 +1160,156 @@ function ajax_command_add_css($styles) {
+ * @note: I'd say this is an improvement on the replacements "nojs<->ajax" � la
+ * D7/ctools URL

Wouldn't this maybe even fix the problem that some JS errors lead to json responses? This is a huge WIN from my perspective. Also the DX seems to be improved a lot. I'm wondering whether this should be a method on the content_negotation service, but it seems to be to specific.

+++ b/core/includes/ajax.incundefined
@@ -1160,3 +1160,156 @@ function ajax_command_add_css($styles) {
+function ajax_is_xhr_request() {

Do people know that an "ajax" request uses xhr requests internally? Maybe we could improve the name.

+++ b/core/includes/ajax.incundefined
@@ -1160,3 +1160,156 @@ function ajax_command_add_css($styles) {
+  return NULL;

nitpick: So it's not a xhr request, let's return FALSE.

+++ b/core/includes/ajax.incundefined
--- a/core/misc/ajax.js
+++ b/core/misc/ajax.jsundefined

In general it seems to be that we should split up the ajax.js file to have all that dialog logic in a separate file.

+++ b/core/misc/dialog.ajax.jsundefined
@@ -0,0 +1,56 @@
+      $dialog = $('<div id="' + response.selector.replace(/^#/, '') + '"/>').appendTo('body');

Do we use Drupal.theme for something like that?

+++ b/core/misc/dialog.jsundefined
@@ -48,7 +48,8 @@ Drupal.dialog = function (element, options) {
+  // Extend the default settings in drupallSettings with the local options.

drupall, what could that be ;)

We seem to miss an equivalent for closing a modal

frega’s picture

FileSize
19.85 KB
26.64 KB

Rerolled the patch to address a few of the points above, adding & adjustings tests as well converting the "old" style ajax commands to D8's Command objects/AjaxResponse functionality.

Just in general: I'm wonderning whether and how we can explain in documentation the difference
between a dialog and a modal.

A modal is a singleton blocking the UI whilst (normal) dialogs stack and do not block the UI.

Re: moving ajax_dialog_form_wrapper deeper into FAPI - i feel the same and am not sure what the best way to proceed is. The current approach here is similar to the ctools_modal one which is sort of proven to work.

I guess we can skip that, drupal_build_form will always return a render array.

Fixed.

Should we put that into the AjaxRender class, as it seems to be something fundamental like css/js?

Good point, but maybe in a follow up?

nitpick: So it's not a xhr request, let's return FALSE.

Fixed.

In general it seems to be that we should split up the ajax.js file to have all that dialog logic in a separate file.

Leaving the dialog "insertion" logic in ajax.js seemed to be something effulgentsia wanted. I am not sure about it myself. But the form-"wiring" certainly should be moved to a separate function.

We seem to miss an equivalent for closing a modal

CloseModalDialogCommand and CloseDialogCommand are *very* similar so i felt it was overkill but am happy to add it.

Status: Needs review » Needs work
Issue tags: -VDC, -Spark

The last submitted patch, 1870764-15.patch, failed testing.

frega’s picture

Status: Needs work » Needs review
Issue tags: +VDC, +Spark

#15: 1870764-15.patch queued for re-testing.

Wim Leers’s picture

Status: Needs review » Needs work

First: the included dialog_example module is a godsend for reviewing this patch — thanks :)

Overall: works well. Except for once, where I had opened the "Complex Form [modal]" case, clicked "Okayish" and it went to the non-AJAX fallback page. But I can't reproduce that, so maybe I'm imagining things.

Initial round of feedback, a bunch of nitpicks, but also a bunch of bigger questions/concerns.

+++ b/core/includes/ajax.incundefined
@@ -1160,3 +1163,90 @@ function ajax_command_add_css($styles) {
+function ajax_is_xhr_request() {

ajax_is_ajax_request() is a better name because you're checking for ajax, not xhr?

+++ b/core/lib/Drupal/Core/Ajax/CloseDialogCommand.phpundefined
@@ -0,0 +1,54 @@
+class CloseDialogCommand implements CommandInterface {

(This is related to what effulgentsia said at #1851414-6: Convert Views to use the abstracted dialog modal.)

How will this be used? Well, you'll send some data to the server (typically a form), and based on the result, it is decided whether the dialog should be closed (e.g. no form validation errors, so the dialog's job is done).
But why send back an extensive AJAX command, when a small JSON response would be sufficient for the JS to figure out whether to close the dialog or not?

I definitely see there are use cases for this (when you don't want to write any custom JS, for example), but it still feels very strange to see this kind of thing.

We can improve those things down the road, right now we need to move forward. I just wanted to note that I share effulgentsia's concerns.

+++ b/core/lib/Drupal/Core/Ajax/CloseDialogCommand.phpundefined
@@ -0,0 +1,54 @@
+  /**

Newline before this. That's the convention.

+++ b/core/lib/Drupal/Core/Ajax/CloseDialogCommand.phpundefined
@@ -0,0 +1,54 @@
+   * A settings array to be passed to any any attached JavaScript behavior.

"any any"

+++ b/core/lib/Drupal/Core/Ajax/CloseDialogCommand.phpundefined
@@ -0,0 +1,54 @@
+  protected $settings;

Do we really need to pass settings here? It makes sense for InsertCommand and subclasses, because new HTML content is being added, which may very well contain "behavioral HTML".

But for a *close* command?

+++ b/core/lib/Drupal/Core/Ajax/CloseDialogCommand.phpundefined
@@ -0,0 +1,54 @@
+   * Constructs an CloseDialogCommand object.

s/an/a/

+++ b/core/lib/Drupal/Core/Ajax/CloseDialogCommand.phpundefined
@@ -0,0 +1,54 @@
+   * @internal param string $selector A CSS selector.*   A CSS selector.

Broken comment.

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,91 @@
+   * @var string

bool?

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,91 @@
+   * Stores dialog specific settings.

dialog-specific

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,91 @@
+   * @param array $dialog_settings
+   *   An array of JavaScript settings to be passed to the dialog implementation.
+   * @param bool $modal
+   *   Open a modal dialog
+   * @param array $settings
+   *   An array of JavaScript settings to be passed to any attached behaviors.
+   *   @todo: do we need to duplicate $settings and $dialog_settings?

If $dialog_settings is about "JavaScript settings", then it's a subarray of $settings?

If we want to keep $dialog_settings, then it should really be data that gets passed as a parameter to the JS function that instantiates a dialog, otherwise we're duplicating/confusing things.

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,91 @@
+

Delete newline.

+++ b/core/lib/Drupal/Core/Ajax/OpenModalDialogCommand.phpundefined
@@ -0,0 +1,36 @@
+class OpenModalDialogCommand extends OpenDialogCommand {

Why have a separate AJAX command? Why not have a $(is_)modal parameter for OpenDialogCommand?

+++ b/core/misc/dialog.ajax.jsundefined
@@ -0,0 +1,57 @@
+ * Integrates the dialog API with the drupal AJAX commands.

Drupal

+++ b/core/misc/dialog.ajax.jsundefined
@@ -0,0 +1,57 @@
+    // Setup the wrapper, if there isn't one.

Set up

dawehner’s picture

How will this be used? Well, you'll send some data to the server (typically a form), and based on the result, it is decided whether the dialog should be closed (e.g. no form validation errors, so the dialog's job is done).
But why send back an extensive AJAX command, when a small JSON response would be sufficient for the JS to figure out whether to close the dialog or not?

So what views does is to store a list of forms it wants the user to fill out. This is used for example if you add multiple fields in a row.
As the php side of views stores the forms which still have to be filled out, it also has to control to close the actual form at some point.
I think an extra ajax command is the best and clean way to handle that.

nod_’s picture

in dialog.ajax.js: Drupal.ajax.prototype.commands.openDialog(ajax, response, status); should be ajax.commands.openDialog(ajax, response, status); since you could override the command for a specific ajax object, same for insert.

And yeah the option merging needs to be looked closer, I got the order wrong :p

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.04 KB
   * @todo: determine visibility is public, in order to allow for altering

If you want to allow altering you should implement getter/setters and then use request subscribers to alter it.

Why have a separate AJAX command? Why not have a $(is_)modal parameter for OpenDialogCommand?

I kept it for better implicit documentation what is going on.

Fixed the problems from the review, and fixed some TODOs.

dawehner’s picture

FileSize
19.11 KB

Forgot to include some files.

dawehner’s picture

FileSize
26.45 KB

dawehner--

Status: Needs review » Needs work

The last submitted patch, drupal-1870764-23.patch, failed testing.

frega’s picture

FileSize
16.21 KB
27.9 KB

dawehner was kind enough to take care of most of the nit-picks.

This patch fixes a small issue in the JS of the patch #23 and (hopefully) allows the tests to run again.

I reworked some of the function signatures to be a cleaner and also added a CloseModalDialogCommand (but no corresponding JS function). I would prefer to actually only have *one* OpenDialogCommand and CloseDialogCommand with "modal" being passed in, but will only do so, if people agree :)

How will this be used? Well, you'll send some data to the server (typically a form), and based on the result, it is decided whether the dialog should be closed (e.g. no form validation errors, so the dialog's job is done).
But why send back an extensive AJAX command, when a small JSON response would be sufficient for the JS to figure out whether to close the dialog or not?

As @dawehner already pointed out in #21 we'll need this approach for complex situations and to be able to add additional AjaxCommands after a submission has taken place (that's what the dialog_example module does by "replacing" the title).

frega’s picture

Status: Needs work » Needs review

Forgot to change status.

Status: Needs review » Needs work
Issue tags: -VDC, -Spark

The last submitted patch, drupal-1870764-25.patch, failed testing.

frega’s picture

Status: Needs work » Needs review
Issue tags: +VDC, +Spark

#25: drupal-1870764-25.patch queued for re-testing.

dawehner’s picture

FileSize
28.72 KB
2.42 KB

Just some pieces of cleanup, we certainly need feedback from effulgentsia

Status: Needs review » Needs work
Issue tags: -VDC, -Spark

The last submitted patch, drupal-1870764-29.patch, failed testing.

frega’s picture

Status: Needs work » Needs review

#29: drupal-1870764-29.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1870764-29.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +VDC, +Spark

#29: drupal-1870764-29.patch queued for re-testing.

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

Assigning to myself, so I remember to review this as soon as I get a chance. Am shooting for later this afternoon or tomorrow.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
Status: Needs review » Needs work

Thanks frega, dawehner, and others for working on this.

My biggest concern is the coupling between forms and dialogs in all of the new code added in ajax.inc and ajax.js. I can see that core's FAPI/AJAX is lacking in its ability to integrate complete forms being returned by ajax that then need to trigger more ajax, and given that Views needs that, I support adding that integration. But I don't yet see why that should be coupled to dialogs. Maybe it's better UX for form wizards to happen in dialogs rather than other kinds of AJAX containers, but I don't like those things being coupled at the API level. I'll try to work on decoupling that, but let's not hold up Views progress on that.

I would prefer to actually only have *one* OpenDialogCommand and CloseDialogCommand with "modal" being passed in, but will only do so, if people agree

For what it's worth, I agree with that.

effulgentsia’s picture

Status: Needs work » Needs review

Oops. Didn't mean to change status. I think this patch could use more work, but I didn't leave enough specific feedback to justify the status change.

effulgentsia’s picture

I opened a spin-off for just the settings/options portion: #1909144: Allow #ajax['dialog'] to contain options other than 'modal' in order to help unblock #1821548: Add a "diff" of some kind to the CMI UI. I may try to spin-off the commands from the FAPI portion as well, to aid the remaining discussion and review process.

YesCT’s picture

I updated the issue summary with a postponed on this and related issues section

YesCT’s picture

Issue summary: View changes

added postponed on this and related issues section

webchick’s picture

Assigned: Unassigned » rfay
Issue tags: -Spark

Since #1818142: [meta] Unified Blocks and Layouts (SCOTCH+WSCCI+Spark+Field UI) isn't happening, untracking from Spark. Also since #1909144: Allow #ajax['dialog'] to contain options other than 'modal' was committed, not sure this is critical anymore?

webchick’s picture

Assigned: rfay » Unassigned

What?

rfay’s picture

That was pretty impressive :-) And giving me the easy stuff too :-)

sun’s picture

Status: Needs review » Needs work

#1851414: Convert Views to use the abstracted dialog modal is still a critical task that is postponed on this, so unless that task's priority is demoted, this one needs to be critical, too.

The code here definitely needs work though. It contains plethora of @todos that we'll have to resolve prior to commit. Also, the ajax.inc as well as ajax.js changes look a bit fishy to me. Can we try to simplify and clean this up as much as possible?

quicksketch’s picture

This issue may also end up blocking #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms. For now I may take the views-ish approach and write a dialog-close AJAX command as part of editor.module, but then we'll end up with two places in core that are providing specialized closing AJAX actions and CSS styling for dialogs. On the other hand, it'll make it clear what requirements we have for making something that actually works in both places

frega’s picture

FileSize
4.08 KB
28.14 KB

Rerolled the patch from #29, minor manual merge, minor cleanup und updating example module dialog_example (.info -> info.yml).

Crell’s picture

Status: Needs work » Needs review
DamienMcKenna’s picture

Issue tags: +SprintWeekend2013

Status: Needs review » Needs work

The last submitted patch, drupal-1870764-44.patch, failed testing.

frega’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
28.12 KB
653 bytes

Another reroll - sorry missed a duplicate "use"-statement in a merged test. Attached interdiff is to #44.

Whilst it would be possible to rewrite views' current modal implementations (or quicksketch's in ckeditor), dawehner and I agreed that this issue needs some architectural feedback before doing that; having a modal/dialog cater for more complex use-cases will inevitably straddles FAPI, routing and JS - so feedback welcome :)

quicksketch’s picture

Status: Reviewed & tested by the community » Needs review

This looks like a great patch @frega! I'll give this a review later today and try to utilize it in #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms and see if I run into any shortcomings. This patch seems like it was accidentally marked RTBC; there are still a number of @todos in the code and hasn't gotten enough review yet.

quicksketch’s picture

Status: Needs review » Needs work

I'm taking a stab at rerolling this patch and I'm finding a lot of areas for improvement. I don't think the changes will be drastic code-wise, but we have some strange workarounds happening between the AJAX framework and the dialog framework. This probably happened because the dialog API happened first, without the AJAX commands.

The root of the problem stems from trying to define the response behavior inside of $element['#ajax']. Properties on the #ajax element beyond 'callback' is a legacy approach. When the D6 AJAX framework was written, we intended everything to be controlled through settings on the #ajax property. However, the D7 revamp of the AJAX framework introduced by @merlinofchaos shifted the functionality from being defined in the element to being defined in the AJAX response. The new approach is drastically more flexible, because you can tell the client-side to do *anything* you want, and it's not all bound up in a never-expanding set of FAPI properties. This newer post-D7 version of the framework provides lots of functionality that isn't otherwise supported using the #ajax property such as removing HTML in addition to adding it, adding CSS/JS, restriping tables, etc (basically everything in /core/lib/Drupal/Core/Ajax). We don't have FAPI properties for any of those things, and we shouldn't. The same thing applies to dialog functionality.

So... what should happen here is that we should remove the #ajax['dialog'] property entirely. Trying to have the build element control the response behavior is a huge mess, and it requires a bunch of modifications to ajax.js. If we switch to using AJAX commands only, we can remove all these nasty hacks we've inserted into ajax.js that deal with dialogs. There's an uncomfortable amount of interdependency right now that can be remove entirely.

Anyway, working on it now. I'll post when I get everything working again. I chatted with @dawehner in IRC and he seems to support the idea also. Considering AJAX commands is the way both Views UI and Editor module are using right now, so far 100% of our core dialog implementations prefer AJAX commands over #ajax properties.

larowlan’s picture

Which brings us right back to where we were in November in #1667742-45: Add abstracted dialog to core (resolves accessibility bug) (patch at comment 45)- using ajax commands instead of FAPI properties. Oh well, sometimes its the journey as much as the destination.

sun’s picture

Hm.

We need to differentiate between 1) triggering/opening a dialog, and 2) interacting within a dialog.

For 1), what we currently have with #ajax][dialog seems to be most appropriate, since it's all about triggering the thingie only.

The needs for 2) may vary in multiple ways. I'm not sure whether our current toolset is sufficient for that.

What you're essentially trying to do is to implant a Command & Conquer™ facility to trigger and control dialog operations into... server-side form/render arrays and/or Ajax commands. Regardless of how we design it, that's a daunting task.

Since all operations within a dialog apparently mean that we're operating in a JS environment already, one would normally assume that this is something the front-end should primarily take over (e.g., a backbone micro-app that's able to properly set and track state from the client-side, and which sends and receives data to/from the backend), since realistically, the backend is not able to reliably track the dialog's state.

I'd recommend to explore in that direction instead. @frega, @nod_, and others are most likely happy to help with the frontend plumbing.

larowlan’s picture

Further to @sun's points at #52, option (1) (triggering a dialog) is being used by CMI to display the diff in their UI so I don't think it is an option to remove it. i.e. we need both.
I agree that using the client-side to track dialog state makes more sense. One issue I can see with that is how would we go about making it degrade gracefully for non-js users?.

quicksketch’s picture

Further to @sun's points at #52, option (1) (triggering a dialog) is being used by CMI to display the diff in their UI so I don't think it is an option to remove it. i.e. we need both

I can assure you that using AJAX-style commands will work in every situation that #ajax-based elements worked. This new version will also still not make dialog.js dependent upon ajax.js, so if you have a non-AJAX situation, you won't need to use AJAX commands at all.

I agree that using the client-side to track dialog state makes more sense. One issue I can see with that is how would we go about making it degrade gracefully for non-js users?.

The same way that Views works already. Every single page has a "nojs" version that is used by default. It's only when ajax.js goes through and replaces all "use-ajax" links on the page that "nojs" is replaced with "ajax" in the URLs, so every link that is visited immediately knows if it need to return an AJAX command or just normal page content.

We're not really doing anything new here. We're just adjusting the Views/CTools approach to dealing with modals and using the same thing in core.

One huge advantage I've seen with this approach is that the dialog.js file doesn't need to be loaded on the page until a dialog is necessary. Because ajax.js already dynamically adds necessary JS/CSS, the dialog.js file is loaded *on demand* when a dialog command comes back. Better yet, this means that if your initial page load doesn't have jQuery UI yet, even *that* is all lazy-loaded only when you need it. The bandwidth/requests savings is fantastic.

I have everything working already at this point, I'm just revising our example module for testing purposes to demonstrate the full gamut of use-case scenarios.

quicksketch’s picture

Followup to my own comment:

so every link that is visited immediately knows if it need to return an AJAX command or just normal page content.

The patch in #48 also includes ajax_is_ajax_request() which determines whether the page is an AJAX request by the header, but I haven't fully investigated/implemented that into my approach yet. It's probably a good idea. It'd be nice not to have "nojs" or "ajax" in all your AJAX URLs.

dawehner’s picture

The same way that Views works already. Every single page has a "nojs" version that is used by default. It's only when ajax.js goes through and replaces all "use-ajax" links on the page that "nojs" is replaced with "ajax" in the URLs, so every link that is visited immediately knows if it need to return an AJAX command or just normal page content.
,/blockquote>
One detail I really like about this patch is that you removes this behaviour and triggers different behaviour, dependent of the asked content in http.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
36.13 KB

Okay here's the new patch so far. I didn't get through updating all the previous examples in the dialog_example module. I did however get through all of the ajax_test.module, so to see more examples comments out the hidden: true in the ajax_test.info.yml to get more working examples.

The basics of this patch:

  • The #ajax['dialog'] property is entirely removed. This means that we don't have any interdependency between ajax.js and dialog.js. They're now completely separate.
  • Instead of using #ajax['dialog'], you use #ajax['callback'] to specify a JavaScript-only callback function. In the submit handler for this callback function, you can tell a dialog to open with the form contents.
  • Instead of hijacking all the buttons in the form that is loaded into the dialog, the form definition must be manually updated to support working in a modal. For example a button that wants to close the modal and submit normally needs to detect if its in a modal, and if so, provide an #ajax['callback'] property. This callback is then responsible for executing submit handlers or providing alternative behavior. Then it can either close the dialog, or do whatever functionality is applicable to that particular situation.
  • All special wrapper functions around drupal_build_form() and such have been removed. Using the above approach, they're no longer needed as far as I have reproduced. I haven't covered *every* situation yet, but I'm fairly sure we'll be able to cover every use-case at least Views and Editor with this approach, as its endlessly flexible.
  • New AJAX commands were added: SetDialogOptionCommand, SetDialogTitleCommand, and SetWindowLocation. The test module uses SetDialogOptionCommand, I needed SetDialogTitleCommand for Editor module, and to replicate the previous behavior of "Submit, close the dialog, then redirect" we need SetWindowLocation.
  • dialog.js has been modified to be prototype-object based instead of a generic object. We need to keep the dialog element as part of the object, and converting the approach both reduces code and solves this requirement.

Git has been giving me trouble generating the interdiff for this one, so here are the most important summary points.

For links, instead of using #ajax (which was never supported on links before as far as I know, maybe it was?) you set the 'use-ajax' class on the link. You can optionally include the special "nojs" string in your URL, which will be replaced with "ajax" by ajax.js. This is no different than any other AJAX link in use today, we're just adopting the current behavior for modals also.

@@ -131,10 +122,8 @@ function ajax_test_dialog() {
   $build['link'] = array(
     '#type' => 'link',
     '#title' => 'Link 1 (modal)',
-    '#href' => 'ajax-test/dialog-contents',
-    '#ajax' => array(
-      'dialog' => array('modal' => TRUE),
-    ),
+    '#href' => 'ajax-test/nojs/dialog-contents/1',
+    '#attributes' => array('class' => array('use-ajax')),
   );

For buttons, we do the same thing. Instead of making up a new #ajax['dialog'] property, we use the existing #ajax['callback'] property:

   $form['button2'] = array(
     '#type' => 'submit',
     '#value' => 'Button 2 (non-modal)',
     '#ajax' => array(
-      'dialog' => array(),
-      'wrapper' => 'ajax-test-dialog-wrapper-1',
+      'callback' => 'ajax_test_dialog_form_callback_nonmodal',
     ),
   );

However by shifting all this code from the build stage to the response, menu handlers and AJAX callbacks are going to need to specifically state what functionality should be done upon clicking/submitting. There is no magically ability to just point at any form and say "put this in a dialog". Each form has to be updated to specifically handle what happens if it's been put into a modal by adding a #ajax['callback'] property on its buttons. This is more slightly more tedious, but can be mitigated by eventually updating the confirm_form() function to globally support modals. Since confirmation forms are one of our most likely candidates, after updating it, enabling modal confirmation forms will be a 1-3 line affair on each place where we want to use a confirmation modal.

Removing the magic behavior also makes it so that our modals work in more situations. The previous approach (as far as I could tell) hijacked ALL the buttons in the form. This meant that if you had existing buttons that had different AJAX behaviors, such as a file upload field or the "Add another" button in a modal, it wouldn't be supported by the magic behavior. Now that every button in the form is specifically targeted with a different #ajax['callback'] property, we don't have these conflicts any more. The #ajax['dialog'] approach also wouldn't have worked for Editor module, since we don't actually want to submit the form in a traditional sense: we need the specific values in the form to be inserted into the editor rather than redirecting the user to some other location.

This patch still needs some work and will definitely fail tests. I'm setting to needs review just to get an idea of the damage to tests, and some general review would also be greatly appreciated.

quicksketch’s picture

One thing that *did not* change from previous patches: ajax_is_ajax_request(). This handy utility function can detect if you're in an AJAX request directly without using the swap "nojs"/"ajax" trick. So if you don't want to update your menu paths, you can still use this method to detect an AJAX/dialog request. This additional bit of flexibility is really handy, though not entirely a necessity.

Status: Needs review » Needs work

The last submitted patch, ajax_dialogs-1870764-51.diff, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
37.58 KB
2.91 KB

Should fix tests

larowlan’s picture

Turns out we don't need ajax_is_ajax_request.

quicksketch’s picture

Turns out we don't need ajax_is_ajax_request.

@larowlan pointed out to me in IRC that Drupal::service('request')->isXmlHttpRequest() already provides this functionality. So the new patch removes the function because it's not necessary. IMO at this point we should go ahead and remove the demo module, moving what's testable into the ajax_test.module and what's not potentially into the examples project. However Views is probably the most complicated situation we'll experience, it should provide an extensive example itself. Other areas like the filter tips would also be easy wins.

jibran’s picture

Issue tags: +modal dialog

Tagging.

quicksketch’s picture

Mostly the same page as #62 but tidying up:

- dialog_example module removed.
- Changed dialogSettings to dialogOptions for consistency (dialog.js and jQuery UI use the term options, not settings).
- Removed changes to confirm_form(). Confirm form modifications will be its own issue.
- Minor docblock updates.

quicksketch’s picture

FileSize
29.54 KB
3.99 KB

Hm, missed a few settings => options renaming.

Status: Needs review » Needs work

The last submitted patch, ajax-dialog-1870764.65.diff, failed testing.

larowlan’s picture

Added followup for cleaning up drupal_get_js to use Drupal::service('request')->isXmlHttpRequest() too #1941288: Regression: ajaxPageState not being updated with AjaxResponse, assets (js/css) being added twice

quicksketch’s picture

Status: Needs work » Needs review
FileSize
35.57 KB
8.48 KB

Fixes the two broken tests.

Adds new test coverage for dialog commands. Hats off to @effulgensia and @sun for the creation of WebTestBase::drupalPostAJAX(). That actually makes it so that we can get a reasonably accurate emulation of an AJAX response for opening a dialog. Unfortunately it only works for POST requests sent by a form (rather than a link) so we can only use it for the dialog-opening buttons. The links on the page use drupalGetAJAX(), which isn't quite as accurate a simulation, but still suitable for checking our specific commands.

The previous test for dialogs was kind of comical: A full WebTestBase installation of Drupal that only went to a single URL then a @todo for "add some tests". :)

quicksketch’s picture

FileSize
35.59 KB
2.27 KB

Dang, a few comment typos. Sorry testbot.

twistor’s picture

Looking good, and with that, nitpicks:

diff --git a/core/includes/ajax.inc b/core/includes/ajax.inc
index 212dff6..298f0ec 100644
--- a/core/includes/ajax.inc
+++ b/core/includes/ajax.incundefined
@@ -5,6 +5,9 @@
  * Functions for use with Drupal's Ajax framework.
  */
 
+use Drupal\Core\Ajax\OpenDialogCommand;
+use Drupal\Core\Ajax\OpenModalDialogCommand;
+
 /**

Why is this being added?

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,105 @@
+   * @param string $html
+   *   String of HTML that will be placed in the dialog
+   * @param array $dialog_options

Missing period.

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,105 @@
+   *   jQuery UI option can be used. See http://api.jqueryui.com/dialog.
+   * @param array|null $settings
+   *    An optional array of other settings that will be passed to the behavior.

(Optional)?

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,105 @@
+  public function __construct($selector, $title, $html, array $dialog_options = array(), $settings = NULL) {
+    $dialog_options += array('title' => $title);
+    $this->selector = $selector;
+    $this->html = $html;
+    $this->dialogOptions = $dialog_options;
+    $this->settings = $settings;
+  }

All of these properties need to be declared.

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,105 @@
+      'data' => $this->html,
+      'dialogOptions' => $this->dialogOptions
+    );
+  }

Trailing comma, there are lots of these.

+++ b/core/lib/Drupal/Core/Ajax/OpenModalDialogCommand.phpundefined
@@ -0,0 +1,33 @@
+   * Constructs an OpenModalDialog object.
+   *
+   * @param string $title
+   *   The title of the dialog.
+   * @param string $html
+   *   String of HTML that will be placed in the dialog
+   * @param array $dialog_options

Trailing period.

+++ b/core/lib/Drupal/Core/Ajax/OpenModalDialogCommand.phpundefined
@@ -0,0 +1,33 @@
+   *   jQuery UI option can be used. See http://api.jqueryui.com/dialog.
+   * @param array|null $settings
+   *    An optional array of other settings that will be passed to the behavior.
+   */

(Optional)?

+++ b/core/lib/Drupal/Core/Ajax/OpenModalDialogCommand.phpundefined
@@ -0,0 +1,33 @@
+  public function __construct($title, $html, array $dialog_options = array(), $settings = NULL) {
+    $dialog_options['modal'] = TRUE;
+    return parent::__construct('#drupal-modal', $title, $html, $dialog_options, $settings);
+  }

Returning the constructor?

+++ b/core/lib/Drupal/Core/Ajax/SetWindowLocationCommand.phpundefined
@@ -0,0 +1,46 @@
+  /**
+   * The URL that will be loaded into window.location.
+   *
+   * @var array
+   *

Empty line.

+++ b/core/misc/dialog.ajax.jsundefined
@@ -0,0 +1,63 @@
+  Drupal.ajax.prototype.commands.closeDialog = function (ajax, response, status) {
+    var $dialog = $(response.selector) || $('#drupal-modal');
+    if ($dialog.length) {
+      var dialog = new Drupal.dialog($dialog);
+      dialog.close();
+    }

Don't we set #drupal-modal as the default already?

+++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/AjaxCommandsUnitTest.phpundefined
@@ -305,5 +312,121 @@ function testSettingsCommand() {
+    $command = new OpenDialogCommand('#some-dialog', 'Title', '<p>Text!</p>', array(
+      'url' => FALSE,
+      'width' => 500
+    ));

Trailing commas.

quicksketch’s picture

Thanks @twistor! I think you're right on all counts. Will reroll.

quicksketch’s picture

Everything in #70 addressed. Expanded out docblock a little bit and code style fixes.

twistor’s picture

I guess I didn't do a very good job the first time.

+++ b/core/lib/Drupal/Core/Ajax/CloseModalDialogCommand.phpundefined
@@ -0,0 +1,22 @@
+/**
+ * @file
+ * Contains \Drupal\Core\Ajax\CloseDialogCommand.
+ */
+
+namespace Drupal\Core\Ajax;
+
+use Drupal\Core\Ajax\CloseDialogCommand;
+
+/**
+ * Defines an AJAX command that closes the currently visible modal dialog.
+ */
+class CloseModalDialogCommand extends CloseDialogCommand {
+  /**
+   * Constructs a CloseDialogCommand object.
+   */
+  public function __construct() {
+    $this->selector = '#drupal-modal';
+  }

The references to CloseDialogCommand are incorrect. Also, if we're hardcoding #drupal-modal here, why are we setting it as a default in the parent class? It's odd that CloseDialogCommand would set its default to #drupal-modal.

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,133 @@
+   * @param array|null $settings
+   *   (optional) Other settings that will be passed to the behavior.
+   */
+  public function __construct($selector, $title, $html, array $dialog_options = array(), $settings = NULL) {

Perhaps the other settings could be documented?

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,133 @@
+  public function getdialogOptions() {
+    return $this->dialogOptions;
+  }

CamelCase.

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,133 @@
+   * @param array $dialog_options
+   *   An array of keys passed to the Drupal.dialog javascript object.

Could $dialog_options be documented?

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,133 @@
+  public function setdialogOptions($dialog_options) {
+    $this->dialogOptions = $dialog_options;
+  }

CamelCase.

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,133 @@
+  /**
+   * Sets the dialog title (an alias of setDialogOptions).
+   *
+   * @param mixed $title
+   *   The new title of the dialog.

Title isn't a string?

+++ b/core/lib/Drupal/Core/Ajax/OpenModalDialogCommand.phpundefined
@@ -0,0 +1,38 @@
+/**
+ * Defines an AJAX command to open certain content in a dialog in a modal dialog.
+ */

Comment is 81 characters.

+++ b/core/lib/Drupal/Core/Ajax/OpenModalDialogCommand.phpundefined
@@ -0,0 +1,38 @@
+   * @param array|null $settings
+   *   (optional) Other settings that will be passed to the behavior.
+   */

Document $settings?

+++ b/core/lib/Drupal/Core/Ajax/SetDialogOptionCommand.phpundefined
@@ -0,0 +1,65 @@
+class SetDialogOptionCommand implements CommandInterface {
...
+  /**
+   * Constructs a CloseDialogCommand object.
+   *

SetDialogOptionCommand not CloseDialogCommand.

+++ b/core/lib/Drupal/Core/Ajax/SetDialogOptionCommand.phpundefined
@@ -0,0 +1,65 @@
+   * @param string $selector
+   *   The selector of the dialog to close.
+   * @param string $option_name
+   *   The name of the option to set. May be any jQuery UI dialog option.
+   *   See See http://api.jqueryui.com/dialog.
+   * @param mixed $option_value
+   *   The value of the option to be passed to the dialog.
+   */
+  public function __construct($selector = NULL, $option_name, $option_value) {
+    $this->selector = $selector ? $selector : '#drupal-modal';
+    $this->option_name = $option_name;
+    $this->option_value = $option_value;

Arguments trailing a default argument. Is that kosher? Also, (optional).

+++ b/core/lib/Drupal/Core/Ajax/SetWindowLocationCommand.phpundefined
@@ -0,0 +1,45 @@
+class SetWindowLocationCommand implements CommandInterface {
...
+  /**
+   * Constructs an OpenDialog object.
+   *
+   * @param string $url
+   *   The URL that will be loaded into window.location. This should be a full
+   *   URL, one that has already been run through the url() function.

SetWindowLocationCommand not OpenDialog.

+++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/DialogTest.phpundefined
@@ -13,20 +13,87 @@
+      'name' => 'AJAX dialogs commands',
+      'description' => 'Performs tests on opening and manipulate dialogs via AJAX commands.',
       'group' => 'AJAX',

manipulating

larowlan’s picture

+++ b/core/lib/Drupal/Core/Ajax/CloseDialogCommand.phpundefined
@@ -0,0 +1,41 @@
+    $this->selector = $selector ? $selector : '#drupal-modal';

can't this be $this->selector = $selector then make the default '#drupal-modal' instead of NULL?

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,133 @@
+   *   (optional) Options to be passed to the dialog implementation. Any

Nitpick, should include 'defaults to array()', same story for $settings

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.phpundefined
@@ -0,0 +1,133 @@
+  public function setdialogOptions($dialog_options) {

setDialogOptions instead?

+++ b/core/lib/Drupal/Core/Ajax/SetDialogOptionCommand.phpundefined
@@ -0,0 +1,65 @@
+  protected $option_name;

these should be camel case

+++ b/core/lib/Drupal/Core/Ajax/SetDialogOptionCommand.phpundefined
@@ -0,0 +1,65 @@
+  protected $option_value;

same

+++ b/core/lib/Drupal/Core/Ajax/SetDialogOptionCommand.phpundefined
@@ -0,0 +1,65 @@
+    $this->selector = $selector ? $selector : '#drupal-modal';

Same here, can't we use a sensible default in the arguments?

Apart from that and the points from @twistor, I think this is RTBC, plus with actual tests now too ;)

quicksketch’s picture

FileSize
36.71 KB
8.93 KB

Fixes everything from #73 and #74. Some exceptions:

can't this be $this->selector = $selector then make the default '#drupal-modal' instead of NULL?

Unfortunately if you pass in a NULL value in PHP, it blows away any default specified in the function declaration. So using $this->selector = $selector ? $selector : '#drupal-modal'; is the only way to do this. However there's no point in making the first argument "optional" if you have to fill out the second parameter, so I removed the argument default and updated the PHPdoc to specifically state that you may pass in an empty value to affect the modal dialog.

Nitpick, should include 'defaults to array()', same story for $settings

This doesn't seem to be a coding standard. Existing counts of such references in core:
- Defaults to array() - 2 counts
- Defaults to empty array - 2 counts
- Defaults to an empty array - 10 counts
- Functions that contain a default of an empty array (estimated by regex search) - 287 counts

This is inline with our documentation on coding standards: http://drupal.org/coding-standards/docs#param

Optional parameters are indicated by (optional); include information about the default value only if it is not obvious from the function signature.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +JavaScript

You're right on the coding standards points at #75.
I think this is RTBC but tagging as JavaScript to get more eyes on it.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Didn't mean to do that until after reviews from JS team.

larowlan’s picture

nod_’s picture

Nice that seems to work well :)

Few things:

  1. Why was Drupal.dialog transformed to a constructor? The ajax bits doesn't that fact at all. I wanted a simple API I'd prefer not adding a constructor for that since it's not required.
  2. Usually to have default parameter || is used instead of ? :, options = options || {};
  3. Put back the show/showModal API (that comes from the HTML5 spec)
  4. Moved the show stuff after populating the dom with the modal content (to avoid having to recalculate something once the content starts showing up).
  5. Moved the .cancel handling to dialog.ajax.js, I never liked having that in dialog.js. The purpose of this file is to be replaced by a HTML5 polyfill at some point. The cancel stuff is clearly Drupal.

Couple of comments:

  • naming: setWindowLocation is pretty verbose, wouldn't location or redirect work better?

it appears that the ajax stuff doesn't update ajaxPageState anymore and the dependencies get loaded over and over. Don't think it's related to this patch.

quicksketch’s picture

Why was Drupal.dialog transformed to a constructor?

Just from looking at the code, it was screaming at me: this is an object. Drupal.dialog() was nothing but a wrapper around several variables and two callback functions. If this is intended to be a polyfill of sorts for HTML5, any native-DOM based dialog is definitely going to be a real object of a particular class, not a generic object that is used as a closure around a couple of functions. The API is still simple, it's just using a defined object instead of a generic one.

Looks like this latest patch undoes those changes. I'm not particularly a fan of the reverting, but since it turned out storing the element as part of the dialog object isn't really necessary after all, it's an unnecessary change for this patch.

naming: setWindowLocation is pretty verbose, wouldn't location or redirect work better?

Location seemed too vague to me, so I used WindowLocation to be clear. I didn't think of Redirect, which would be better and shorter. I'll reroll.

quicksketch’s picture

- Renamed SetWindowLocationCommand to RedirectCommand.
- Fixed a few dialog.js whitespace indentations introduced in #79.
- Reformatted the comment on Drupal.behaviors.dialog.attach() to fit in the 80 character bar.

nod_’s picture

Looking RTBC to me :)

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Agreed

podarok’s picture

+1RTBC
very nice code with a good documentation
happypush )

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

The #ajax['dialog'] property is entirely removed.

config_admin_sync_form() needs to be fixed then. Currently, clicking on "View differences" on the admin/config/development/sync page results in the AJAX spinner starting and stopping, but the contents not being displayed anywhere.

quicksketch’s picture

Status: Needs work » Needs review
Issue tags: -VDC
FileSize
38.94 KB
2.83 KB

Thanks @effulgentsia. I didn't know that had happened already. Fixed their usage. Although more verbose, I think it demonstrates customizing the page based on if it's shown in a modal or not. For example we use the title element as the modal title and only add the "dialog-close" class if it's applicable. I can make a shorter implementation if needed, but now that we have this ability to adjust contextually for the dialog we should encourage people to use it.

effulgentsia’s picture

Issue tags: +VDC
+++ b/core/modules/config/config.admin.inc
@@ -175,10 +176,29 @@ function config_admin_diff_page($config_file) {
+  // Return AJAX requests as a dialog.
+  if (Drupal::service('request')->isXmlHttpRequest()) {
+    // Add class to the close link.
+    $output['back']['#attributes']['class'][] = 'dialog-cancel';
+
+    $dialog_content = drupal_render($output);
+    $response = new AjaxResponse();
+    $response->addCommand(new OpenModalDialogCommand($title, $dialog_content, array('width' => '700')));
+    return $response;
+  }
+  // Otherwise show the page title as an element.
+  else {
+    $output['title'] = array(
+      '#theme' => 'html_tag',
+      '#tag' => 'h3',
+      '#value' => $title,
+      '#weight' => -10,
+    );

What I don't like about this is that this changes config_admin_diff_page() to:
a) Care about whether it's being returned as an Ajax response or an HTML page.
b) Decide that if an Ajax response, then as a dialog.

I see that as regressing a separation of concerns that we worked hard to achieve in D7 and improve in D8: that page callbacks (and the _content controllers they're slowly being turned into in other issues) just worry about the content to return, and other code (that can be reused across many page callbacks / _content controllers) deals with packaging for HTML, Ajax, or whatever other format is wanted, and if Ajax, whether to put it in a dialog or in some other wrapper.

but now that we have this ability to adjust contextually for the dialog we should encourage people to use it

I understand that in some complex cases like Views, we may need the individual page callback to make these decisions, but I'm concerned about making that the pattern for all the simple cases like the config diff ui.

quicksketch’s picture

What I don't like about this is that this changes config_admin_diff_page() to:
a) Care about whether it's being returned as an Ajax response or an HTML page.

Even in this simple case of the diff page, the contents of the page *should* be different based on whether its shown in a modal. Although in this particular example, the only thing that needs be changed is the addition of the "dialog-close" class. However, it's a change nonetheless. By explicitly modifying the form that is returned to function in a dialog correctly, we're removing a lot of magic and interdependencies. After this patch, jQuery UI is only added after clicking on the "View Differences" link, whereas before the page required jQuery UI (base, draggable, dialogs, etc) as well as dialog.js all before a dialog was even used. Now it only requires ajax.js and the rest is loaded on demand. Although it's slightly more PHP code, we have a huge boost in client-side efficiency.

b) Decide that if an Ajax response, then as a dialog.

We could add additional checking that specifically passed in some "isDialog" variable as part of the GET request so AJAX wouldn't necessarily mean "dialog", but in this situation I think it's entirely unnecessary. If something is being returned via AJAX, it's only a JSON response that gets returned containing all the necessary information for the page. If you wanted some other kind of AJAX-response system for viewing diffs, you could easily ignore the JSON commands that weren't relevant and retrieve the needed information only.

I'm not really sure which part of your comment to address. It sounds like you're arguing for implementation simplicity in the guise of purity of the menu callbacks. If in complicated situations it's okay to modify content based on the response, why would it be considered a regression in simple cases? In both situations we're tailoring the response to the way the data is going to be displayed. This is just removing the magic (note the removed @todo for inappropriately using the 'title' property to determine dialogs in the response) and making it explicit.

If we end up converting all menu callbacks to routers/content controllers (I admit I don't really know what this would mean, I only have a rudimentary understanding of most D8 concepts), it sounds like we could move the manipulation one level up and separate the content of the page from the AJAX commands. It seems like that would be an improvement towards separation and avoid all the magic at the same time. However in the interest of avoiding a rewrite of config.module's callbacks, this patch demonstrates the cleanest implementation we can have without conversion.

quicksketch’s picture

@larowlan pointed out in IRC #1833440: Implement partial matcher based on content negotiation MIME type, which indicates that the new router system can set up different _content callbacks based the requested content type. This seems like what we're looking for: the AJAX request would get a different handler (which would most likely call the "normal" handler and then manipulate it). Then we get the explicit behavior and the clean separation.

effulgentsia’s picture

Hm. I'll think about #89 a bit and will post back here tomorrow.

jibran’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/DialogTest.phpundefined
@@ -13,20 +13,87 @@
-    // @todo What else should we assert?

#1884530: Complete modal dialog tests. is marked as duplicate.

jibran’s picture

Issue summary: View changes

Updated issue summary added a postponed issue

nod_’s picture

After this patch, jQuery UI is only added after clicking on the "View Differences" link, whereas before the page required jQuery UI (base, draggable, dialogs, etc) as well as dialog.js all before a dialog was even used. Now it only requires ajax.js and the rest is loaded on demand. Although it's slightly more PHP code, we have a huge boost in client-side efficiency.

Just want to underline that to me, this justify any amount of workaround or weirdness on the PHP side. As quicksketch said, it's pretty huge.

larowlan’s picture

Here's a wip on how it would be done with routes, doesn't work for the ajax callback yet but might helps someone see what we're talking about

podarok’s picture

Status: Needs review » Needs work
+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.phpundefined
@@ -0,0 +1,114 @@
+    return new static($container->get('config.storage'), $container->get('config.storage.staging'));

#1875086: Improve DX of drupal_container()->get()
http://drupal.org/node/1929006
this needs update to latest DX

quicksketch’s picture

Status: Needs work » Needs review

@podarok the patch in #94 is not intended to be part of this patch (at least not yet), it's merely demonstrating the concept that could be used instead of the approach in the current patch, which is #86. #94 is a complete patch, #86 is only a demonstration and would need to be incorporated into the existing patch or (hopefully) moved to a followup that converts all the content handlers in config.module at the same time.

larowlan’s picture

Re #95, I believe that is the correct approach when the container is injected, this is in the controller create method and is consistent with the recent changes.

effulgentsia’s picture

Thanks for #94! I like that direction, and furthermore, would like the diffDialog() controller further genericized into a dialog() controller that can be reused for many simple dialog routes. I'm okay with us proceeding with #86 here, and doing #94 in a follow up, if that would help unblock progress on Views. But if we do that, I think it would make sense to keep #1842036: [META] Convert all confirm forms to use modal dialog postponed until after that follow up.

quicksketch’s picture

the diffDialog() controller further genericized into a dialog() controller that can be reused for many simple dialog routes

Yeah this route would allow any path in Drupal to be "dialogified" simply by adding a second route at the same URL and enabling AJAX on the link that should open the dialog. @effulgentsia and I talked about adding some kind of ability at the time that you specify the AJAX link to also specify the request type, so the link could specifically request a dialog back from Drupal and use that particular route/content handler. This way we'd get pretty much everything "good" and nothing "bad" about the new system: complete separation between the original content handler and the dialog handler, easy enabling of any link to become a dialog (using the generic "wrapper" content handler), and the ability to use your own specialized content handler for advanced dialogs. Plus we get consistency and lazy-loading of the dialog libraries in both simple and complex use-cases.

However all of that isn't included in this patch. We're not entirely sure the routing system can handle this situation at present, and it's a lot to try to bundle all together into a single issue. This issue does what we set out to do: add AJAX commands for dialogs. However, the server-side handling could be improved by using routes to separate the page content from the dialog and providing a generic content handler that could wrap any page in Drupal. Let's open a followup and add a @todo that can provide a tip for users who look at this for a particular example, since by the time we finished the current approach would only apply to non-routed menu callbacks.

quicksketch’s picture

I also looked into using the #ajax property on links again instead of the 'use-ajax' class, but taking a look at drupal_pre_render_link and ajax_pre_render_element, these functions do not appear to be good candidates for handling links that have no #ajax properties and simply want a class. We don't need any Drupal.settings added to the page for these links. Additionally using #ajax introduces an unnecessary #attached for jquery.form.js, which is needed for form elements but not links. Refactoring these functions would be yet another ball of yarn, so for now just specifying the class 'use-ajax' and the required ajax.js #attached seems significantly more straightforward.

quicksketch’s picture

New issue to implement this fancy-pants generic handler: #1944472: Add generic content handler for returning dialogs

quicksketch’s picture

Here's a minor reroll that references our follow-up issue so that users who look to config.module for an example can be pointed to the issue that will provide our recommended approach (once we get that working).

If the router system provides the functionality advertised, we should be able to convert Views right away. Regardless of the router system, I do know this at least provides everything Editor module needs to move forward with image/link handling; as it won't provide a non-JS version of WYSIWYG functionality and so doesn't need multiple content handlers at the same path.

quicksketch’s picture

If the router system provides the functionality advertised, we should be able to convert Views right away.

Well I didn't quite understand how deep Views went with its custom form handling. It'll take some significant work to undo Views' custom form handling mechanisms and use the approaches @larowlan is investigating in #1944472: Add generic content handler for returning dialogs. However, directly replacing the modal system without rewriting the form handling in Views is almost trivial. I did the full conversion in #1851414-19: Convert Views to use the abstracted dialog modal. We can now knock off two critical tasks. Any takers on RTBC?

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

#1944472: Add generic content handler for returning dialogs now works cleanly on top of this, providing a proof of concept for the new routing approach. Good to go

webchick’s picture

Status: Reviewed & tested by the community » Fixed

AWESOME. So great to see this at RTBC! The code is actually much less complicated than I feared, too. And as a bonus, it fixes the CMI dialog that we accidentally broke in #1843220: Convert form AJAX to new AJAX API.

Committed and pushed to 8.x. Thanks!

Not sure if this needs a change notice or not, since I don't see any changes in the calling code..?

quicksketch’s picture

Closed out #1863478: Split Dialog API and optional integration helpers into separate files as duplicate of this one.

Not sure if this needs a change notice or not, since I don't see any changes in the calling code..?

This change notice no longer applies: https://drupal.org/node/1853388. We removed #ajax['dialog'] in this patch. Should we update that change notice, delete it, or both update and make new one?

jibran’s picture

Title: Add an ajax command which makes it easy to use the dialog API in complex cases » Change notice: Add an ajax command which makes it easy to use the dialog API in complex cases
Status: Fixed » Active
Issue tags: +Needs change record
effulgentsia’s picture

I didn't change http://drupal.org/node/1852224, because I think everything there is still correct. Am I wrong on that?

I updated http://drupal.org/node/1853388 to indicate it has been reverted (do we have a more standard way of doing that?).

We still need a new change notice for the command-based API, so leaving this issue open for that.

larowlan’s picture

Status: Active » Needs review

Draft notice is here: http://drupal.org/node/1989646

Crell’s picture

Title: Change notice: Add an ajax command which makes it easy to use the dialog API in complex cases » Add an ajax command which makes it easy to use the dialog API in complex cases
Status: Needs review » Fixed
Issue tags: -Needs change record

Tweaked the language a little. Looks good now. Yay!

jibran’s picture

Title: Add an ajax command which makes it easy to use the dialog API in complex cases » Change notice: Add an ajax command which makes it easy to use the dialog API in complex cases
Status: Fixed » Needs review
Issue tags: +Needs change record

I think we need to add all the cases.
1. form link element.

  // Dialog behavior applied to a #type => 'link'.
  $build['link'] = array(
    '#type' => 'link',
    '#title' => 'Link 1 (modal)',
    '#href' => 'ajax-test/dialog-contents',
    '#attributes' => array(
      'class' => array('use-ajax'),
      'data-accepts' => 'application/vnd.drupal-modal',
    ),
  );

2. theme_links/form operations element.

  // Dialog behavior applied to links rendered by theme_links().
  $build['links'] = array(
    '#theme' => 'links',
    '#links' => array(
      'link2' => array(
        'title' => 'Link 2 (modal)',
        'href' => 'ajax-test/dialog-contents',
        'attributes' => array(
          'class' => array('use-ajax'),
          'data-accepts' => 'application/vnd.drupal-modal',
          'data-dialog-options' => json_encode(array(
            'width' => 400,
          ))
        ),
      ),
      'link3' => array(
        'title' => 'Link 3 (non-modal)',
        'href' => 'ajax-test/dialog-contents',
        'attributes' => array(
          'class' => array('use-ajax'),
          'data-accepts' => 'application/vnd.drupal-dialog',
          'data-dialog-options' => json_encode(array(
            'target' => 'ajax-test-dialog-wrapper-1',
            'width' => 800,
          ))
        ),
      ),
      'link4' => array(
        'title' => 'Link 4 (close non-modal if open)',
        'href' => 'ajax-test/dialog-close',
        'attributes' => array(
          'class' => array('use-ajax'),
        ),
      ),
    ),
  );

3. buttons/form action element.


  $form['button1'] = array(
    '#type' => 'submit',
    '#name' => 'button1',
    '#value' => 'Button 1 (modal)',
    '#ajax' => array(
      'callback' => 'ajax_test_dialog_form_callback_modal',
    ),
  );
/**
 * AJAX callback handler for ajax_test_dialog_form().
 */
function ajax_test_dialog_form_callback_modal($form, &$form_state) {
  $content = "Something to show in the modal";
  $title = "Hi, I'm a Modal";
  $response = new AjaxResponse();
  $response->addCommand(new OpenModalDialogCommand($title, $content, array('width' => '700')));
  return $response;
}

4. Difference between using #ajax and #attribute.

$build['link'] = array(
    '#type' => 'link',
    '#title' => 'Link 1 (modal)',
    '#href' => 'ajax-test/dialog-contents',
    '#attributes' => array(
      'class' => array('use-ajax'),
      'data-accepts' => 'application/vnd.drupal-modal',
    ),
  );

is same as

$build['link'] = array(
    '#type' => 'link',
    '#title' => 'Link 1 (modal)',
    '#href' => 'ajax-test/dialog-contents',
    '#ajax' => array(
      'accepts' => 'application/vnd.drupal-modal',
    ),
  );

Ensure the path ajax-test/dialog-contents is handled by a route.

Crell’s picture

jibran: Go ahead and do so, please.

jibran’s picture

larowlan’s picture

Status: Needs review » Fixed

Looks good, thanks @jibran

larowlan’s picture

Title: Change notice: Add an ajax command which makes it easy to use the dialog API in complex cases » Add an ajax command which makes it easy to use the dialog API in complex cases

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

xjm’s picture

Issue summary: View changes

Updated issue summary.