Use modals in field creation and field edit flow

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!

Thanks @poker10! Tried to update the issue summary with some context.

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.
3. But without including the field we won't be able to get to the edit config form.

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 #3393732: Drupal Usability Meeting 2023-10-20. 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 #3356894: Make field selection less overwhelming by introducing groups, 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 #3395355: With an open dialog modal also elements in the background are added to the accessibility object model. 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.

Edit: I spoke too soon. Only the $tempStore property can be moved from the parent class to the child class.

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 #3397164: Use modals in field re-use flow 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 #3395398: Drupal Usability Meeting 2023-10-27. That issue has a link to a recording of the meeting.

We agreed that this issue resolved the biggest usability problem from #3356894: Make field selection less overwhelming by introducing groups. 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. #3397715: Indicate where user is in the process of creating fields
2. #3397709: [PP-1] Replace the "Change field type" button with one that goes to the previous step
3. #3397711: Move label and machine name fields to the field config edit form

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

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

@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 #3397785: Dialog focus is returned to dialog instead of first tabbable element on dialog AJAX update.

Created #3398568: Ajax submission by (ctrl+enter) causes jquery errors 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 #3379495: Convert enable/disable to install/uninstall in hook_help().

Okay it is confirmed that the commit on #3348789: Compress ajax_page_state 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.
I had to comment out the declaration in core services yml to get them passing, which means it has something to do with AjaxPageState class and it's 2 functions probably.

    http_middleware.ajax_page_state:
      class: Drupal\Core\StackMiddleware\AjaxPageState
      tags:
        - { name: http_middleware, priority: 500 }

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 #3346682: Improve re-use an existing field user experience and #2880003: Use modals on the Manage Fields page as related issues.

I think i finally understood why the description are not announced in voiceover and safari (see #20). 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). 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 safari 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 #3397164: Use modals in field re-use flow 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 with br tags.

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.

@rkoller 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. strike out the text 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 given that it doesn't currently use any of the form API functionality, 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.

#1671190: Use <button /> form element type instead of <input type="submit" /> 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.

I like having the new alter hook - the comment implementation shows how it's cleaner than using form alter.

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 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.

#3408738: Create a new OpenModalDialogWithUrl command is in.

I need help with the test failures -

1. \Drupal\Tests\field_ui\Functional\ManageFieldsTest::testAddField() - In this test we are testing that field descriptions appear, both 1 line and multiple lines. But the failure is that the single line description is not showing on the screen. And when I change the code
   description: new TranslatableMarkup("This one-line field description is important for testing"), to

   description: [
     new TranslatableMarkup("This one-line field description is important for testing"),
     ],

   in the plugin core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldType/TestItemWithSingleDescription.php then it works.

2. All the failing FunctionJavascript tests - All the tests are failing with the error 'SyntaxError: Identifier 'DrupalDialogEvent' has already been declared'. I tried reverting all the changes done in core/misc/dialog/dialog.js in https://git.drupalcode.org/project/drupal/-/commit/9b2d61c6e159ffdb62d5b... then it works fine without errors.

Important to note: the merge Lauri did failed tests, my commits after are similarly failing. We might want to revert my commit of the dialog.js changes until we get the tests green again

Tests fixed and CR added. This issue can be reviewed again. Thanks

1. I like that it still works without JS
2. fix on the JS needed
3. Any idea of the perf hit on the tests? it feel like it'd be significant
4. this CR is not up to date anymore (version): https://www.drupal.org/node/3400352
5. can we reduce the description text from https://www.drupal.org/node/3503549, it's too long and makes it hard to see what changed

Adding #3485202: Change the element wrapping the title in dialogs from span to heading element as related issue. The issue is about changing the element wrapping the dialog modals title from a span to an h1, which is relevant and important for this issue as well.

After

Drupal\Tests\field_ui\FunctionalJavascript\DisplayModeBundle   2 passes   60s                                      
Drupal\Tests\field_ui\FunctionalJavascript\ManageDisplayTest   3 passes  136s                                      
Drupal\Tests\field_ui\FunctionalJavascript\ManageFieldsTest    7 passes  211s                                      
Drupal\Tests\field_ui\FunctionalJavascript\EntityDisplayTest   3 passes   72s                                      
Drupal\Tests\field_ui\FunctionalJavascript\DefaultValueWidge   1 passes   24s                                      

Before

Drupal\Tests\field_ui\FunctionalJavascript\DisplayModeBundle   2 passes   32s        
Drupal\Tests\field_ui\FunctionalJavascript\ManageDisplayTest   3 passes  136s                                      
Drupal\Tests\field_ui\FunctionalJavascript\ManageFieldsTest    7 passes  211s                                      
Drupal\Tests\field_ui\FunctionalJavascript\EntityDisplayTest   3 passes   72s                                      
Drupal\Tests\field_ui\FunctionalJavascript\DefaultValueWidge   1 passes   29s                                      

haven't checked all tests but it seems fine, except for DisplayModeBundle

One detail to note, i've tested the latest state of the MR and the point i've raised in #20 still applies. when the dialog modal is closed by pressing esc or the close button the focus isnt returning to the "create a new field" button, but instead to the top of the DOM, so the keyboard user has to navigate back to the previous position one left off. the expected and desired behavior is that the focus moves back to the component which revealed the content/dialog.

Update: turns out, it is an issue with safari 18.3... just tested with latest edge and firefox and there the focus moves back to the component which revealed the content/dialog. but in safari it does not instead

I cleaned up the new CR and deleted the outdated CR.

i've retested with the latest changes, and the problem in #97 with the focus not moving back to the component that revealed the dialog in safari still persists.

Sorry, I forgot this included the fix from #3502911: Compressed ajax_page_state['libraries'] can exist in both $request->request and $request->query simultaneously, so this is technically postponed. There's also a failing test now, and it might have been from the last commit.

#99 Can you file a follow-up to fix that for Safari?

Thanks @nod_! Rebased

The remaining tasks in the issue summary are all in other issues, does that mean they are followups or pre-requisites? This would be important to make it clear :)

Also, there are no outstanding review comments on the MR, anything in particular to review or just looking for general feedback?

Finally, the MR tests were failing, but a spot-check lead me to believe they are unrelated. The file download PHPUnit test and the a CK5 image test was failing. Sent for a retest.

I think all remaining tasks are either done here or in separate issues(issue links are mentioned).
And yes the MR needs a general review.

The fact that they are in separate issues could still mean they are to be done before this or after :) Can you clarify that answer? Thanks!

On visual review, the feature looks and works great.

However there is a range of screen width where the dialog goes out of the viewport, even though the page content (eg. toolbar, messages, etc) are not. This is odd on the type selection, but one can still move forward. However not purely inconvenience after that, because the form buttons to go forward are not visible. There is no scrollbar to get to them either, so one can only tab to them and attempt to press blindly :)

Minor note (don't spend time on fixing this to get it in IMHO) but I think the "Choose a type of field" label looks very odd. It is tiny compared to the massive selection options under it (I do realize this is the standard label size). The whole dialog does not look like a form (it does not even have a button), so the red asterisk feels out of place (those belong on forms :D). Also this label does not add anything to the dialog, we already know we are adding a field, I don't think the label clarifies anything about the selection below. Either way, I don't think it is worth debating this part at least not to holding up anything :)

The responsive behaviour would be good to fix though IMHO as it makes it impossible to use the feature on certain screen sizes.

Regarding #107, an issue for the problem 'dialog goes out of the viewport' already exists. See #3395590: Modal dialogs clip content with certain viewport width.

@gábor-hojtsy UX weeklies on Fridays have been going through all the option descriptions during the past few months. It's possible to check the label in that meeting. Another related discussion is keyboard navigation for sighted users between elements that was discussed in accessibility (radios work differently from links and buttons). The label is required I think for screen reader users, but may be debated for sighted users. At UX meetings we keep popping into a lot of things out of scope for the text change issue, so there will be quite a few new issues to this as well.

As discussed, #107 is half pre-existing, half-out-of-scope.
#109 is in response to that, but unchanged here.

I updated the CR per discussion with @lauriii.

I rebased the branch after fully re-reviewing it again.

Thanks @kunal.sachdev!

updating credits

I understand my concerns were preexisting and/or out of scope, agree with the RTBC.

What kind of framework manager review is needed here? Is that tag still current?

Found what I think is one serious(ish) issue and a couple of more minor ones.

I have addressed all the feedback/suggestions.

Thanks for addressing the feedback @kunal.sachdev

OK I don't think I've reviewed this enough to commit it, but I think I have enough to remove the FM review tag, so doing that.

Committed 7df6570 and pushed to 11.x. Thanks!

I'm not sure of the process here, whether to reopen this or a different issue.

But the CRs on this issue I think is missing some detail on some breaking changes for tests for field ui.

I found this issue when looking at https://github.com/Chi-teck/drupal-code-generator/issues/209 with @chx in slack.

It also seems like some routes and wrappers changed, I think it might be helpful to document those on a CR that makes it clear what changed here.