Use a modal for entity delete operation links

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 ✨ Use a modal for content entity form delete links confirmation forms Closed: duplicate 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

tim.plunkett → credited olli → .

tim.plunkett → credited pooja saraah → .

tim.plunkett → credited runeasgar → .

Adding credits for work done on ✨ Use a modal for content entity form delete links confirmation forms Closed: duplicate

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: ✨ Use modals on the Manage Fields page Needs work .

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.

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.

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 📌 Focus is lost on dialog close if the opener is inside a collapsible element Fixed 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 📌 Focus is lost on dialog close if the opener is inside a collapsible element Fixed 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?

bnjmnm → closed merge request !3700

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 📌 Allow AJAX to use GET requests Fixed . 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: 🐛 Ajax state leaking to Views destination paths Fixed .

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