Use a modal for entity delete operation links

Looks like the modal title isn't coming through correctly.
I would expect to see the question 'Are you sure you want to delete' etc.

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -123,6 +123,10 @@ protected function getDefaultOperations(EntityInterface $entity) {
+          'class' => array('use-ajax'),
+          'data-accepts' => 'application/vnd.drupal-modal',

If you add

      'data-dialog-options' => Json::encode(array(
        'width' => 'auto',
      )),

Then it will fit just fine.

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -122,6 +122,13 @@ protected function getDefaultOperations(EntityInterface $entity) {
+          'class' => array('use-ajax'),
+          'data-accepts' => 'application/vnd.drupal-modal',
+          'data-dialog-options' => Json::encode(array(
+            'width' => 'auto',
+          )),

Out of scope for this issue, but it'd be realllly nice if we had a way to just have 'class' => array('use-modal'), and it would expand to all of this.

There is an issue for that #2170541: Allow dialog ajax definition to be less verbose

Steps to test:

Visit /admin/structure/types, click delete on the content types

Content type with content:

Issues:

• Needs an action button to dismiss the dialog (not just a close cross in the corner)
• Double escaped HTML in the dialog header

Content type with no content:

Issues:

• Cancel link should be next to the delete link - seems weird in the modal body
• Double escaped HTML in the dialog header

rerolled - not sure what the best strategy is for the escaped titles.

Isn't that the same kind of problem as what got tackled on #2207247: Dialog titles double escaped for views handlers and delete forms ?

@dawehner indeed

rerolled

Guess we should sending it to testbot.

The other one seems to have an okay from nod_, so let's postpone this issue on top of the other one?

And.. its one year later ;)

I've just RTBC'd #2207247: Dialog titles double escaped for views handlers and delete forms as nobody else wanted to do it. Maybe we can try this again for 8.x-1.x. Note that many config delete forms got quite a bit bigger due to config dependency information but I guess this still makes sense.

This is just a re-roll of #24--it doesn't appear to be working though as something must have changed in the past few years. We can also now add javascript tests for this. I came across this issue while working on #2828201: Add a modal form option to the confirm and field entry form action link types.

See #2488192: Modal/dialog/ajax is using query parameters instead of accept headers, you want data-dialog-type now, no longer the arcane accepts header.

Thanks @berdir!

This is now working--still probably needs a js test though.

Fixes the failing tests.

This has the start of a test. See the @todo for some odd behavior. I'd expect a new page load with a confirmation message on submit. However, the entity does appear to be deleted...

That's a failure I haven't come across before:

Current page is "/entity_test/list", but "/checkout/entity_test/list" expected.

I would have thought that $this->assertSession()->addressEquals(Url::fromRoute('entity.entity_test.collection')->toString()); would take a subdirectory into account...

@jhedstrom: Looks to me like it fails *because* it takes that into account: We actually are on /checkout/entity_test/list and it sees just /entity_test/list but toString() includes the directory. Try '/' . getInternalPath(), or just hardcode it..

Ah, I was reading that backwards. I made the change suggested in #42, and have also changed to not use the loadUnchanged(), and instead just manually cleared the cache (there is an @todo on the method that suggests its behavior will change in the future).

I am still not sure why the page isn't reloading after clicking on the delete button.

I am still not sure why the page isn't reloading after clicking on the delete button.

I figured out why this appears to be the case. Apparently when using clickLink, the verbose page visit logging does not take place, so it only appeared there was no page reload when I was looking at the pages generated by the html output printer.

Given #44, this removes the @todo and just checks for the delete message to be present on the page.

Rerolled the patch #45.

Love it.

there's a little problem which is similar to this: #2741877: Nested modals don't work: opening a modal from a modal closes the original

We can assign a new selector to avoid this problem. @see #2741877 Comment 3. (also @see #2741877 Comment 12 for my points)

Thanks.

Patch stil applies cleanly, looks great, and it seems to work as advertised (tested it a bit manually).

One small nitpick:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Entity/EntityListBuilderTest.php
@@ -0,0 +1,72 @@
\ No newline at end of file

Fixing that on the go with this patch (please don't credit me for this!).

Anything else we need to be doing here?

Loading icon is coming on next line(ref: screenshot). I guess @Manuel Garcia has fixed it in #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait

re #50: Yeah that's a css bug in seven, but my fix for it sits on #26 of #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait
https://www.drupal.org/files/issues/interdiff_26071.txt

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -150,6 +151,13 @@ protected function getDefaultOperations(EntityInterface $entity) {
+            'width' => 'auto',

+++ b/core/modules/config/src/Tests/ConfigEntityListTest.php
@@ -64,6 +65,13 @@ public function testList() {
+            'width' => 'auto',

@@ -134,6 +142,13 @@ public function testList() {
+            'width' => 'auto',

On admin/structure/block, we use width 700, which works great, and is still responsive friendly.

Can we use the same width here?

I tried twice on simplytest with the latest patch but did not get a modal when deleting an article. Am I missing something?

yeah, this issue is for config entities. Content entities are in #2254935: Use a modal for content entity form delete links confirmation forms. Updated titles on both issues :)

Just a re-roll

Thanks for the rerroll @chr.fritsch

About the issue mentioned around the loading icon on #50 - The fix that was originally part of #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait was split off into its own issue, and was already fixed: #2909882: Throbbers showing within dropbuttons jump to next line - We should probably verify it is now working in this situation just in case.

I checked the use-case from @vijaycs85 in #50 and can verify that this issue is already fixed

Re #52:

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -154,6 +155,13 @@ protected function getDefaultOperations(EntityInterface $entity) {
+          'data-dialog-options' => Json::encode([
+            'width' => 'auto',
+          ]),

+++ b/core/modules/config/tests/src/Functional/ConfigEntityListTest.php
@@ -67,6 +68,13 @@ public function testList() {
+          'data-dialog-options' => Json::encode([
+            'width' => 'auto',
+          ]),

@@ -137,6 +145,13 @@ public function testList() {
+          'data-dialog-options' => Json::encode([
+            'width' => 'auto',
+          ]),

I have checked in core, and in all places we're using a width of 700:

• BlockListBuilder
• BlockLibraryController
• PlaceBlockPageVariant
• ContentModerationConfigureForm

Only on \Drupal\off_canvas_test\Controller\TestController do we use a different width (555), but that is only for tests.

Can we switch to using width 700?

Re #59 - thanks @chr.fritsch for checking, great to hear that is now fixed :)

Sure, I updated the patch

Thanks @chr.fritsch - that test failure:

1) Drupal\Tests\views\Kernel\Plugin\RowRenderCacheTest::testAdvancedCaching
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'  EditDelete'
+'  EditDelete'

+++ b/core/modules/views/tests/src/Kernel/Plugin/RowRenderCacheTest.php
@@ -181,7 +183,7 @@ protected function doTestRenderedOutput(AccountInterface $account, $check_cache
-        '<li><a href="' . $node_url . '/delete?destination=/" hreflang="en">Delete</a></li>' .
+        '<li><a href="' . $node_url . '/delete?destination=/" class="use-ajax" data-dialog-type="modal" data-dialog-options="' . Html::escape(Json::encode(['width' => 'auto'])) . '" hreflang="en">Delete</a></li>' .

I'm guessing we should also change the width here, sorry forgot to mention earlier. Not entirely sure it's why the test is failing though, so we'll see...

Fix tests

For me, this works on the entity list, but not on the entity edit form...

Ah maybe it works only for admin theme routes?

Thanks @chr.fritsch, that was it then :)

@k4v this issue is for the entity operations links only, which in core are only used on the entity list pages (collection routes). It should work anywhere you use this via views as well.

I can find no other issues with the latest patch, so RTBC++

Implementation looks good to me.

Only nitpick:

+    // Should still be on the listing page.
+    $this->assertSession()
+      ->addressEquals(Url::fromRoute('entity.entity_test.collection')
+        ->getInternalPath());

+    $this->assertSession()
+      ->pageTextContains(t('The test entity Test entity 3 has been deleted.'));

It's weird to me to see method calls broken down into multiple lines, especially when the indentation in the first case doesn't even match. Guessing it was done automatically by phpstorm, and then not corrected.

Feel free to ignore this if core is fine with such a style.

Good point @bojanz - I don't believe this is valid?

Improved the nitpick mentioned in #68.

Great!

Awesome thanks all!

@k4v re #66 - I've just realized that we are already working on the entity edit form delete link, see #2254935: Use a modal for content entity form delete links confirmation forms

Just noticed this has RTBC. Some manual a11y testing will be a good idea here, to avoid a serious regression. We're using an existing pattern so hopefully it'll be fine, but I'd like to check everything is wired up correctly here, labels make sense, etc.

Can someone please move commit credits here from #1842036: [META] Convert all confirm forms to use modal dialog?

@lauriii pointed out in slack that I can do it myself. As the initial version of this patch was posted in #1842036: [META] Convert all confirm forms to use modal dialog so I went ahead added the patch contributors from there to this issue.

Needed a re-roll after #2935255: Remove all usages of drupal_set_message and drupal_get_messages in core lib.

Moving back to RTBC, because this patch just got rerolls sincs #70 and I changed EntityListBuilderTest to use WebDriverTestBase now.

This is still tagged as needing usability and accessibility review, and I don't think that happened since being requested in #74. So ticking back for now, sorry.

oh true, sorry about that.

Not much activity on this in a while, are people still interested in this? To me it sounds like a good idea.

I'm happy to schedule this in for a UX Call, but we'd want some up-to-date screenshots in the issue summary (there are ones early in the issue but they use old styling).

Issues summary also generally needs an update with more description and using the templates.

Thanks.

Re-roll patch given for 9.3.x, Remains Need Work because of #98

Both the patches are same, By mistake added, Please ignore.

Added "$defaultTheme" and "void return typehint for ::setup()" in EntityListBuilderTest file.

Rerolled and updated test in patch in #108

Verified the patch #108 and tested it on Drupal version 10.1.x. The patch works fine and I have added the before and after screenshots for reference.

Verified and tested patch #108 on 10.1.x-dev and patch applied cleanly.

Test Result:
1. Links to delete entities via operation links open as a confirmation model with "Delete" and "cancel" buttons - works fine for config entities (content type, menus, taxonomy, custom blocks), terms and node content.
2. When we delete menu link via operation link (/admin/structure/menu/manage/test-menu) then it opens a new page. Expected is that this should open in a modal similar to terms and node.
Please refer attached after screenshot of menu link.

I see title says "config entities" but in #2254935: Use a modal for content entity form delete links confirmation forms it is mentioned that delete entities via operation links irrespective of config or content to be handled in this issue #2253257.

The title of this issue was updated in #54 to use "config entities", which IMO is incorrect. Please correct me if I am missing anything here.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

There are still many needs tags on this issue.

-

It wasnt clear skimming the issue how I would be able to opt out of this behavior, in case a contrib module or site specific functionality enhanced the delete form with more UI. It wouldn't be ideal if we changed this to a modal making existing UI unusable.

At least, we should describe ways in code to override this in a change notice.

Screenshots provided in #110 so removing tag

Adding credits for work done on #2254935: Use a modal for content entity form delete links confirmation forms

So going to give my best accessibility review

Clicking delete content type the modal opens (Assuming the modal has been tested enough it passes)
Focus is on the X button of the modal
Tabbing takes me through all the elements
Focus stays within the modal.

But the modal does not open when deleting a field.

Last anyone on the accessibility team gave this a review was when @andrewmacpherson looked at it 5 years ago. His summary at the time was that it needed a manual review when it was good to go.

As he pointed out at the time, this is a pattern we're already using and testing elsewhere.

@smustgrave So with your review of the content type, I assume that there is no problem closing the modal. I can't see there being links or other focusable elements aside from the "Delete" & "Cancel" buttons.

What about your report about the modal not opening when deleting the field. Is that a new issue, or something needs to be fixed & then re-tested here.

Generally this looks good though. Thanks for doing the keyboard-only testing for this.

Re #122

But the modal does not open when deleting a field.

That is not in the scope of this issue. There is a separate issue for that: #2880003: Use modals on the Manage Fields page.

When using this feature on content with translations, there is a contrast issue with the "delete all translations" button. In Claro theme.css loads after button.css, so theme's .ui-widget styling overrides the button.css button's background color but not the text color, so it winds up white text on a light gray background.

While this is not due to the changes here, landing this as-is would increase the likelihood of users encountering this accessibility bug. This can probably be best addressed in a followup that postpones this issue, but if there's a good argument for addressing it here that's an option. If the work is done here, the "Needs followup" tag can be removed.

Re comment #124, I would strongly suggest that we fix that bug first, even if it's not caused by this issue we would be making the user experience worse by committing this without fixing that problem, so I would definitely support postponing this issue on fixing that bug first.

I tried to reproduce issue mentioned in #124, but it worked well for me.
Please see attached screenshots.

Re #126

I tried to reproduce issue mentioned in #124, but it worked well for me.
Please see attached screenshots.

I added testDeleteModal() to Drupal\FunctionalJavascriptTests\Theme\ClaroViewsBulkOperationsTest to confirm that this is happening. Creating that test also exposed the fact that the modal does not work if toolbar isn't present. I suspect this is because the drupal.ajax library is not loaded unless that is the case.

The testDeleteModal() test can probably stick around in some form, be it in this issue or another one, but I could see it changing a bit when it becomes test coverage to confirm functionality instead of its current role as coverage to prove a problem exists.

I think the problem can be addressed if core/drupal.dialog is added on page load instead of when the delete button is pressed. When that library is invoked on AJAX request it reloads Claro's theme.css because it is in the definition of a library that is not yet present. That CSS file is appended as the last item in <head>, and takes precedence over Claro's button styling due to appearing after the button CSS. The asset has a lower weight, but when it's added via AJAX that isn't taken into account.

Addressed feedback raised in #122 and #124.

#124 mentioned the delete field was out of scope and covered in another ticket. That change should be reverted.

Unless the scope of this issue is being expanded

Addressed #130.

The latest change includes things that should not appear in the MR.

Re #130

#124 mentioned the delete field was out of scope and covered in another ticket. That change should be reverted.

That's not accurate. When you mentioned "But the modal does not open when deleting a field." I said that was out of scope as deleting a field is not in scope. This whole issue is about creating modals for delete link confirmation forms, it's just specific to content entities, so fields are not included in that scope.

The latest change includes things that should not appear in the MR.

I'm not spotting any of this - could you add some comments on the MR?

****************************************************************************************
This needs an issue summary update, whether or not the scope changes here.

Although it's not the current scope, there may be benefits to including field deletion in this issues scope, and such a change was discussed. Were that to change, the issue summary should be updated and. If that doesn't change, the issue summary should still be updated because the screenshot there is a confirmation modal of a field being deleted, not a content entity. The screenshot is fine if the scope is broadened, but should be replaced if not.

I clicked on the compare with previous version link, and saw changes in settings.php (https://git.drupalcode.org/project/drupal/-/merge_requests/3700/diffs?di...) I can't see it in https://git.drupalcode.org/project/drupal/-/merge_requests/3700/diffs, so that comment can be scratched.

Updated IS. Added note about #133

There needs to be FunctionalJavascript test coverage for both the field and entity delete links. The only thing that resembles that is a test I added to ClaroViewsBulkOperationsTestto prove that #124 was in fact happening. Much of that can probably be repurposed to create the tests for the two delete modals, but the functionality should get test coverage, that isn't in the tests for a specific theme, and that actually deletes something as part of the tests (the test I wrote is just proving a button looks bad).

On the one thread

I think we should also attach this library to the entity form.

Can that still happen please.

Left a question about a missing test.

This probably shouldn't go in until #3359494: Focus is lost on dialog close if the opener is inside a collapsible element lands, but I don't want to set this to postponed as it's still fine to review this. The one thing a reviewer should note, however, is right now focus will be lost on modal-close if that modal was opened with something inside a dropbutton. That will be fixed by the aforementioned issue.

There is a module for this https://www.drupal.org/project/admin_dialogs

Addressed all the feedbacks.

Left feedback in MR of additional things that need doing.

Addressed all the feedbacks.

This seems pretty close. It looks like one of the reviews asked to move the test coverage for content type deletion but it wound up getting removed instead. That should probably get a home somewhere in the test coverage and this might be ready to go after that, but at that point we may have to postpone on #3359494: Focus is lost on dialog close if the opener is inside a collapsible element as I wouldn't want to add all these modals without focus going to somewhere logical when the modal closes.

Speaking of focus management... As long as #3359494 is taken care of then this passes accessibility review. This doesn't introduce anything that hasn't already been vetted in other confirmation dialogs within core, and I put it through additional manual tests to confirm there was no new surface for problems & that the page-titles-now-dialog-titles still make sense.

Should this remain in 11.x to be backported?

Should this remain in 11.x to be backported?

Technically yes, good call. I want to do it a little differently because this was started pre-11 and has a bunch of threads.

The MR is against 10.1.x and it can get ugly bumping up an MR against version that didn't exist when the fork was created. Sometimes it's necessary to just start a new MR that wipes out the threads. Fortunately it won't impact the review process as nothing here is fundamentally different in 11.x with the files being changed. If we can delay the 11-ifying until a pretty solid RTBC, the switch is a little more painless

I added the content type delete operation test coverage to the node module since it's essentially provided by the node module, and doesn't have a dependency on other modules. I decided to reuse the \Drupal\Tests\node\FunctionalJavascript\NodeDeleteConfirmTest since it seems a bit overkill to have a separate test class for node type delete confirmation. Also, the set-up would be nearly identical for these two.

I tagged this for needs usability review 5 years ago. We have been running many iterations of user interviews this spring where we have converted existing forms to modal dialogs and the feedback has been positive there. I don't think this needs further investigation so reverting the tag.

Had an 11x branch ready to push but Gitlab does not appear to be working so here's the 11x in classic patch format.

When this issue was created, the top movie in the US was "Captain America: The Winter Soldier"

Today we witness "Issue 2253257: Committed to 11.x and cherry-picked to 10.1.x". Stick around after the issue credits for a sneak peek at some character that will appear in the next commit!

Will it have change record or release notes mention?

This is a really great UX improvement, it would be nice to highlight this in the release notes.

Well done everyone!

This is really broken on ajax views. Deleting an entity from the second page of the view takes me to the following path:

/admin/content?page=1&ajax_page_state[theme]=claro&ajax_page_state[theme_token]=OUqFxWhAOwXtkRPcSAdL4jbOuw8Cz4O-QlNSqWuM82c&ajax_page_state[libraries]=big_pipe/big_pipe%2Cclaro/drupal.nav-tabs%2Cclaro/global-styling%2Ccontextual/drupal.contextual-links%2Ccontextual/drupal.contextual-toolbar%2Ccore/drupal.active-link%2Ccore/drupal.dropbutton%2Ccore/drupal.tableheader%2Ccore/drupal.tableresponsive%2Ccore/drupal.tableselect%2Ccore/normalize%2Cshortcut/drupal.shortcut%2Csystem/admin%2Csystem/base%2Ctoolbar/toolbar%2Ctoolbar/toolbar.escapeAdmin%2Ctour/tour%2Cuser/drupal.user.icons%2Cviews/views.ajax%2Cviews/views.module.

I tried enabling ajax views and delete node on the second page and it's working 🤔

@mstrelan I still can't reproduce the bug you reported :( Could you by any chance try if the regression gets addressed by this diff?

@lauriii it seems to only happen with big pipe enabled. Full steps to reproduce:

1. Install Drupal 11.x with standard profile - commit hash 18432dacb5
2. Enable ajax on the /admin/content view
3. Create one article node
4. Visit /admin/content and apply a filter (e.g. Type = Article)
5. Delete the article

Applying the patch was slightly better, but I was redirected to the front page instead of back to the admin content view.

FWIW I tested first with a custom docker compose setup and again with ddev default config for Drupal 10.

I could reproduce the problem you mentioned on #167 with #166 but I still can't reproduce #164.

My hypothesis is that there's something wrong about the libraries on the page. Here's another issue which tries to make loading the libraries more consistent. Could you test if this has any impact on #164?

With the help of #167, I was able to reproduce this now. The bug reported in #164 is not actually caused by this issue but #956186: Allow AJAX to use GET requests. I'm marking this as fixed since that regression is not made better or worse by this issue.

We may still want to commit something along the lines of #168 though because it addresses some dialog styles in Claro.

Filed the follow-up issue: #3364088: Ajax state leaking to Views destination paths.