Problem/Motivation

Follow up for #1667742: Add abstracted dialog to core (resolves accessibility bug) - to port remaining dialogs to modal.

Proposed resolution

Update links/form submit buttons to display the confirmation form in a modal

Remaining tasks

User interface changes

Confirm forms will be displayed in modals

API changes

None

Files: 
CommentFileSizeAuthor
#126 interdiff-1842036-122-126.txt1.36 KBInternetDevels
#126 modal-confirm-1842036-126.patch18.33 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 63,110 pass(es). View

Comments

rvilar’s picture

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

finn.lewis’s picture

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?

jibran’s picture

Assigned: Unassigned » jibran
effulgentsia’s picture

FileSize
1.79 KB
PASSED: [[SimpleTest]]: [MySQL] 49,275 pass(es). View

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

effulgentsia’s picture

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.

jibran’s picture

Status: Active » Needs review
FileSize
4.67 KB
13.47 KB
FAILED: [[SimpleTest]]: [MySQL] 49,271 pass(es), 11 fail(s), and 26 exception(s). View

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.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Novice

The last submitted patch, confirm-forms-to-use-modal-dialog_1842036-6.patch, failed testing.

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Needs review
Issue tags: +JavaScript, +Novice
FileSize
13.06 KB
7.18 KB
20.65 KB
FAILED: [[SimpleTest]]: [MySQL] 49,258 pass(es), 10 fail(s), and 26 exception(s). View

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.
views-bug.png

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

Status: Needs review » Needs work

The last submitted patch, confirm-forms-to-use-modal-dialog_1842036-8.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
3.87 KB
24.52 KB
FAILED: [[SimpleTest]]: [MySQL] 49,274 pass(es), 5 fail(s), and 12 exception(s). View

Fixed some tests

Status: Needs review » Needs work

The last submitted patch, confirm-forms-to-use-modal-dialog_1842036-10.patch, failed testing.

andypost’s picture

jibran’s picture

Status: Needs work » Needs review
Issue tags: +VDC
FileSize
795 bytes
25.09 KB
PASSED: [[SimpleTest]]: [MySQL] 50,380 pass(es). View

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.

dawehner’s picture

Issue tags: -VDC
FileSize
27.56 KB
PASSED: [[SimpleTest]]: [MySQL] 50,203 pass(es). View
2.81 KB

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

xjm’s picture

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

jibran’s picture

FileSize
2.79 KB
29.65 KB
PASSED: [[SimpleTest]]: [MySQL] 50,228 pass(es). View

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.

dawehner’s picture

+++ 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 :)

Status: Needs review » Needs work
Issue tags: -JavaScript, -Novice, -d8ux, -D8UX usability

The last submitted patch, confirm-forms-to-use-modal-dialog_1842036-16.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Novice, +d8ux, +D8UX usability
jibran’s picture

FileSize
860 bytes
29.69 KB
PASSED: [[SimpleTest]]: [MySQL] 50,183 pass(es). View

Comment fix.

dawehner’s picture

+++ 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?

jibran’s picture

FileSize
881 bytes
29.86 KB
PASSED: [[SimpleTest]]: [MySQL] 50,665 pass(es). View

Updated the comment

dawehner’s picture

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

falcon03’s picture

tagging

sun’s picture

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

jibran’s picture

FileSize
1.88 KB
29.72 KB
FAILED: [[SimpleTest]]: [MySQL] 50,652 pass(es), 0 fail(s), and 1 exception(s). View

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;
+  }
tstoeckler’s picture

Status: Needs review » Needs work
+++ 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.

jibran’s picture

Status: Needs work » Postponed
Issue tags: -Usability

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.

jibran’s picture

Issue tags: +Usability

@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?

tstoeckler’s picture

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

jibran’s picture

Issue summary: View changes

Updated issue summary.

jibran’s picture

Issue summary: View changes

Formatting

jibran’s picture

Issue tags: +modal dialog

Tagging.

jibran’s picture

jibran’s picture

Issue summary: View changes

Updated issue summary.

klonos’s picture

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?

effulgentsia’s picture

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.

andypost’s picture

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

yched’s picture

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 ?

jibran’s picture

effulgentsia’s picture

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.

aspilicious’s picture

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?

jibran’s picture

Assigned: Unassigned » jibran
Status: Active » Needs work

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.

larowlan’s picture

We may need to make sure form enhancer runs before dialog enhancer...

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Needs review
FileSize
38.35 KB
FAILED: [[SimpleTest]]: [MySQL] 56,456 pass(es), 2 fail(s), and 0 exception(s). View

Here is a re-roll.

Status: Needs review » Needs work

The last submitted patch, confirm-forms-to-use-modal-dialog_1842036-45.patch, failed testing.

larowlan’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -77,9 +77,17 @@ public function getOperations(EntityInterface $entity) {
+    drupal_add_library('system', 'drupal.ajax');

+++ b/core/modules/action/lib/Drupal/action/Controller/ActionController.phpundefined
@@ -119,6 +123,9 @@ public function adminManage() {
+      drupal_add_library('system', 'drupal.ajax');

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -121,6 +129,9 @@ public function adminOverview() {
+    drupal_add_library('system', 'drupal.ajax');

+++ b/core/modules/ban/ban.admin.incundefined
@@ -45,6 +49,9 @@ function ban_admin_page($default_ip = '') {
+  drupal_add_library('system', 'drupal.ajax');

+++ b/core/modules/comment/comment.moduleundefined
@@ -921,7 +921,15 @@ function comment_links(Comment $comment, EntityInterface $node) {
+      drupal_add_library('system', 'drupal.ajax');

+++ b/core/modules/image/image.admin.incundefined
@@ -544,6 +555,9 @@ function theme_image_style_list($variables) {
+  drupal_add_library('system', 'drupal.ajax');

+++ b/core/modules/language/language.admin.incundefined
@@ -683,6 +691,9 @@ function theme_language_negotiation_configure_browser_form_table($variables) {
+  drupal_add_library('system', 'drupal.ajax');

+++ b/core/modules/node/content_types.incundefined
@@ -20,6 +20,9 @@ function node_overview_types() {
+  drupal_add_library('system', 'drupal.ajax');

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

+++ b/core/modules/node/node.pages.incundefined
@@ -244,6 +244,10 @@ function node_delete_confirm_submit($form, &$form_state) {
+  drupal_add_library('system', 'drupal.ajax');

+++ b/core/modules/openid/openid.pages.incundefined
@@ -30,6 +30,9 @@ function openid_user_identities($account) {
+  drupal_add_library('system', 'drupal.ajax');

+++ b/core/modules/path/path.admin.incundefined
@@ -19,6 +19,9 @@ function path_admin_overview($keys = NULL) {
+  drupal_add_library('system', 'drupal.ajax');

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutListController.phpundefined
@@ -42,10 +42,18 @@ public function getOperations(EntityInterface $entity) {
+    drupal_add_library('system', 'drupal.ajax');

+++ b/core/modules/system/system.admin.incundefined
@@ -2682,6 +2686,9 @@ function system_date_time_formats() {
+  drupal_add_library('system', 'drupal.ajax');

@@ -2850,6 +2861,9 @@ function system_date_format_language_overview_page() {
+    drupal_add_library('system', 'drupal.ajax');

can we use #attached instead?

Gábor Hojtsy’s picture

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.

larowlan’s picture

Assigned: Unassigned » larowlan

Have been working on this

larowlan’s picture

Status: Needs work » Needs review
FileSize
20.47 KB
42.6 KB
FAILED: [[SimpleTest]]: [MySQL] 55,830 pass(es), 2 fail(s), and 0 exception(s). View
4.8 KB

So this adds some extra changes to dialog enhancer to allow it to work with _form routes.
These dialogs definitely need some elements set like width.

Also this patch can't go-in until all of the delete form paths use routes instead of hook_menu() so we should probably postpone on #1971384: [META] Convert page callbacks to controllers or split it up into distinct patches as routes are converted.

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.
Screen Shot 2013-05-08 at 8.30.35 AM.png

tim.plunkett’s picture

+++ 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

larowlan’s picture

Assigned: larowlan » Unassigned

Back to jibran

Crell’s picture

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

larowlan’s picture

Agreed on the $_content and $_form, good catch

yched’s picture

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)

Status: Needs review » Needs work

The last submitted patch, modal-confirm-1842036.50.patch, failed testing.

yched’s picture

jibran’s picture

Status: Needs work » Postponed

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

Bojhan’s picture

Status: Postponed » Active

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.

effulgentsia’s picture

Title: Convert remaining confirm forms to use modal dialog » Convert all confirm forms already converted to new routing system to use modal dialog
Status: Active » Needs work

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.

larowlan’s picture

#1988666: Cleanup Controllers and Enhancers is in - this needs a re-roll

larowlan’s picture

Status: Needs work » Needs review
FileSize
42.21 KB
FAILED: [[SimpleTest]]: [MySQL] 55,499 pass(es), 2 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, modal-dialogs.post-enhancer-cleanup.patch, failed testing.

Wim Leers’s picture

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?

jibran’s picture

Assigned: Unassigned » jibran

working on #60

jibran’s picture

Status: Needs work » Needs review
FileSize
27.65 KB
34.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch modal-confirm-1842036.66.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, modal-confirm-1842036.66.patch, failed testing.

jibran’s picture

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

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

Pancho’s picture

larowlan’s picture

Status: Needs review » Needs work
Issue tags: -JavaScript, -Needs usability review, -Usability, -Novice, -modal dialog, -d8ux, -D8UX usability

The last submitted patch, modal-confirm-1842036.66.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Needs usability review, +Usability, +Novice, +modal dialog, +d8ux, +D8UX usability

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

jessebeach’s picture

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

jessebeach’s picture

FileSize
37.46 KB
None View

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.

dawehner’s picture

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

jibran’s picture

FileSize
37.46 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View

Reuploading #74 to test.

Status: Needs review » Needs work

The last submitted patch, modal-confirm-1842036.74.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
34.62 KB
FAILED: [[SimpleTest]]: [MySQL] 55,648 pass(es), 45 fail(s), and 13 exception(s). View

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.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Needs usability review, -Usability, -Novice, -modal dialog, -d8ux, -D8UX usability

The last submitted patch, modal-confirm-1842036.77.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

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

The last submitted patch, modal-confirm-1842036.77.patch, failed testing.

jibran’s picture

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

Status: Needs review » Needs work

The last submitted patch, modal-confirm-1842036.77.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Needs usability review, +Usability, +Novice, +modal dialog, +d8ux, +D8UX usability

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

dawehner’s picture

Status: Needs review » Needs work

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.

larowlan’s picture

@dawehner, please review #1998698: Allow Dialog Controller to work with form/entity form routes where the hard-coding is removed (different approach)

larowlan’s picture

Status: Needs work » Needs review

Straight re-roll.
Note that old paths still don't work (only new routes).

Also to control the ajax width - this works - tested with EntityListController and contact categories.

   $operations['delete'] = array(
      'title' => t('Delete'),
      'href' => $uri['path'] . '/delete',
      'ajax' => array(
        'accepts' => 'application/vnd.drupal-modal',
        // Nominate a width.
        'dialog' => array(
          'width' => 800,
        ),
      ),
      'options' => $uri['options'],
      'weight' => 100,
    );
larowlan’s picture

FileSize
26.59 KB
FAILED: [[SimpleTest]]: [MySQL] 57,949 pass(es), 47 fail(s), and 14 exception(s). View

Status: Needs review » Needs work

The last submitted patch, dialog-confirm-1842036.87.patch, failed testing.

YesCT’s picture

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

jibran’s picture

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
19.89 KB
19.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,586 pass(es), 8 fail(s), and 1 exception(s). View

Here is the reroll.

Status: Needs review » Needs work

The last submitted patch, drupal8.forms-system.1842036-92.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
648 bytes
20.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.forms-system.1842036-94.patch. Unable to apply patch. See the log in the details link for more information. View

Fixed a failing test.

Status: Needs review » Needs work

The last submitted patch, drupal8.forms-system.1842036-94.patch, failed testing.

pfrenssen’s picture

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.

pfrenssen’s picture

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.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: -JavaScript, -Needs usability review, -Usability, -Novice, -modal dialog, -d8ux, -D8UX usability

Status: Needs review » Needs work
Issue tags: +JavaScript, +Needs usability review, +Usability, +Novice, +modal dialog, +d8ux, +D8UX usability

The last submitted patch, drupal8.forms-system.1842036-94.patch, failed testing.

Bojhan’s picture

Issue tags: -Needs usability review

No idea what to evaluate.

Bojhan’s picture

Issue summary: View changes

Updated issue summary.

klonos’s picture

andypost’s picture

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

Gábor Hojtsy’s picture

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.

andypost’s picture

Issue tags: +Needs reroll
zvischutz’s picture

FileSize
14.2 KB
PASSED: [[SimpleTest]]: [MySQL] 59,729 pass(es). View

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

zvischutz’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 105: drupal8.forms-system.1842036-105.patch, failed testing.

martin107’s picture

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

martin107’s picture

Status: Needs work » Needs review
pfrenssen’s picture

FileSize
14.2 KB
FAILED: [[SimpleTest]]: [MySQL] 59,323 pass(es), 2 fail(s), and 1 exception(s). View

Straight reroll.

Status: Needs review » Needs work

The last submitted patch, 110: drupal8.forms-system.1842036-110.patch, failed testing.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

Going to work on this.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
15.25 KB
PASSED: [[SimpleTest]]: [MySQL] 59,421 pass(es). View
1.05 KB

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.

Crell’s picture

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

dawehner’s picture

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.

tstoeckler’s picture

Status: Needs review » Needs work
+++ 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 :-))

pfrenssen’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.29 KB
FAILED: [[SimpleTest]]: [MySQL] 59,869 pass(es), 1 fail(s), and 0 exception(s). View

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

dawehner’s picture

FileSize
15.29 KB
FAILED: [[SimpleTest]]: [MySQL] 59,747 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 119: modal-1842036.patch, failed testing.

The last submitted patch, 118: modal-1842036.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
4.98 KB
17.67 KB
FAILED: [[SimpleTest]]: [MySQL] 59,782 pass(es), 3 fail(s), and 1 exception(s). View

Some more fixes.

Status: Needs review » Needs work

The last submitted patch, 122: 1842036-122.patch, failed testing.

alvar0hurtad0’s picture

Assigned: Unassigned » alvar0hurtad0
alvar0hurtad0’s picture

Assigned: alvar0hurtad0 » Unassigned
InternetDevels’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
18.33 KB
PASSED: [[SimpleTest]]: [MySQL] 63,110 pass(es). View
1.36 KB

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);
     }
Xano’s picture

Status: Needs review » Closed (won't fix)
Related issues: +#2253257: Use a modal for entity delete operation links

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

andypost’s picture

Title: Convert all confirm forms already converted to new routing system to use modal dialog » [META] Convert all confirm forms already converted to new routing system to use modal dialog
Status: Closed (won't fix) » Active

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

cmanalansan’s picture

Issue tags: -Novice

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

andypost’s picture

looks that should go to 8.1

Crell’s picture

Version: 8.0.x-dev » 8.1.x-dev

Concur.

pfrenssen’s picture

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?

pfrenssen’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.