[META] Convert all confirm forms to use modal dialog

I want to help in this issue. We're in a Drupal8 sprint in Barcelona and if I can, I want to help on this issue.

I'll look at #1667742: Add abstracted dialog to core (resolves accessibility bug) and post a patch in a while

I'm also interested in this, but could not work out how to see an example of modal dialog being used in the admin UI.
I read through http://drupal.org/node/1667742 and can see that has been committed.
Are there examples of modal dialogs in the admin UI that I should be able to see now?

I am going to work on this list http://api.drupal.org/api/drupal/core%21modules%21system%21system.module....

Are there examples of modal dialogs in the admin UI that I should be able to see now?

No. We ended up removing it from the scope of that issue. But here is what we had there before removing it. This adds it to node deletion (both the operations link on admin/content and the button on node/NID/edit).

Also note, since this is done via Ajax, and Ajax relies on content negotiation, you may find that some of the places in #3 don't work. For example, in the other issue we initially had one of the forum.module forms converted, but it wasn't working correctly. Once we know what the problem ones are, we can fix them to be compatible with Drupal 8 content negotiation. Just mentioning this as a warning so it doesn't take you by surprise.

I have worked on the modules form a-n, remaining are o-v.
I have also converted forum.module but removed it from the patch and created a separate patch for it. I want to know if this is right way or not.
While creating the patch I noticed $links are still present in menu.module. so I think it needs follow up.
Image module is horrible to see forms are built in theme function. I found #1812684: [meta] Consolidate all table templates and add theme_hook_suggestions which is going to fix it.
Do I have to convert admin/reports/status/rebuild to modal dialog?
I'll update remaining modules in the next patch.

All the confirm forms are converted.

I have also converted cancel user form but it is not working correctly.
I haven't converted admin/reports/status/rebuild and admin/structure/views/view/%views_ui_cache/break-lock because both url's are created by url() function.
I have also not converted views_ui_confirm_delete because it start showing to progress indicators.

Last patch has failed some tests I might need some help fixing those.

Fixed some tests

Related discussions
#402760: Regression: Turn "delete tabs" back into buttons
#1834002: Configuration delete operations are all over the place

Remaining issues
1. Converted admin/reports/status/rebuild and admin/structure/views/view/%views_ui_cache/break-lock because both url's are created by url() function. I think #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases will fix the issue
2. Convert forum.module and figure out the issue. see #5 for detail.

Followup issues
1. menu.module required cleanup. $links are still present in menu.module
2. Fix views bug in #8 so adding VDC tag.

Fixed the follow up in the views code.

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -44,6 +44,9 @@ function aggregator_view() {
     $links['remove'] = array(
       'title' => t('remove items'),

out of scope: We could open a follow to rename this to delete to be more consistent: #1878302: Use 'delete' over 'remove' in the aggregator module

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestFormController.phpundefined
@@ -75,6 +75,15 @@ public function save(array $form, array &$form_state) {
+    $actions = parent::actions($form, $form_state);
+    unset($actions['delete']['#ajax']);

We maybe should explain why this was done in a short comment.

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -595,6 +595,9 @@ function translation_entity_form_field_ui_field_settings_form_alter(array &$form
+        '#ajax' => array(
+          'dialog' => array('modal' => TRUE),

We should maybe discuss whether we should inline this second array.

Jibran suggested to convert the break-lock to use the modal. In general i'm not 100% sure whether we can use an ! placeholder when we really need fancy handling of theme_link/l().

I think the current array format is fine. Either way works. This way it is nicely scannable.

In this patch

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestFormController.phpundefined
@@ -75,6 +75,15 @@ public function save(array $form, array &$form_state) {
+    $actions = parent::actions($form, $form_state);
+    unset($actions['delete']['#ajax']);

Added comment.

Converted admin/reports/status/rebuild to modal dialog.

Remaining task
Figure out forum.module issue and find out the solution. See #5 for detail. Patch attached in #6.

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestFormController.phpundefined
@@ -79,6 +79,7 @@ public function save(array $form, array &$form_state) {
+    // Don't need model dailog confirmation form.

So ... the actual question I have here is why? The tests will probably don't test that right? Btw. multiple spelling mistakes here :)

#16: confirm-forms-to-use-modal-dialog_1842036-16.patch queued for re-testing.

Comment fix.

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestFormController.phpundefined
@@ -79,7 +79,8 @@ public function save(array $form, array &$form_state) {
+    // Can't test the delete confirmation form in model dialog so don't need

what about "in a modal dialog, so remove #ajax".

Do we have a confirmation of the UX team that all these different places should use modal dialogs?

Updated the comment

So cool to see all these modals, though someone from the UX team should give a go.

tagging

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestFormController.php
@@ -75,6 +75,17 @@ public function save(array $form, array &$form_state) {
   /**
+   * Overrides Drupal\Core\Entity\EntityFormController::actions().
+   */
+  protected function actions(array $form, array &$form_state) {
+    $actions = parent::actions($form, $form_state);
+    // Can't test the delete confirmation form in a modal dialog, so remove
+    // #ajax.
+    unset($actions['delete']['#ajax']);
+    return $actions;
+  }

I don't understand this —

1) The mere existence of #ajax doesn't make the delete link unusable without Ajax/JS.

2) Tests are actually able to test Ajax operations via drupalPostAjax().

So why is #ajax removed here?

Aside form that, the patch looks good to me. I'd recommend to go ahead and do actual/real user testing among core developers in HEAD; i.e., just commit it, and see how it goes. We can always revise later.

Thanks @sun for correcting my mistake of course I didn't know about drupalPostAjax. I was just trying to fix the test. New patch with drupalPostAjax. And removed.

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestFormController.php
@@ -75,6 +75,17 @@ public function save(array $form, array &$form_state) {
   /**
+   * Overrides Drupal\Core\Entity\EntityFormController::actions().
+   */
+  protected function actions(array $form, array &$form_state) {
+    $actions = parent::actions($form, $form_state);
+    // Can't test the delete confirmation form in a modal dialog, so remove
+    // #ajax.
+    unset($actions['delete']['#ajax']);
+    return $actions;
+  }

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewEditFormController.php
@@ -84,10 +84,19 @@ public function form(array $form, array &$form_state, EntityInterface $view) {
+      $break_link = array(
+        '#type' => 'link',
+        '#href' => 'admin/structure/views/view/' . $view->get('name') . '/break-lock',
+        '#title' => t('break this lock'),
+        '#ajax' => array(
+          'dialog' => array('modal' => TRUE),
+        ),
+      );
+      $break_link = drupal_render($break_link);
       $form['locked'] = array(
         '#type' => 'container',
         '#attributes' => array('class' => array('view-locked', 'messages', 'warning')),
-        '#children' => t('This view is being edited by user !user, and is therefore locked from editing by others. This lock is !age old. Click here to <a href="!break">break this lock</a>.', array('!user' => theme('username', array('account' => user_load($view->locked->owner))), '!age' => format_interval(REQUEST_TIME - $view->locked->updated), '!break' => url('admin/structure/views/view/' . $view->get('name') . '/break-lock'))),
+        '#children' => t('This view is being edited by user !user, and is therefore locked from editing by others. This lock is !age old. Click here to !break_link.', array('!user' => theme('username', array('account' => user_load($view->locked->owner))), '!age' => format_interval(REQUEST_TIME - $view->locked->updated), '!break_link' => $break_link)),
         '#weight' => -10,
       );

This change is not compatible with translatability. As it is done before, we should spell out the < a > tag completely. If that is considered to be too verbose, let's just leave that out of this patch and discuss it in a follow-up.

Currently modal dialog doesn't have complete test coverage so while using drupalPostAjax mentioned in #25 I ran into 1 exception Undefined index: callbackin #26 so created this #1884530: Complete modal dialog tests. and postponing it till #1884530: Complete modal dialog tests. is fixed.

@tstoeckler

--- a/core/modules/node/node.module
+++ b/core/modules/node/node.moduleundefined
@@ -95,7 +95,16 @@ function node_help($path, $arg) {
       $message = t('The content access permissions need to be rebuilt.');
     }
     else {
-      $message = t('The content access permissions need to be rebuilt. <a href="@node_access_rebuild">Rebuild permissions</a>.', array('@node_access_rebuild' => url('admin/reports/status/rebuild')));
+      $link = array(
+        '#type' => 'link',
+        '#href' => 'admin/reports/status/rebuild',
+        '#title' => t('Rebuild permissions'),
+        '#ajax' => array(
+          'dialog' => array('modal' => TRUE),
+        ),
+      );
+      $link = drupal_render($link);
+      $message = t('The content access permissions need to be rebuilt. !link.', array('!link' => $link));
     }
     drupal_set_message($message, 'error');
   }
@@ -3736,11 +3745,19 @@ function node_requirements($phase) {

@@ -3736,11 +3745,19 @@ function node_requirements($phase) {
       $value = t('Disabled');
     }
     $description = t('If the site is experiencing problems with permissions to content, you may have to rebuild the permissions cache. Rebuilding will remove all privileges to content and replace them with permissions based on the current modules and settings. Rebuilding may take some time if there is a lot of content or complex permission settings. After rebuilding has completed, content will automatically use the new permissions.');
-
+    $link = array(
+      '#type' => 'link',
+      '#href' => 'admin/reports/status/rebuild',
+      '#title' => t('Rebuild permissions'),
+      '#ajax' => array(
+        'dialog' => array('modal' => TRUE),
+      ),
+    );
+    $link = drupal_render($link);
     $requirements['node_access'] = array(
       'title' => t('Node Access Permissions'),
       'value' => $value,
-      'description' => $description . ' ' . l(t('Rebuild permissions'), 'admin/reports/status/rebuild'),
+      'description' => $description . ' ' . $link,

I have also done it for rebuild permission link so should this also be reverted?

I must have missed that. Yes, that should be reverted. The entire < a > tag should be part of the t() call.

Updated issue summary.

Formatting

Tagging.

Duplicates #617730: UX: Use modal dialogs for confirmation forms. #114546: Confirmation alerts done with JS/AJAX (delete confirmation page and any others)

Postponed on #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases

Postponed on #1944472: Add generic content handler for returning dialogs as per #1870764-98: Add an ajax command which makes it easy to use the dialog API in complex cases

Updated issue summary.

Over in #1834002: Configuration delete operations are all over the place there's debate over removing the delete tab and reverting back to #402760: Regression: Turn "delete tabs" back into buttons. It's often mentioned as a drawback of removing the delete tab that the delete action would be destructive. People seem to be concerned by the possibility of accidental clicks on the delete button.

Those of us familiar with this issue here reassure people that there'll be a confirmation dialog on delete and actually point them here. We tell them that instead of it being a separate page/tab it'll now be a modal. How do we fall back though when there's no java enabled in browsers?

How do we fall back though when there's no java enabled in browsers?

Once #1944472: Add generic content handler for returning dialogs lands and this issue's patch is rerolled for it, it should be the case that without JS, the same confirmation form that is otherwise displayed in a modal, just gets displayed as a full html page. Nothing special needs to be done for that: that's the magic of that issue.

There's a nice table of all confirm forms #1945406: [meta] Convert all of confirm_form() to ConfirmFormBase most of them are entity-related. So the only question here - what would be displayed in breadcrumbs for fallback (none-js) pages? Once #1834002: Configuration delete operations are all over the place removes all Delete tabs so the only possible way to use "cancel-url" from confirm form

#1960928: Provide an opt-out modal confirmation when saving changes to content that is not configured to hold revisions.

People have been trying to help with #1946404: Convert forms in field_ui.admin.inc to the new form interface, but stumbled on the corresponding menu items currently being defined in a foreach, and thus the need to go through RouteSubscriber / RouteEnhancer (or something :-)).

Any knowledgeable people to provide docs or code pointers on the correct way to leverage that over there ?

#1944472: Add generic content handler for returning dialogs is in.

The first step here is to reroll #26 so that it applies to HEAD. Additionally:

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -140,6 +140,9 @@ protected function actions(array $form, array &$form_state) {
+        '#ajax' => array(
+          'dialog' => array('modal' => TRUE),
+        ),

These should all be replaced with:

+        '#ajax' => array(
+          'accepts' => 'application/vnd.drupal-modal',
+        ),

There'll be more to do to make it all work, including the follow up mentioned at the end of #1944472-70: Add generic content handler for returning dialogs, but getting an up to date patch here will help a lot in having some concrete stuff to work with in doing that. It might even make sense to do the _form controller work as part of this issue, since this issue provides such a great set of simple use cases for it.

With all the confirm forms beign converted to configformBase form handler I don't think this is the right moment to reroll this... Nothing from the original patch applies as most forms are already converted to the new forminterface...

What do you expect in this reroll?

This patch is doing nothing with confirm forms. It is just showing confirm forms in model dialog. Only thing relevant to confirm forms is

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

which is moved to ConfirmFormBase and I don't think we need this anyway.

I am going to re-roll the patch so assigning to me myself. @effulgentsia thanks for explaining the changes.

Here is a re-roll.

Trying this out on simplytest.me, I'm getting:

POST http://s0d9f94d237feb49.s3.simplytest.me/admin/structure/contact/manage/... 404 (Not Found)
POST http://s0d9f94d237feb49.s3.simplytest.me/admin/structure/custom-blocks/m... 415 (Unsupported Media Type)
POST http://s0d9f94d237feb49.s3.simplytest.me/admin/structure/taxonomy/manage... 415 (Unsupported Media Type)
POST http://s0d9f94d237feb49.s3.simplytest.me/admin/structure/views/view/fron... 404 (Not Found)

So a mix of 404 and 415, no dialogs whatsoever showing up.

+++ b/core/lib/Drupal/Core/Ajax/DialogController.phpundefined
@@ -54,12 +54,14 @@ protected function forward(Request $request, $content) {
+  public function modal(Request $request, $_content = FALSE) {

@@ -69,19 +71,25 @@ public function modal(Request $request, $_content) {
+  public function dialog(Request $request, $_content = FALSE, $modal = FALSE) {

This FALSE looks odd, could probably just be NULL

+++ b/core/lib/Drupal/Core/HtmlFormController.phpundefined
@@ -63,6 +63,9 @@ public function content(Request $request, $_form) {
+    if ($request->attributes->get('dialog')) {

Could/should this be ->has() instead? Not sure if it matters

+++ b/core/lib/Drupal/Core/Ajax/DialogController.php
@@ -69,19 +71,25 @@ public function modal(Request $request, $_content) {
+    if (!$_content) {
+      // No $_content means we have a form instead.
+      $_content = '\Drupal\Core\HtmlFormController::content';
+    }

That seems like a dangerous assumption. Instead, since we have $request here we can just check it directly for _content, _form, etc. and remove $_content from the method signature entirely.

+++ b/core/lib/Drupal/Core/Routing/Enhancer/DialogEnhancer.php
@@ -51,8 +51,10 @@ public function __construct(ContentNegotiation $negotiation) {
-    if (empty($defaults['_controller']) && !empty($defaults['_content']) && $this->negotiation->getContentType($request) == $this->targetContentType) {
-      $defaults['_controller'] = $this->controller;
+    if ($this->negotiation->getContentType($request) == $this->targetContentType) {
+      if (empty($defaults['_controller']) && (!empty($defaults['_content']) || !empty($defaults['_form']))) {
+        $defaults['_controller'] = $this->controller;
+      }

This minorly conflicts with #1988666: Cleanup Controllers and Enhancers, but that should be resolvable.

And finally, here's a screenshot of it in action on the aggregator feed delete form - demonstrating that we need to add some widths to some of these dialogs.

Is the reasoning that the form needs to specify some width so that the title doesn't get truncated ?
Problem is the texts are going to be translated, and the resulting string length will vary greatly depending on the language, so thre's no "reasonable assumption" to be made in code about the width. I'd say we just can't have an output for our popups that truncates strings if they don't fit ?
(not to say that being able to.specify widths wouldn't be otherwise nice)

FYI: #1993518: Display batch progress in modals ?

Thanks @larowlan for working on it. As per #50 this is postponed on #1945406: [meta] Convert all of confirm_form() to ConfirmFormBase.

I would not postpone this on conversions anymore, anything we want to get in - we need to get in now. Conversions or not. And we can easily pass on certain interfaces, that are not yet converted - the majority are.

I agree with #59. There's enough converted forms already in HEAD to make this worth getting in and tested by more people. After this is in, we can update #1945406: [meta] Convert all of confirm_form() to ConfirmFormBase with instructions to also modalize the remaining ones as part of their conversions to ConfirmFormBase.

On the "setting width" aspect that's been mentioned several times now: can't it just be smart and auto-adjust the width to fit the title? Alternatively, can't the title be wrapped over multiple lines?

working on #60

I have replaced

-      'attributes' => array(
-        'class' => array('use-ajax'),
-        'data-accepts' => 'application/vnd.drupal-modal',
+      'ajax' => array(
+        'accepts' => 'application/vnd.drupal-modal',

So all links are now showing in modal dialog weather converted to routes or not. The only downside to this approach is that we can't use data-dialog-options.
IMO for FAPI #ajax we should add 'dialogOptions' key somthing like this

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

Known Bug

• admin/reports/status/rebuild is not showing in modal related issue #1946434: Convert all of confirm_form() in node.admin.inc and node.pages.inc to the new form interface.

• ------------- core/lib/Drupal/Core/Entity/EntityFormController.php -------------
  index 98d4a3d..2a7ed6c 100644
  @@ -201,6 +201,9 @@ protected function actions(array $form, array &$form_state) {
         ),
         'delete' => array(
           '#value' => t('Delete'),
  +        '#ajax' => array(
  +          'accepts' => 'application/vnd.drupal-modal',
  +        ),
           // No need to validate the form when deleting the entity.
           '#submit' => array(
             array($this, 'delete'),

  This is not working for delete buttons. For buttons see diff of search.admin.inc

in the patch.

• On admin/structure/types dropbutton delete link is not working.

Unassigning and setting to CNR to get some assistance because I am unable to figure out test failures.

Some more converted confirm forms: #1946454: Convert all confirm_form() in system.module and system.admin.inc to the new form interface and convert route

#66: modal-confirm-1842036.66.patch queued for re-testing.

The failed tests in #66 came back green locally, so I spooled them up for a retest.

Sorry, I'd rerolled the patch in #66 locally because it wasn't applying any more. I then retested it locally and the tests returned green, so here's the reroll that should pass.

+++ b/core/lib/Drupal/Core/Controller/HtmlFormController.phpundefined
@@ -63,6 +63,9 @@ public function content(Request $request, $_form) {
+    if ($request->attributes->get('dialog')) {
+      return drupal_render($form);

+++ b/core/lib/Drupal/Core/Form/ConfirmFormBase.phpundefined
index c355cc7..9fe5223 100644
--- a/core/lib/Drupal/Core/Routing/Enhancer/ContentControllerEnhancer.php

+++ b/core/lib/Drupal/Core/Routing/Enhancer/ContentControllerEnhancer.phpundefined
@@ -48,8 +48,13 @@ public function __construct(ContentNegotiation $negotiation) {
+    if (empty($defaults['_controller']) &&
+      ($type = $this->negotiation->getContentType($request)) &&
+      (!empty($defaults['_content']) ||
+      (!empty($defaults['_form']) && $type != 'html'))) {

This hardcodes stuff, so I'm not sure whether this is really the right approach.

Reuploading #74 to test.

http://qa.drupal.org/pifr/test/527323 and http://qa.drupal.org/pifr/test/525543 both has FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. same error.

Uploading a re-roll.

#78: modal-confirm-1842036.77.patch queued for re-testing.
Edit: I am able to clone drupal normally.

#78: modal-confirm-1842036.77.patch queued for re-testing.

#78: modal-confirm-1842036.77.patch queued for re-testing.

On #1983164: Entity Forms in ajax requests don't find the route I started an approach for entity forms which are called by a subrequest of AjaxController which maybe would be also capable to solve this problem here.

#1988716: Convert delete confirm to AJAX dialog for config_translation is postponed on this.

I *think* the remaining tasks in the issue summary are still up-to-date.

We are almost done #1945406: [meta] Convert all of confirm_form() to ConfirmFormBase other then #1938318: Convert book_remove_form to a new-style Form object which is block on #1947880: Replace node_access() by $entity->access(). I'll reroll the patch soon.

Here is the reroll.

Fixed a failing test.

I looked into the test failures but they are occurring on a spot where there are no confirmation forms used. When trying to replicate them manually I was constantly hitting #1831846: Help block is broken with language path prefixes, and in that issue these tests are being reworked.

This might be related to the test fails, before things start going haywire an error message appears when a translation is marked as outdated: #2087299: Impossible to save nodes if translation is newer than default language. This also happens without this patch applied, but it seems like the entity is in an unexpected state for most of the remaining test.

#94: drupal8.forms-system.1842036-94.patch queued for re-testing.

No idea what to evaluate.

Updated issue summary.

There's issue to discus about comment edit/delete forms #1902874: Comment delete and edit to open in overlay

Marked #1988716: Convert delete confirm to AJAX dialog a duplicate of this issue, since the config translation module (that that issue belongs to) is now in core, so this patch would need to cover that.

This is a re-roll of previous patch (#94) to current (20/12/2013) drupal 8 version

Looking at the failure reported by testbot ... I cannot reproduce .. restesing

php core/scripts/run-tests.sh --verbose --file ./core/modules/image/lib/Drupal/image/Tests/ImageEffectsTest.php

Drupal test run
---------------

Tests to be run:
- Image effects (Drupal\image\Tests\ImageEffectsTest)

Test run started:
Sunday, December 22, 2013 - 17:44

Test summary
------------

Image effects 41 passes, 0 fails, and 0 exceptions

Test run duration: 7 min 5 sec

105: drupal8.forms-system.1842036-105.patch queued for re-testing.

Straight reroll.

Going to work on this.

Cannot replicate the test failure locally. Updated the patch with the delete confirmation dialog from #1988716: Convert delete confirm to AJAX dialog as per comment #103.

+++ b/core/modules/node/node.pages.inc
@@ -164,12 +164,18 @@ function node_revision_overview($node) {
+          'ajax' => array(
+            'accepts' => 'application/vnd.drupal-modal',
+          ),

If the goal is that confirmation forms should all, by default, be modals, we shouldn't force people to remember to set this seemingly obscure marker every time. Is there some way we can add an affordance so that it happens by default if someone does something more "obvious"? Eg, a way to recognize a confirmation form link, or a way to simply flag "modal" rather than setting the accept mime type directly, or something along those lines?

(I at the moment have no opinion about whether confirmation forms SHOULD all be modals; I leave that decision to the UX team.)

I really like the idea proposed by crell, as we should not expose those really deep level details in our public apis like that.
As long we don't require people to use modal => TRUE (or something similar) this is not even an API change but an addition.

+++ b/core/modules/node/node.module
@@ -88,7 +88,9 @@ function node_help($path, $arg) {
+      drupal_add_library('system', 'drupal.ajax');

drupal_add_library() should no longer be used, see #2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses. Instead we should use something like

$build = array(
  '#attached' =>
    'library' => array(
      array('system', 'drupal.ajax'),
    ),
  ),
);
drupal_render($build);

(Probably with less whitespace than that :-))

@dawehner, #modal => TRUE is a very nice idea, would love that :D

Just a rerole. We indeed hat #ajax][dialog at some point already.

Just a rerole. We indeed hat #ajax][dialog at some point already.

Some more fixes.

Hi! I've investigated this issue and found that problem is somehow related to the form caching after adding ajax to it. For some reason we lose plugin after cache receiving. I tried to figure out why but with no luck, so I've added a small workaround. Maybe somebody can offer better solution:

     if ($this->plugin instanceof PluginFormInterface) {
-      $this->plugin->submitConfigurationForm($form, $form_state);
+      $this->entity->getPlugin()->submitConfigurationForm($form, $form_state);
     }

@larowlan and I agreed that instead of re-rolling the last patch, it's better if we split this issue up in multiple smaller ones that target specific contexts. See #2253257: Use a modal for entity delete operation links for the first sub-issue.

So maybe better to re-title as meta with child sub-issues?

I am removing the Novice tag from this issue because:

Avoid issues that are:
...
long, because they can be overwhelming

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

looks that should go to 8.1

Concur.

Would it be a good idea to add a convenience method to FormBase to detect whether a form is displayed in a modal?

  /**
   * Returns whether the form is displayed in a modal.
   *
   * @return bool
   *   TRUE if the form is displayed in a modal.
   */
  protected function isModal() {
    return $this->getRequest()->query->get(MainContentViewSubscriber::WRAPPER_FORMAT) === 'drupal_modal';
  }

I am using this to hide the "Cancel" link in my confirm forms when they are displayed in a modal. The close button takes the role of cancelling the form.

Since this is a meta issue, should I create a separate child issue for this?

Created #2661046: Allow forms to easily determine whether they are shown in a modal.

I created a simple utility module at Modal Configuration to make this configuration admin manageable. The main reason for this is, if the site admin/user don't want to have modal, they still be able to opt-out and same way they can add more modal, they can. Its still in very early stage, but can be improved, if we plan to go this route.

@vijaycs85 From my POV, a contrib module could perfectly provide this configurability - I dont see it necessary to overload core with options like that.

I think it's a really important UX improvement to have confirmation messages in modals. The new page load every time is very 90's ;) and costs a lot of time. So that part should not be a separate module but core default I think. Just my 2 cent ;) because we just ran into the same discussion.

I think it's a really important UX improvement to have confirmation messages in modals. The new page load every time is very 90's ;)

@Anybody, agreed. but having as a separate module (I don't mind having it incore experimental module, if possible :)) gives option to configuration or disable if some sites decided to.

Usability is preferred over UX, D7UX, D8UX etc. See https://www.drupal.org/issue-tags/topic

Do we have an issue for doing this to content entities delete operation links?

are there any plans to introduce this as a core feature? page loads for confirmations makes drupal feel dated and cumbersome.

is there an easy way to do this if there are no plans to introduce as a core feature?

Updated the title as well.

I agree that this would be better UX.

This issue is related but different:
https://www.drupal.org/project/drupal/issues/2883755

I also found this module which I'm surprised hasn't been posted here yet:
https://www.drupal.org/project/admin_dialogs

I haven't tried it yet, but as the time of posting it is maintained.