Allow Close(Modal)DialogCommand to remove the dialog from the DOM, document how client-side dialogs can do that too [#2107199]

Updated: Comment #0

Problem/Motivation

At #1872206: Improve CKEditor toolbar configuration accessibility, I ran into a bug that was caused by the fact that any Drupal.dialog is retained in the DOM after it's been closed, even though this appears to be counter to what everybody expects.

The only other dogfooding instance in Drupal core right now is in edit.module: when you start in-place editing an entity, make a change and then decide to stop in-place editing, you'll get a modal dialog asking you whether to discard changes or not. No matter what you answer, the dialog will remain in the DOM. As will all other future such dialogs. Try and see for yourself.

Currently there is not a single user of the open/close (modal) dialog AJAX commands in core. #1851414: Convert Views to use the abstracted dialog modal will introduce the first. There, too, the old modal dialog just lingers in the DOM, only to be overwritten again, even when opening the *same* dialog.

Proposed resolution

The thing is:

Drupal.dialog is closely modeled after the W3C Dialog API, and there a dialog won't be removed from the DOM upon closing. So we should not change the default
Server-generated dialogs will almost always want to remove the dialog from the DOM when closing it, since opening that dialog inherently sends a new version anyway. So it should default to $remove = TRUE.

Client-side generated dialogs will almost never use Drupal behaviors, yet the default which looks like this

drupalSettings.dialog = {
  …
  close: function (event) {
    Drupal.detachBehaviors(event.target, null, 'unload');
  }
};

obviously detaches behaviors even though there won't be any in >90% of the uses. In this case, the elegant solution is to just override that default close method:

Drupal.dialog(Drupal.theme('somethingThatIsRenderedOnTheClientSide'), {
  title: Drupal.t('My client-side dialog'),
  close: function (event) {
    $(event.target).remove();
  }
});

This way, we don't have to enforce a default of "remove on closing" onto Drupal.dialog itself, so it can continue to mimic the W3C dialog API as close as possible. Instead, we just make the calling code on the server side remove it automatically on closing, which is probably the most common use of the dialog API. Then, for fully client-side dialogs, it's a matter of adding the above one-line function.

Remaining tasks

Make tests pass.
Build consensus.

User interface changes

None.

API changes

A new optional $remove parameter for CloseDialogCommand and CloseModalDialogCommand, that defaults to TRUE.

#1872206: Improve CKEditor toolbar configuration accessibility
#1851414: Convert Views to use the abstracted dialog modal

Comment	File	Size	Author
#17	dialog_remove_dom-2107199-17.patch	5.91 KB	Wim Leers
#17
#15	dialog_remove_dom-2107199-15.patch	5.91 KB	Wim Leers
#15
#15	interdiff.txt	4.97 KB	Wim Leers
#10	dialog_remove_dom-2107199-10.patch	5.89 KB	Wim Leers
#10
#6	dialog_remove_dom-2107199-6.patch	5.9 KB	Wim Leers
#6
#6	interdiff.txt	2.12 KB	Wim Leers
#1	dialog_remove_dom-2107199-1.patch	3.43 KB	Wim Leers
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 8 October 2013 at 09:31

Status:

Active

» Needs review

File	Size
dialog_remove_dom-2107199-1.patch	3.43 KB

I only tested the client-side part, but it should work. (Juggling too many patches simultaneously right now.)

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 8 October 2013 at 09:38

I was going to provide a simplytest.me link that combines #1851414: Convert Views to use the abstracted dialog modal and this patch (http://simplytest.me/project/drupal/8.x?patch[]=https://drupal.org/files/dialog_remove_dom-2107199-1.patch&patch[]=https://drupal.org/files/core-views-js-modal-1851414-91.patch), but that sadly doesn't work, the patches probably conflict.

Comment #3

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 8 October 2013 at 09:58

When looking at the patch I was very puzzled as to why would you have a $remove defaulted to TRUE and never set it to FALSE. Also reading most of the issue summary makes it clear this is indeed a good idea by default. So looks like the only reason not to do it out of the box is the W3C dialog API. Doh. It would be good to document this fact somehow in the code, because later on people will not have this information, and just reading the code it is puzzling why there is even an option to keep the dialog, since you are closing it anyway. (Sounds like it is bad for accessibility too).

Comment #4

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 8 October 2013 at 10:02

It would be good to document this fact somehow in the code, because later on people will not have this information, and just reading the code it is puzzling why there is even an option to keep the dialog, since you are closing it anyway. (Sounds like it is bad for accessibility too).

Because the W3C Dialog API defines it that way, and because it makes sense from their POV: the Dialog API consists of nothing more than saying "hey, see that DOM element? Please show that as a dialog. kthxbye". By that reasoning, it makes some sense to also keep that element in the DOM when closing the dialog, which is precisely what that API does.

It's good to read that I'm not the only one who finds that extremely counter-intuitive :)

Note that Drupal.dialog being based on the W3C dialog API is explicitly documented in dialog.js:

/**
 * @file
 *
 * Dialog API inspired by HTML5 dialog element:
 * http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.html#the-dialog-element
 */

But I agree that additional comments to explain where there is a mismatch in typical explanation are valuable.

Comment #5

8 October 2013 at 10:20

Status:

Needs review

» Needs work

The last submitted patch, dialog_remove_dom-2107199-1.patch, failed testing.

Comment #6

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 8 October 2013 at 16:18

Status:

Needs work

» Needs review

File	Size
interdiff.txt	2.12 KB
dialog_remove_dom-2107199-6.patch	5.9 KB

The Drupal\system\Tests\Form\TriggeringElementTest failure was a random fail. The other two were legitimate fails. Fixed.

Comment #7

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 9 October 2013 at 09:10

Status:

Needs review

» Reviewed & tested by the community

Looks good to me!

Comment #8

nod_

French

Lille

CreditAttribution: nod_ commented 9 October 2013 at 10:12

+1 for me too, thanks for keeping it out of dialog.js :)

Comment #9

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 9 October 2013 at 10:20

+++ b/core/misc/dialog.js
@@ -14,6 +14,12 @@ drupalSettings.dialog = {
+  // When using this API directly (when generating dialogson the client side),

dialogs on

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 9 October 2013 at 10:26

File	Size
dialog_remove_dom-2107199-10.patch	5.89 KB

Identical to #6, reroll only to fix the typo reported in #9.

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 9 October 2013 at 15:23

Issue tags:

+DrupalWTF

Comment #12

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 9 October 2013 at 16:47

Status:

Reviewed & tested by the community

» Needs review

Ok, had a long talk about this with Theodore and Wim, including a trip to #whatwg on Freenode, because my impression was the same as Gábor's impression: that *of course* you would remove the element from the DOM when it is no longer in your face, and since there was no language in http://www.whatwg.org/specs/web-apps/current-work/multipage/commands.htm... that said *not* to do that (and since, in fact, the language around removing it from the Document and removing it from the stack is exactly the same language), that we could fill in the blanks here with sensible, logical behaviour.

Unfortunately, though, that's not how it works. :(

<jgraham>: webchick: Since it doesn't way to remove it from the DOM, you don't remove it from the DOM
<jgraham>: Whatever is not explicitly allowed is forbidden
<jgraham>: Basically the dialog exists in the DOM always, but the display is controlled by CSS

Margh. :(

Ok, so knowing that this is the spec, and we want Dialog API to conform to the spec as closely as possible...

+++ b/core/lib/Drupal/Core/Ajax/CloseDialogCommand.php
@@ -20,13 +20,23 @@ class CloseDialogCommand implements CommandInterface {
+   * @param bool $remove
+   *   (optional) Whether to remove the dialog from the DOM or not.
...
-  public function __construct($selector = NULL) {
+  public function __construct($selector = NULL, $remove = TRUE) {

It looks *really* odd for a constructor to include an argument about removing itself. The whole point of a constructor is to set things up, not tear them down. :)

Therefore, could we rename this variable to "$persist" or something, and sort of flip the language?

Comment #13

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 9 October 2013 at 16:48

Also, the topic of the #whatwg IRC channel contains "Please leave your sense of logic at the door, thanks!" .... I'll just go ahead and let that pass by without comment. ;)