Use modals on the Manage Fields page

Feels a bit odd to special case this UI.
Why don't we open an issue to modalize all delete forms? They all pretty much look the same, so they share the same benefit of being in a modal.

Great start @manuel-garcia

Probably we can have a no-patch/policy issue to discuss and fix common problems and approach and can have child issues to group by functionality (i.e.. all delete form) or component (all forms in field UI) based depending on size.

Some of the common problems:

1. +++ b/core/modules/field_ui/src/FieldConfigListBuilder.php
   @@ -132,7 +133,16 @@ public function buildRow(EntityInterface $field_config) {
   +                'title' => $this->t('Edit field settings.'),
   +                'class' => ['use-ajax'],
   +                'data-dialog-type' => 'modal',
   +                'data-dialog-options' => Json::encode([
   +                  'width' => 700,
   

   This is repeating for every item. probably we can have a common place and elements can have one element attribute to use these values as it is and allow to override, if necessary.

2. Accessibility review

Thanks for finding related issues @Manuel Garcia. Few things from related issues:

1. Width: Seems we can leave the width to 'auto' instead of fixed 700
2. Adding tests like we have in #2253257-49: Use a modal for entity delete operation links

Dialogs are tricky, at the very least you need to use there:
$assert_session->waitForElementVisible('named_exact', ...), but that likely won't be 100%, see #2856047: Avoid random failures in JavascriptTestBase when testing functionality in a dialog for more.

You can use Drupal\Tests\views_ui\FunctionalJavascript\FilterCriteriaTest as a template, it should have all you need as it also is testing dropbutton with dialog, and works most of the time.

@Manuel Garcia: it fails at $modal = $page->findById('drupal-modal'); I'll investigate why in test issue.

It was mostly because of missing:
$build['#attached']['library'][] = 'core/drupal.dialog.ajax';.

You can see the screenshots, the bad height is due to debounce on the dialog (here's the patch to create them).

Re-rolled the last patch for 9.3

Fixed some fail tests, Updated functional javascript tests according to drupal 9.3.x standard.

Fixed Tests, Please have a look.

Verified and tested patch#40 on the drupal 9.3.x-dev version. Patch applied successfully and looks good to me.

Testing Steps:

1. Install drupal 9.3.x-dev version.
2. Go to /admin/structure/types/manage/article/fields
3. Click on Edit or Storage Setting or Delete option and observe the modal not displayed to manage the field setting.
4. Now apply the patch and again verify by clicking on the Edit or Storage Setting or Delete option modal open to manage the field setting.

Expected Result:

1. By clicking on the Edit or Storage Setting or Delete option modal should open to manage the field setting.
2. Modal should display properly and work expected.

Actual Result:

1. By clicking on the Edit or Storage Setting or Delete option modal opens to manage the field setting.
2. Also, that modal displayed properly on the Mobile and desktop.

For reference, please refer to the attached screen recording video.

Move this ticket to RTBC.

#2088121: Remove Overlay

Applied patch #40 working fine now i am able to use modals in manage fields for reference sharing screenshot

The gif in the IS is from 2009! The Issue Summary definitely needs to be updated. This makes it easier for any reviewer and committer to do their work. I suggest adding the standard template and completing all the sections. See Write an issue summary for an existing issue for guidance on doing that.

This is also a fairly big change and I wonder what approval it may need (still learning that). And it would be helpful to know if there is any criteria or goals in the Meta that are to be included in the patch.

@manojithape, thanks for the detailed testing steps and screenshots. They should go in the Issue Summary. Also, a working patch is not sufficient to mark an issue RTBC. There are several steps, or core gates that an issue must pass first.

@vikashsoni, there is already a video of the patch being used, there is no need for more. Therefore removing credit per How is credit granted for Drupal core issues.

I skimmed the patch and found this small change.

1. +++ b/core/modules/field_ui/tests/src/FunctionalJavascript/FieldConfigListBuilderTest.php
   @@ -0,0 +1,137 @@
   +    $this->adminUser = $this->drupalCreateUser(['administer content types', 'administer node fields']);
   

   This would be easier to read if on multiple lines.

2. +++ b/core/modules/field_ui/tests/src/FunctionalJavascript/FieldConfigListBuilderTest.php
   @@ -0,0 +1,137 @@
   +   * Test that field type column storage forms use a modal dialog.
   

   s/Test/Tests/

Making the changes as suggested in #46, please check.

Rerolled #47 to 10.1.x.

Added an MR with all tests passing. Left a few things that need addressing in the MR.

MR appears to have issues.

Could there be a way for a site to opt out of this? Modals are neat but for a number of our sites I don't imagine we would want this.
Tagging for usability and accessibility review.

Was previously tagged for issue summary which still appears to need and happen.

Thank you for your feedback @smustgrave! At the moment there isn't a way to opt out of this. I'm curious to know why you believe using modals would be a regression for your sites. 😊 Is there something that we should change about the implementation that would make the user experience more seamless?

Let me ask, could there be a scenario where a custom module with a custom field could break with this change? And there be no way to edit your field.

Other concern is accessibility. Know Drupal backend is not the best with that but adding to it would seem bad to me.

This almost reminds of the overlay module of Drupal 7. Think I got that right.

Let me ask, could there be a scenario where a custom module with a custom field could break with this change? And there be no way to edit your field.

You're right that this could be disruptive to a custom field type which configuration form relies on JavaScript (and the JavaScript hasn't been written according to Drupals best practices). Do we have any examples of field types that would ship JavaScript for their admin form?

There certainly would be workarounds (i.e. open the link in a new tab) to this but it wouldn't be ideal to have users rely on that. We would also do something like this only in a future minor giving people time to test and fix their custom code.

Other concern is accessibility. Know Drupal backend is not the best with that but adding to it would seem bad to me.

Do you have specific accessibility concerns in mind? I would love to make sure that we are taking those into account 😊

We certainly want to make sure that the solution is accessible and easy to use for all users. This has already received some feedback regarding accessibility from @bnjmnm who is a provisional accessibility topic maintainer, and I know that he's keeping a close eye on this. We are also using dialogs in some pre-existing UIs such as Layout Builder, meaning that it is a pre-existing pattern which has received review in the past.

This almost reminds of the overlay module of Drupal 7. Think I got that right.

Overlay opened all of the admin pages in a dialog. While this is also utilizing dialogs, it is not exactly the same since we are only using it for specific actions. This is similar to the block UI where a block can be added through a dialog, without having to navigate to a separate page. What is interesting about the block UI is that it is not using dialogs for editing blocks 🤔

Don't quote me but my only knowledge of modals is that focus must remain inside the modal while it's open for tabbed users. Must be able to be closed via tab. Not sure what/if any aria attributes it may need.

We are utilizing the built-in Drupal Dialog (which is using jQuery UI Dialog) which should have all of the necessary accessibility considerations implemented. AFAIK it cannot be closed with a tab and I've personally not heard of that before. If that's a requirement for modal dialogs to be accessible, we should probably open a new issue to investigate that in relation to all of our dialogs.

One issue we are aware of specifically in relation to this issue is that when a dialog is opened using a link that is inside dropbutton, focus is not returned to the triggering element because it is no longer focusable. This is something that will need to be addressed as part of this issue.

I removed a lot of unrelated code that was from prototyping other Field UI changes.

I also fixed the next few failing steps in EntityReferenceAdminTest.

However, I highlighted (see the newly add @todo's) the problem we're still facing.
FormBuilder has this code:

    // If this form is an AJAX request, disable all form redirects.
    if ($ajax_form_request = $request->query->has(static::AJAX_FORM_REQUEST)) {
      $form_state->disableRedirect();
    }

which is good, because we don't want it to redirect, but bad, because we have to figure out how to share the destination logic between the submit and the ajaxSubmit.

Meaning, if JS is disabled, things work as expected, but with it on, the AJAX code doesn't know where to take you next. And each form in the chain needs to be fixed.

All of those concerns are for the Add Field flow, which is extremely complicated right now. That portion could be handled in a follow-up, while this can make the links in the dropbutton use a modal.
But until we decide to do that, postponing this on #3347291: Combine field storage and field instance forms

Converting the field creation flow to use modal is blocked by #3358049: Save FieldStorageConfig at the same time as FieldConfig. This would be also simplified by #3347291: Combine field storage and field instance forms, but this isn't a hard requirement for working on this issue.

In Comment #46, @quietone asked for an issue summary update. I am not familiar enough with the current work to update the description myself, but as a first step I have added the standard template. I am keeping the "Needs issue summary update" issue tag.

In Comment #70, this issue was postponed on #3347291: Combine field storage and field instance forms. Comment #72 also mentions #3358049: Save FieldStorageConfig at the same time as FieldConfig. Both of those issues have landed, so I am un-postponing this issue. If it should still be postponed, then please mention the blocking issue in the "Remaining tasks" section of the issue summary. (See the link in Comment #46.)