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

Files: 
CommentFileSizeAuthor
#17 dialog_remove_dom-2107199-17.patch5.91 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,515 pass(es).
[ View ]
#15 dialog_remove_dom-2107199-15.patch5.91 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 58,868 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#15 interdiff.txt4.97 KBWim Leers
#10 dialog_remove_dom-2107199-10.patch5.89 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,856 pass(es).
[ View ]
#6 dialog_remove_dom-2107199-6.patch5.9 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,787 pass(es).
[ View ]
#6 interdiff.txt2.12 KBWim Leers
#1 dialog_remove_dom-2107199-1.patch3.43 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 58,356 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new3.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,356 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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

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).

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.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.12 KB
new5.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,787 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Looks good to me!

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

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

dialogs on

StatusFileSize
new5.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,856 pass(es).
[ View ]

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

Issue tags:+DrupalWTF

.

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?

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. ;)

Status:Needs review» Needs work

Wim agreed with this, so marking needs work.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new4.97 KB
new5.91 KB
FAILED: [[SimpleTest]]: [MySQL] 58,868 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

$remove$persist, language & logic flipped.

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new5.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,515 pass(es).
[ View ]

CloseModalDialogCommand's constructor's default $persist value was wrong, hence the test failed. Fixed now.

Status:Reviewed & tested by the community» Fixed

Awesome, thanks. I think this reads a lot better.

Committed and pushed to 8.x. Thanks!

Issue tags:-sprint

Thanks! :)

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

Issue summary:View changes

Updated issue summary.