Use modal in add new field flow

Current MR Overview

1. FieldStorageAddForm has a 'Continue' link with a route that points to FieldTempStoreController. The route gets updated every time a field is selected so the correct parameters are set. From FieldStorageAddForm, a "dummy" field name is generated since one is needed to create an entity. This dummy name is necessary now because we moved the field name label to the edit form.

2. Inside FieldTempStoreController::setTempStore we create the entity and set the field and field storage values inside of temp store. Then FieldTempStoreController redirects to another controller FieldConfigAddController which builds the entity form using the entity that was created and then it returns the edit form.

3. Now inside FieldConfigEditForm::validateForm, we create the entity we actually want with the new field name since we have access to that value on this form now.

TODOs so far....
There are 4 todo comments I added in the MR.

1. In FieldStorageAddForm, we added core/drupal.machine-name, core/drupal.states, as a temporary workaround because the machine name on the edit form modal was not running those libraries.

2. machine-name.js Related to #1, comment is in todo

3. Right now, the 'Continue' button on FieldStorageAddForm appears when a user selects a field regardless of whether it is the field type (comment, boolean) or if it is a group that needs a subfield selected. See if it's possible to make the button appear only after the subfield is selected. Currently, the 'Continue' button lives under the $form['group_field_options_wrapper'] so it gets rendered and re-rendered the same as the subfields. The button should also be under a different name since it doesn't have to do with group field options.

4. Remove id from subfields radio (hopefully this doesn't break anything, i don't believe its being used)

Other things
5. Ajax error messages that happen from a modal currently display outside of the modal

6. Add tests for new 'Back' button functionality inside modal (Adding needs tests tag for that). The back button is added to the combined form in case a user wants to change the field type. Also decide maybe remove the Back button from the non-JS way since it may not be necessary.

7. Check that the "dummy" field name doesn't appear for any field types on the edit field storage form.

8. 'Continue' button on field selection page is currently left aligned, should probably fix to be right aligned for consistency with the edit form.

omkar.podey → made their first commit to this issue’s fork.

addressed #7.3

The modal flow now.

It would be great if we can update the IS with some explanation about what problem is this issue trying to solve - e.g. why that page without modals it not good and what benefits should an implementation of modals bring here. Thanks!

tim.plunkett → made their first commit to this issue’s fork.

Attempted a rebase now that combined forms is in. I did not fix the bugs introduced in the last commit

Since there is an active MR for this issue, the status should be NW or NR, not Active.

+1 for bumping the priority to Major.

There seems to be some weirdness with the reuse field flow,

1. So the moment we hit reuse the field is already included before we do anything on the config edit form.
2. I think this is odd and not that obvious so instead we should move the save to the last config edit page instead of saving it right away when we hit the reuse.

I hope to discuss this issue in the weekly Usability meeting in about 11 hours (14:00 UTC on 2023-10-20). We will post a link to the Zoom meeting in the #ux Slack channel 10 minutes before the meeting.

I rebased the merge request on the 11.x branch. The only conflict was in core/modules/field_ui/tests/src/Traits/FieldUiTestTrait.php. See the extended commit message for "Update fieldUIAddNewField test trait" for details.

The test core/modules/field_ui/tests/src/Functional/FieldUIDeleteTest.php fails before and after my rebase, so I do not think I broke anything that was working.

In testing, I could add a "Plain text" field, but when I tried adding a "Selection list" field, any submit button ("Add another item", "Save", "Change field type") bought me back to the previous step.

We discussed this issue at 📌 Drupal Usability Meeting 2023-10-20 Active . That issue will have a link to a recording of the meeting.

For the record, the attendees at today's usability meeting were @AaronMcHale, @anmolgoyal74, @benjifisher, @lauriii, @rkoller, @shaal, @simohell, and @worldlinemine.

We like the idea of using modals for adding a new field. It is a big improvement over the process introduced in ✨ Make field selection less overwhelming by introducing groups Fixed , and we hope this issue can be fixed in time for Drupal 10.2. (If not, then there will be a big change in 10.2 and another big change in 10.3.)

We realize that this issue is still in development, not yet ready for review. We noticed several problems in our testing: some are obvious, but some may not be. To keep track, I am adding them to the issue summary, under "Remaining tasks". As you fix these problems, please update the issue summary by crossing out the relevant lines.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

I added a few commits that should fix some of the tests. The last version had 98 fails, so let's see how many we get now.

I also left some comments on the MR. I wanted to avoid making too many changes before the tests are passing.

The failing tests point to a serious problem. The changes in the MR break editing an existing field. For example,

1. Go to Administration > Structure > Content types > Article.
2. In the row for Tags, click the Edit button.
3. Submit the form (Save settings).

A shortcut for the first two steps is to go to /admin/structure/types/manage/article/fields/node.article.field_tags.

After the third step, nothing happens.

It was worse before my last few commits: when loading the form, the Label field was empty. I checked out a couple of the "previous" tags from the issue fork to see what would happen. So I am pretty sure that the problem is not something that I caused.

At this point, it might make more sense to start fresh: create a new MR and take parts of the existing one that look correct, trying hard not to break anything.

One addition to point eight added to the list of remaining steps in the issue summary. I was under the impression that it was a general issue across all browser that the bullet pointed descriptions aren't announced but I've revisited and retested again in Safari 17, latest Firefox and latest Microsoft Edge on MacOS Ventura (13.6) with VoiceOver. Turns out Firefox and Edge are announcing the unordered list with the attribute of aria-describedby correctly (see edge.mp4) but Safari is not (see safari.mp4). Maybe the issue is related to the following filed webkit bug? https://bugs.webkit.org/show_bug.cgi?id=262895 the reporter adrian roselli tested in Safari 17 as well but with MacOS 14. It sounds like the combination of Safari 17 and MacOS 14 might introduced a new interaction after the field name announcement: "Press control option command slash to bring up the more content menu.". That interaction is missing for Safari 17 and MacOS 13. For Safari 17 and MacOS 13 there is no indication of any content, there is basically no difference between the announcement for the options of a reference field (where the options have no description) and the options of for example a plain text field (where the options have the unordered list). The descriptions on the first step, where the type of field is selected, are announced in Safari 17 - but those aren't using aria-describedby.

Another detail in the context of Safari 17 (on MacOS 13). The focus isn't restored to the Create a new field button when the field creation modal is closed/quit. In Firefox and Edge it restores that focus properly.

And I post the following issue in here for reference and exposure ✨ With an open dialog modal make only elements within the modal available to screenreaders Active . I finally understood the problem while testing this issue a few days ago but I've created a new issue because it applies to Core in general - basically all open dialog modals. When a modal is open the elements in the background aren't hidden from screenreaders and not removed from the AOM. In addition to adding the aria-modal="true" attribute as a short term fix it might also make sense as a followup to that linked issue to change the element of the modal title from a span to an h1.

I have started a new MR.

So far, all it does is extend FieldConfigEditForm to create the new FieldConfigAddForm and use the new class when adding a new field.

Given the changes we will need for this issue, I think it makes sense to have separate classes for adding a new field (use modals) and editing an existing field (no changes). We can make as many changes as we like to the new class without affecting the process of editing an existing field. As a bonus, we can add parameters to the constructor without worrying about backwards compatibility, since this is a new class.

It might make more sense to derive both classes from a common parent class. If we decide to do that, it should be easy to update this MR to make it so.

I will not do any more work on this issue until after my day job. If you like this approach and want to help, then you can start copying code from the old MR to the new one. Please make atomic commits and push often, in order to trigger automatic tests. Let's try not to get back into the situation where we have a lot of test failures. (An "atomic" commit does not have to be small, but it should do just one thing, clearly expressed in its commit message.)

I'm done with MR 4773 for today. I tried to make progress towards having passing tests. There's still couple test failures due to issues related to dialog positioning in the tests, as well as there are some error states that are not indicated correctly on the form. I also removed some of the unrelated code changes. I wonder if it would make sense to go back to working on this MR given that it's so much closer to passing? 😊

@lauriii:

I wonder if it would make sense to go back to working on this MR given that it's so much closer to passing?

At the moment, I think it makes sense to work in parallel on the two MRs. You have made a lot of progress on getting the tests to pass. (I am impressed.) But I still think that it makes sense to have separate form classes for new fields and existing fields. So I spent some time looking at some recently added code and moving it from the existing (parent) class to the new (child) class.

For example: two of the three parameters that were added to the constructor recently are only needed for new fields. If we add them to the child class, then we do not have to add the deprecation notices for BC. But the main advantage is that the net change from 10.1 to 10.2 in that class will be smaller and that the code will have fewer if statements.

At some point, we should combine the two bits of work. I do not want to work on MR 4773 until it passes tests. I am afraid that anything I do might create additional problems. It is up to you when and if you think it is a good idea to work on MR 5094.

Proposed solution for task #4
1. make radio buttons for options visually distinct from the buttons in type selection phase. Then navigation difference is understandable.
2. use USWDS as a reference for usable radio buttons: only single column of radio buttons, aligned in a single vertical line, add some margin before additional descriptions

Visibility of options on the screen is not affected comparing single column and 2 column versions. Cognitive accessibility/usability is reduced by 2 column layout.

2 column layout can result in lines of text so short they are hard to read. Usually literature recommends to have 45 to 75 characters by line. Single column layout may result in a 85 character line but not usually.

Comparison of the single and 2 column versions:

To be done elsewhere:

Additional text should be reviewed and unneccessery text removed.

Some modal scaling issues appear, but this is reported against core JS and will be fixed via getting rid of some jquery ui stuff, so it is out of scope.

The changes in !5094 seem reasonable, but are unrelated to this issue. Nothing in that changeset has anything to do with modals. I'd ask that you create a new issue for your refactoring so we can focus on one MR to accomplish implementing modals.

Agreed with @simohell with the suggestion from #25: the vertical solution is easier to scan if there are less than 6-7 items or a lot of text.

Created ✨ Use modals in field re-use flow Active for changing the re-use field flow.

I rebased on the current 11.x branch and added two new commits. Those commits are code cleanup, and I have some more in mind. I will try to do some of it tomorrow.

I did some more cleanup, adding several new commits and a couple of questions. (I have more questions, but it is late in the day for me.)

On the MR, I suggested moving OpenModalDialogWithUrl to the Drupal\Core\Ajax namespace. On Slack, @lauriii agreed with that suggestion, so I implemented it here. I was not sure what to do with the related JS file. For now, I added it to core/misc/dialog/ and created a separate library for it. Maybe it should just be added to dialog.ajax.js.

We had another look at this issue in 📌 Drupal Usability Meeting 2023-10-27 RTBC . That issue has a link to a recording of the meeting.

We agreed that this issue resolved the biggest usability problem from ✨ Make field selection less overwhelming by introducing groups Fixed . Therefore it is all right to postpone some of the other improvements we suggested (see Comment #18) to follow-up issues. I am adding the issue tag for follow-up issues to remind us of that.

For the record, the attendees at the usability meeting were @AaronMcHale, @anmolgoyal74, @benjifisher, @rkoller, @shaal, @simohell, and @worldlinemine.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

I did some manual testing and updated the "Remaining tasks" in the issue summary.

When testing keyboard navigation, I noticed that no field has focus when the modal window opens, or when the next step opens in the modal. I am adding a task to the list. Like the others, it can be handled in a follow-up issue if it is not fixed here. Keyboard navigation is already improved, since only the targets in the modal window are active. For example, before this issue I have to tab through the admin menu before I get to the form.

When testing "Selection list > List (text)", I noticed another existing problem with keyboard navigation. After entering an item, the first tab takes me to the Edit link for the machine name, but then focus immediately goes back to the Name field. The second tab lets me edit the machine name. Do we already have an issue for this?

Opened couple follow-ups for items in the issue summary:

1. 📌 [PP-1] Indicate where user is in the process of creating fields Active
2. 📌 [PP-1] Replace the "Change field type" button with one that goes to the previous step Postponed
3. 📌 [PP-1] Move label and machine name fields to the field config edit form Active

#31: I opened a separate issue for the list (string) focus handling bug: 🐛 Improve machine name AJAX focus handling integration Needs review .

Opened another issue for the dialog focus handling: 🐛 Dialog focus is returned to dialog instead of first tabbable element on dialog AJAX update Fixed .

@benjifisher is "Use links or radio buttons consistently so that keyboard navigation is consistent in the various modal screens." in the issue summary referring to #25 or is this referring to something else?

We need a change record for Drupal\Core\Ajax\OpenModalDialogWithUrl, adding that to the remaining tasks.

@laurii:

#4 in the list of Remaining tasks is an alternative to Comment #25. Either make the two stages visually similar with consistent keyboard navigation or else make them visually distinct with different navigation. And #25 makes specific recommendations if we continue to use radio buttons.

Thanks for the clarification @benjifisher! I'm removing that from the issue summary since #25 received a +1 from @ckrina in #27 and has been addressed already.

Observed the field UI forms but it's essentially different in the way that it only uses the submitform and not the ajaxsubmit or the successfulajaxsubmit. I did this to understand how under keyboard navigation ctrl+enter submits the form. There does seem to be a bug in the ajax submission.

The keyboard navigation works as expected for the most part, one thing to still tryout is defining the tabindex => -1 so that the modal boundary is not the first thing in focus.

@omkar.podey I believe #40 would be addressed by 🐛 Dialog focus is returned to dialog instead of first tabbable element on dialog AJAX update Fixed .

Created 🐛 Ajax submission by (ctrl+enter) causes jquery errors Active for the ajax submission errors when done through the keyboard (ctrl + enter) specifically.

Change record for the new OpenModalDialogWithUrl, [#3398575]

Removing resolved "needs" tags

I added some comments to the MR. That, and the static analysis failures, are enough to set the status back to NW.

I also noticed some odd behavior.

1. Install Drupal from the MR with the Standard profile.
2. Enable the Media and Media Library modules.
3. Go to Administration > Structure > Content types > Basic page > Manage fields (/admin/structure/types/manage/page/fields).
4. Click "Create a new field".

In Step 4, the modal window looks wrong:

I want to step back from looking at lines of code and ask two questions:

1. Should the "Create a new field" process work without JavaScript?
2. Can we reduce the complexity of the active MR?

To JavaScript or not to JavaScript, that is the question

Drupal core used to have a policy of progressive enhancement: the admin UI can be used without JavaScript, but it works better if JavaScript is available. I am not sure whether that policy is still in effect. If not, is it still a recommendation?

This question is closely related to the current test failures and my latest comment on the MR. The "Create a new field" link has href="/admin/structure/types/manage/page/fields/add-field". Without JavaScript, following that link goes to that path, and after recent work (within the last week) that path now returns an AJAX response. This is why the Functional (non-JS) tests are failing: they try to load that path expecting an HTML form, but they get an AJAX response.

One option is to decide that this process requires JavaScript. Then I think we need to convert the Functional tests to FunctionalJS tests and get them to pass.

The other option is to preserve the old behavior when JavaScript is not enabled. Then I think we should restore /admin/structure/types/manage/page/fields/add-field so that it returns an HTML response and add a new path for the AJAX response. Ideally, the existing Functional tests should work without any changes.

Less is more: move quickly by taking small steps

The current MR is very complex, which makes it hard to review:

• 111 commits
• 32 files
• +954/-528 lines

As @tim.plunkett pointed out in Comment #26, this issue is about modals. Can we strip out unrelated changes, such as changing the Submit button text from "Save settings" to "Save"? We can do that in a follow-up issue, and I think it will be easier to review and approve two issues than one issue that is more complicated than it has to be.

I am adding a "before" section under "User interface changes" in the issue summary.

I am listing paths, routes, and controllers (or forms) along with the screenshots. That will help me as I continue to review and work on the MR, and I hope it will help other reviewers, too. It also points out that some of the class names (controllers and form classes) no longer match what they generate: at least FieldStorageAddForm would benefit from a new name. In the spirit of my previous comment, I do not want to do anything about that as part of this issue.

By chance I was reading comments by @benjifisher in this issue and saw #46 about the size and complexity of this change. I thought this was an opportunity to share that Drupal does have Issue scope guidelines for Drupal core issues → . That includes information on patch size → and links to an article about code review good practice.

Except the BC breaks , I think this issue could go through another round of review, now that everything is green again.

Based on #51, I am setting the status to NR.

I have a few questions about the new JS file that I have been meaning to ask. I will do that on the MR. I am not much of a JS developer, so some of them might be off base.

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

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

I rebased the active MR and resolved merge conflicts.

This was a difficult rebase. In order to reduce merge conflicts, I first split apart and then recombined a few commits where conflicting changes were later reverted.

As I said in Comment #46, this is a complicated MR. Then it was 111 commits, and it is now up to 123. That makes it harder to resolve merge conflicts.

I still think it would help to remove string changes from the current MR. It would also help to rewrite the git history of the MR, reducing the number of changes that are redone or undone by later commits.

I rebased again, resolving the merge conflict from 📌 Convert enable/disable to install/uninstall in hook_help() Fixed .

Okay it is confirmed that the commit on 📌 Compress ajax_page_state Fixed is causing the test failures as i just removed those changes and we have passing tests expect the Ajax test which was expected to fail.

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

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

From #3346682-7: Improve re-use an existing field user experience → :

One recommendation I have at this point is that after re-using a field, we redirect the user to the field config form instead of opening it in a modal dialog. We want to move the field config form into a modal dialog but we should do that consistently for all instances where that form is being accessed as part of #2880003: [PP-1] Use modals on the Manage Fields page.

If that applies to the "Re-use an existing field" link, then it should also apply to "Create a new field". If, for this issue, that link opens a modal form, but submitting that form loads the field config form, it will drastically simplify the changes for this issue and it will make the two options behave the same way.

I am adding 🐛 Improve re-use an existing field user experience Fixed and ✨ Use modals on the Manage Fields page Needs work as related issues.

I think i finally understood why the description are not announced in voiceover and safari (see #20 ✨ Use modals in field creation and field edit flow Needs work ). Over in the a11y slack curtis wilcox created a reduced test case: https://cdpn.io/pen/debug/JjxeQdg . the odd thing there everything was working as expected and the descriptions were announced in safari and voiceover.
That made me think. I've compared the markup with the one used in the modal in this MR. I've noticed that the unordered lists containing the descriptions are wrapped twice in divs and the outer wrapping div contains the id attribute. And it looks like this is the problem why voiceover is unable to announce any of the descriptions in safari (while in edge in voiceover it works - see both videos in #20 ✨ Use modals in field creation and field edit flow Needs work ). A further reduced down test case to the one curtis wilcox created illustrating that the descriptions are announced without the wrapping of any div: https://codepen.io/ermarus/pen/yLZGOyg

@rkoller I just tried to use voice over on mac and I can't even get to the 'Create a new field' button while chrome works completely fine, I wonder if the problems with descriptions has also something to do with https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Annotati...

So, do you firmly believe that this I've noticed that the unordered lists containing the descriptions are wrapped twice in divs and the outer wrapping div contains the id attribute. is the only thing that is stopping the voiceover from working as expected?

@benjifisher I do agree with #61: marking unread comments in tracker → although we are going to address the config form to open in a modal in case of re-use ina separate issue ✨ Use modals in field re-use flow Active which will make them consistent.

re @omkar.podey

So, do you firmly believe that this I've noticed that the unordered lists containing the descriptions are wrapped twice in divs and the outer wrapping div contains the id attribute. is the only thing that is stopping the voiceover from working as expected?

well the thing is in the reduced test case the list items within the unordered list are not announced or noticed

<div id="one">
    <ul></ul>
</div>

while if the ul is directly linked like in my reduced test case

<ul id="one"></ul>

the list items are simply announced. therefore i assumed that the wrapping and adding the id to the wrapping div was the problem. and yes aria-describedby seems not fully supported yet in voiceover as the link you provided to mdn indicates as well as this one https://a11ysupport.io/tech/aria/aria-describedby_attribute. but those reduced cases just showed that the announcement of the descriptions work in safari as well. but it just looks like safari has issues with wrapping elements perhaps? or maybe it would be already enough instead of having the id for the aria-describedby on the outer wrapping div having it on the unordered list element instead if that would be valid and possible? (just as an idea - i am not a developer)

In addition @cwilcox808 made a followup comment over in the a11y slack. He echos the same, that voiceover has an issue when an ancestor element is referenced.

I made another version with a mix of working and non-working descriptions. Text from lists is accumulated into a description by VoiceOver when the list is directly referenced but not when an ancestor element the list is referenced. VoiceOver doesn't have a problem accumulating text from the other kinds of descendant elements I tried but in the wide world of elements, there may be more than lists that give it trouble.

One detail I wonder. Are those two divs wrapping the ul element necessary at all? For testing purposes i've moved the ul element on the same level as the input and label element in devtools and then moved the idand class attributes of the two divs to the ul element and deleted the two div elements afterwards. Except the positioning of the lines of the list elements everything looks the same, the number of dom elements gets reduced, and voiceover announces the descriptions properly that way.

@rkoller Fixed the voiceover issue, with a small issue that it announces the bullet.

Thank you @omkar.podey! I can confirm that the description is announced now in Safari with VoiceOver in Sonoma. In regards of the small issue you've mentioned about the announced bullets. I've noticed that the descriptions are just wrapped in a single div now and the lines of the different descriptions are just separated by br tags. Why not use the ul element instead of a div and wrap each description line in a list item element. that way the bullets wouldn't be announced and it would be more semantic?

@rkoller that was my initial approach but the voiceover doesn't work on it so I resorted to all text.

hm that is odd because in all the reduced test cases just the ul element was used in most of the examples and voiceover was able to announce the list items. not sure what prevents voiceover to announce the ul in your tests. but if you take a look at for example https://codepen.io/ermarus/pen/yLZGOyg again there the list items wrapped by the ul are announced just fine. could it be some caching issue on your end or have you tried to disable and reenable voiceover. sometimes it seems voiceover mixes up things and is still stuck to an older state when i've tested modifying the dom in devtools or change something in a pen on codepen.

The problem is removing the div 'string--description' which comes from Form/FormBuilder.php:1002 i.e

    if (!empty($element['#description'])) {
      $element['#attributes']['aria-describedby'] = $element['#id'] . '--description';
    }

Need to think of a way to override it, as can't change it directly and also describedby might be a problem.

@omkar.podey Can you confirm if that's a problem with 11.x too? If it is, we should open a follow-up issue for that.

Confirmed on safari with the base 11.x branch that the description is not announced.

@omkar.podey:

Thanks for updating the issue summary. I consider it a start, since there are a lot of changes in the MR that are not explained in the "Proposed resolution" section.

For now, I want to consider the first item there:

Split up FieldStorageAddForm to reduce text in a single modal into FieldStorageAddController and FieldStorageAddForm.

That controller returns a form. Why not make it a form class instead? Since it is not a form in the current version of the MR, the old form-alter functions no longer work, and that leads to a new hook that they can implement. If you use a form class to generate the form, then you can continue to use form-alter functions.

Even better: turn FieldStorageAddForm into a two-part (multi-step) form. That has a lot of advantages:

1. You do not need the extra form/controller class and the associated route.
2. You do not need to rename the existing form-alter functions.
3. You can save data in form state instead of using temp store and yet another associated route.
4. The AJAX form submit just has to update form state and rebuild the form, so we get one step of "How do I keep all this in the modal?" for free.

Please do not remove items from the list of "Remaining tasks" in the issue summary.

I am reviewing Comment #20, which starts out

One addition to point eight added to the list of remaining steps in the issue summary.

It is hard to understand the context if there are fewer than 8 items in the list. The point of using an ordered (numbered) list instead of an unordered (bulleted) list is that we can refer to items easily. That gets broken if you remove items from the list, or insert new items before existing ones.

When an item is completed. instead of deleting it.

I am restoring several items from an earlier revision of the issue summary. Probably some of them were removed because they were completed, or separate issues were created for them. In that case, go ahead and strike them out.

That controller returns a form. Why not make it a form class instead? Since it is not a form in the current version of the MR, the old form-alter functions no longer work, and that leads to a new hook that they can implement. If you use a form class to generate the form, then you can continue to use form-alter functions.

It is not an actual form, it's just a controller with links. We could convert the controller to a form but it doesn't actually behave like a form so from that perspective it makes sense to use a controller instead. The MR is currently adding an explicit API for modules to remove field types for an entity type. I personally think it's worth considering having an API for this given that right now if a module wants to remove field type from the UI for a specific entity type, they need to rely on the brittle form alter. If we provide an explicit API for this use case, we can change the UI without breaking modules.

Even better: turn FieldStorageAddForm into a two-part (multi-step) form. That has a lot of advantages

I like this idea a lot because it could simplify the implementation quite a bit. I started looking into this, and realized that Form API/AJAX API doesn't support <button> element, which makes implementing this pretty difficult. I think we should add support for <button> element, and then do the refactoring. I personally would prefer if this wouldn't block this issue but it might be worth having a framework manager weigh in on this.

Tagging this issue to get feedback from the framework managers for adding the hook_field_ui_field_type_ui_definitions_alter and doing refactoring proposed in #78 in a follow-up.

📌 Use form element type instead of Needs work is the issue for adding AJAX buttons.

If this issue was adding the temp store dependency, then I'd probably push for trying to do not do that, but since it's modifying the existing usage we already have in HEAD, so I think it's OK to have a follow-up to switch to an actual form postponed on the button element issue.

I do think the controller should stop using the word 'form' though so it's clear it doesn't produce one.

Wim's points in the MR look like they should be addressed here though - if we can avoid duplicating AJAX commands that would be good.

Even better: turn FieldStorageAddForm into a two-part (multi-step) form. That has a lot of advantages

I like this idea a lot because it could simplify the implementation quite a bit. I started looking into this, and realized that Form API/AJAX API doesn't support element, which makes implementing this pretty difficult.

I do not see why we needs the <button> element for this. I am working on implementing this idea, so I hope to have something testable soon.

benjifisher → closed merge request !5094

📌 Create a new OpenModalDialogWithUrl command Active is in.

kunal.sachdev → made their first commit to this issue’s fork.