Use a modal for content entity form delete links confirmation forms

Looks like the question isn't coming through as the title.

Note #2207247: Dialog titles double escaped for views handlers and delete forms

Yes

+++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
@@ -62,6 +63,13 @@ public static function buildCancelLink(ConfirmFormInterface $form, Request $requ
+      'data-dialog-options' => Json::encode(array(
+        'width' => 'auto',
+      )),

Shouldn't this be in the delete link?

8: drupal-2254935-8.patch queued for re-testing.

I am not a huge fan of confirm dialogs, as they turn into a habit even when it concerns critical messages. However I don't see the current step as a bad thing. I have a few questions:

• Does this work well on mobile? I am not sure how well tested our confirm dialogs are.
• Shouldn't we also do this with configuration forms, it seems like going without that will create quite a inconsistency. Also I assume that will be much harder.

The confirm dialog is a global component, if it's not mobile friendly yet then we need to fix that globally. It shouldn't affect the decision to use a dialog on a case per case basis.

Deleting something should follow a consistent pattern, regardless if it's an entity or not. For example, deleting an URL alias has a confirmation step similar to the one for entities:

Lewisnyman: the question is whether a case-by-case approach helps us closer to release. Not sure it does.

Understood that the confirmation step stays. I meant to say that *the process of deleting something* (entity or whatever) in the UI should follow the same pattern. Moving it into a modal for only some of the delete operations makes it less consistent.

I have no objections to the use of the modal itself. Just cautious to apply it only in some places, some not.

Ok, so best next step seems to be to get a list of the non-entity delete forms in core that would not be touched by this patch so that we can see how many exceptions we'd introduce and assess how bad that might be.

Since the parent issue is for 8.4, I am updating this one to be the same.

Re-roll against 8.4.x.

Also the space between the delete link and loading icon seems bit too much. Attaching screenshot.

Updating test fail.

Jot a quick note:

I think we need to test nested Modal with this new feature.

Fixing the failing tests, and expanding a bit the tests on there to check for the modals.

Re: #33: I think nested modals are not working in core at the moment, see #2741877: Nested modals don't work: opening a modal from a modal closes the original

One quick note is that this only seems to work from the delete link while already editing the thing. There's a "Delete" tab on the front-end view of content items where I'd expect the same behaviour but no modal there (yet).

@yoroy my understanding is that this issue is for the entity edit form only.

Another place that is not covered by this patch would be the delete link on the operations links (for example on the /admin/content view)

In any case, I agree from a UX perspective that we should do this for any link pointing to a delete entity form so that it is the same expected behavior.

I guess the question is: Should we expand the scope of this issue or not?

The other ways to delete content are already being handled in separate issues. Please scroll to the top of this issue and see the parent issue and related issues.

Once more proof people don't see things in sidebars :) I see that is covered indeed.

We've got functional javascript tests covering the functionality introduced, so removing the tags.

Latest patch still applies cleanly, anything else we should be doing here?

+++ b/core/lib/Drupal/Core/Entity/EntityForm.php
@@ -244,10 +245,15 @@ protected function actions(array $form, FormStateInterface $form_state) {
+          'data-dialog-options' => Json::encode(['width' => 'auto'])

We're standardizing on width 700 for core here, so fixing that. Also moving that to a new line so as to keep the same format as elsewhere in core.

Testing.

1. Confirming undesirable behavior by creating an article, and deleting it: confirmed. I am taken to a page with the option to delete.
2. Applying patch: $ curl -l https://www.drupal.org/files/issues/2018-04-17/2254935-45.patch | git apply -v
3. Applies cleanly.
4. Creating another article.
5. drush cr
6. Deleting via node tab: does not work - I still get sent to a page.
7. Editing the article, then choosing delete: success, I receive a modal (screenshot below).
8. Trying this on /admin/content for good measure: does not work - I still get sent to a page..

Looks like this ticket is explicitly for the content entity form screen, so marking this as RTBC because it meets the expectations of the issue title. But, I do wonder if this modal should be available for node (or even more entities) deletes in all contexts.

Forgot the screenshot:

Thank you @runeasgar!

You are correct, this issue's scope is only about the delete link on the entity edit forms.

Just noticed this issue and #2253257: Use a modal for entity delete operation links are both RTBC.

Some manual aaccessibility 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.

Finally got to test this manually again. It's not really a useful change if we introduce two ways this confirmation pattern works. I think we should include the tab (on entity display and dropbutton links (in listings) to do the same. This is a super useful and userfriendly change but we should make this the new default then for all ways you can delete entities. Sorry for not making that call earlier.

Yeah you're right @yoroy - let's just expand the scope of this then.

Updating the issue summary and title accordingly.

Re #50 - the entity operation delete links (ie on admin/content) are being tackled here #2253257: Use a modal for entity delete operation links

Updating the Issue summary to clarify the scope and remaining tasks.

Fixed failed commands on #64
Attached patch against Drupal 10.1.x

Changed the operations list to use modal for delete confirmation.

While I did that, I noticed similar issues that @bnjmnm reported on #2880003: Use modals on the Manage Fields page where the focus moved to the window when returning from dialog because the button from toolbar is no longer focusable.

Fixed failed tests from #66

Agree this needs accessibility review as it provides a new popup functionality.

Tested #70
When the modal opens the focus goes to the "Delete" button - which is good
Tabbing does not leave the module. "Delete" button -> "Cancel" button -> X button in the corner - while is good

This is the markup of the modal

<div tabindex="-1" role="dialog" class="ui-dialog ui-corner-all ui-widget ui-widget-content ui-front ui-dialog-buttons" aria-describedby="drupal-modal" aria-labelledby="ui-id-1" 

role = dialog seems correct
aria-describedby and aria-labelledby are the ones I'm not 100% about.

Think accessibility

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.

@smustgrave what are the aria-describedby="drupal-modal" and aria-labelledby="ui-id-1" describing?

These are usually very accurate:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attribut...
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attribut...

These might be:
https://benmyers.dev/blog/aria-labels-and-descriptions/
https://cccaccessibility.org/web-1/web-developer-tutorials/aria-labelled...

This issue is confusingly similar to #2253257: Use a modal for entity delete operation links. While this is intended for content entities and the other for config entities, there is not currently a split implementation between the two. Both content and config entities rely on \Drupal\Core\Entity\EntityForm, and the code changes on the two issues are almost identical. The only difference is test coverage.

Since this is the "newer" issue and the other one has a more recently updated MR, I'm going to mark this as a duplicate. I will transfer contribution credit over to the other issue.

Thanks @tim.plunkett

From both a code maintainability and usability perspective, the more consistency we have across the admin UI the better.