Allow user to add display modes from respective field UI's.

Attaching the screenshots with the updated manage form display and manage display.Also removed the Display Modes from the main structure page.

How would users be able to delete or manage view modes now though? Would they have to go through a content type now to delete a view mode?

Also does it negate all the work done on https://www.drupal.org/project/drupal/issues/3359240 ✨ Enable bundle selection when a new view mode is created Fixed ?

It will help for reviews if we update the issue summary using the Issue summary template → . Can we also get an updated screenshot using Claro? On Slack, @lauriii also suggested updating the title.

@benjifisher beat me to it!

@Utkarsh_33:

Thanks for working on the issue summary.

The goal of rewriting the issue summary is to add enough information so that the issue can be reviewed without reading all the comments and without reviewing/testing the merge request.

It helps to have a more explicit list of changes in the Proposed Resolution section. I think this issue is removing links from the Administration menu in the Standard profile. I am not sure whether they are being added somewhere else. Can you clarify that?

We like to have "before and after" screenshots in the "User interface changes" section. I see that you attached new screenshots to the issue, but you did not use them in the issue summary. The previous version of the issue summary did show one screenshot.

I tested it locally and it's working as expected. I think the Issue summary is updated and "before and after'" screenshots are also added.

I think a change like this should get usability and accessibility reviews. I am setting the status to NR and adding the appropriate issue tags.

I have not had a chance yet to review the changes to the issue summary in detail, but thanks for adding the screenshots.

smustgrave → credited mgifford → .

Looked at this with @mgifford. Noticed that when tabbing to the "Add view mode link" and clicking open. Focus remains on the link and not into the modal.

We discussed this issue at 📌 Drupal Usability Meeting 2023-10-27 RTBC . 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, @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.

In general the group liked the changes introduced with this merge request. To summarize the few additional points that came up during the discussion:

1) The name field is missing the red star for required fields and if you are saving the modal dialog without entering anything into the name field you are forwarded onto the Add new Content view mode page outside of a modal dialog context. Ideally the modal dialogue shouldn't be left until a successful save or deliberate quitting of the modal.

2) If you click the Add new view mode link on the Article content type, no content type in the list of available content types for the new content view mode is selected.
There was a clear consensus that it would be convenient and handy to auto select the content type which the new view mode is created for. So for example if you press the Add new view mode for the Article content type the Article checkbox would be auto-selected on the Add new Content view mode modal.

3) On for example /admin/structure/types/manage/article/display the label for the field set is called Enable more display modes while the add button is called Add new view mode. It might be potentially confusing if the label is using the term display mode while the add button is using the term view mode. Display mode is the parent term for view mode but the user might not necessarily know that. To avoid confusion and to lower the cognitive load it might be a reasonable step to make the wording of the label consistent and use Enable more view modes. And strictly speaking the fieldset is not only about enabling but also about disabling view modes. Therefore the suggestion would be to change the label for the fieldset to something like Enable view modes.

4) Out of scope for this issue: One idea that came up during the meeting is to adopt a pattern for Core in general is to enable the user to select a range of checkboxes by shift clicking (A pattern Gmail employs). if you have checkbox 1 to 50 you first click number 3 and then shift click number 27, that way all checkboxes from 3 to 27 are getting checked.

I'll also remove the Needs usability review tag and set the issue back to needs work.

Thanks for catching that @rkoller. Really appreciate you looking over this in the UX channel and providing the feedback here.

I'd missed that name field was a required field. Are there any instances where this wouldn't be the case?

re #50 📌 Allow user to add display modes from respective field UI's. Needs work in regards of add view mode link click and the focus remaining on the add button instead of going into the dialog modal. would it would make sense to open up a follow up issue and collect and fix the occasions in a single issue because that problem seems to be a more general one happening in other places in the admin ui as well. for example i just noticed it for the place block button on the block layout page as well.

re #52 📌 Allow user to add display modes from respective field UI's. Needs work in the context of the add a view/form mode it looks mandatory. the name is needed for the machine name. without it can't be saved.

So maybe? But that modal issue doesn’t happen on the Display Modes page using the same modal. So actually not sure how it got introduced.

Another thing we noticed during 📌 Drupal Usability Meeting 2023-10-27 RTBC :

If I start on /admin/structure/types/manage/article/display and click on the link to add a new view mode, then the form opens in a modal window. If I try to save the form without entering anything, then the non-modal version of the form loads: /admin/structure/display-modes/view/add/node?destination=/admin/structure/types/manage/article/display:

When there is an error like this (a validation error, I think) can we make it so that the form is still displayed in the modal?

Also, as the screenshot shows, the error message refers to the machine name, but the machine name is still hidden. We have run into this on other recent issues adding modal forms. So maybe that is a systematic problem that deserves its own issue, or maybe that is something that has to be fixed separately on each of these issues. I guess that is why the Name field is not marked as required: the Name field is actually optional, but the machine name is required.

yash.rode → made their first commit to this issue’s fork.

Need to fix the tests and write test coverage for the newly added things (going to do that tomorrow).

To run drrpalci

Hi, in core/modules/field_ui/src/Form/EntityDisplayModeFormBase.php I need to get the value of $bundle which is hardcoded to 'article' right now. I want the value of bundle from where we are opening the EntityDisplayModeAddForm. Can someone help me with this?

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

Still seeing the accessibility issue where when the modal opens up the focus is lost.

Not seeing that when I add a form mode via admin/structure/display-modes and that modal, so maybe the same fix could be made generic or copied over?

I agree with #53 and there is an existing issue for that https://www.drupal.org/project/drupal/issues/3397785 🐛 Dialog focus is returned to dialog instead of first tabbable element on dialog AJAX update Fixed . So, may be this is ready for review.

But this accessibility, for this modal, was introduced in this issue I believe. The modal popup for adding view modes via the Display Modes section doesn't have it.

The focus is working now this can be reviewed.

Confirmed accessibility issue has been fixed here.

Posted feedback on the MR.

All the threads have been addressed. Except the local task issue, which I am not able to reproduce.

I was able to reproduce the issue @lauriii was seeing.

On a standard install
On the article view display, added a new view mode called test
Clicked save and got the message that the view mode was saved but it doesn't appear.
Refreshing the page made it appear.

Re #70, I tried to reproduce the issue but failed. Attached is the screen recording. Can you please let me know if something is missed?
https://www.drupal.org/files/issues/2023-11-30/field_ui_issue.mov →

Pushed commit that fixes #70.

@smustgrave would be a good fit to review this one, as he was able to reproduce it on local.

Some weirdness where it appears the background is refreshing before the modal closes, but may just be my machine.

Don't think it should be a blocker for the overall feature.

The issue before where newly added modes weren't appearing appears fixed.

It all seems to work as expected but I couldn't get the test to fail without the fix.

I think https://git.drupalcode.org/project/drupal/-/merge_requests/4839/diffs?co... is not must have for all the setup but a good to have as it was not showing the display modes in the local task bar for some set-ups. That's the reason for #75.

As everything is working as expected.

I made a few comments/suggestions on the MR. Back to NW for that.

It is not strictly enforced, but it is good practice for each developer to stick to one role: making changes or reviewing the issue. Based on that, it is unusual for @yash.rode to set the status to RTBC.

Also, I am not satisfied with the response to Comments #75, #76. If I read them correctly, they are saying that the new test for this issue does not fail as expected when run without the non-test changes here. If that is the case, then it is not a useful test.

Thanks for the review @benjifisher,

Also, I am not satisfied with the response to Comments #75, #76. If I read them correctly, they are saying that the new test for this issue does not fail as expected when run without the non-test changes here. If that is the case, then it is not a useful test.

this is not right, the test for this issue won't pass without the not-test code. But the test will pass if we skip https://git.drupalcode.org/project/drupal/-/merge_requests/4839/diffs?co.... Reason for that is explained in #77

Addressed feedback and responded to #79

@yash.rode:

Thanks for the more complete explanation in #80.

I did some more code review and added some suggestions on the MR, so back to NW.

I guess the reason for rebuilding the router is that there may be new tabs (local tasks) that should display when the modal form is submitted. That means we also need a page reload, right? If so, then that removes one of the advantages of using a modal in the first place.

It would help to discuss this problem in the "Proposed resolution" section of the issue summary. I wonder if there is a more targeted solution than rebuilding the router.

In #79, I wrote,

It is not strictly enforced, but it is good practice for each developer to stick to one role: making changes or reviewing the issue.

I meant to say that this practice applies one issue at a time. It is OK to take one role on one issue and a different role on another issue.

All the feedback has been addressed.

-While testing this locally one thing that i found was if we keep the "Name" field blank it does not show any error. The AJAX processing throbeer
appears but the error is not shown up(neither js error not the status error message).
-Another thing that i noticed is that if we click on the edit machine name , the edit machine name form element appears after the description but ideally it should be next to the Name form element.

Addressed #85's first point and

Another thing that I noticed is that if we click on the edit machine name, the edit machine name form element appears after the description, but ideally it should be next to the Name form element.

this is the existing form, so if we want to do this we should do that in a separate issue.

Retested this and still seeing expected before.

+1 RTBC from me but leaving in review for additional eyes.

Been about a week so going to go ahead and mark.

One of my comments on the MR was

I tested with a fresh install of Drupal using this MR and the Standard profile. I visited /admin/structure/display-modes/view and used the Edit link for the Teaser view mode. When the form loaded (in a modal dialogue) no options were selected. I saved the form without making any changes. Then I looked at /admin/structure/types/manage/article/display. As expected, the Teaser mode was disabled.

I still see that behavior. I am setting the issue status back to NW and assigning it to myself.

I would like to have another look at this issue with the usability team, so I am adding back the tag for a usability review.

While testing this issue, I realized that it is odd to have the "Add new view mode" link on the "Manage display" page, and to have the modal form trigger a page load. It is confusing to have a form with some elements that do not take effect until the form is submitted and other elements that do not need the page to be submitted.

For example, suppose I

1. Open the "Display settings" details element
2. Check off some view modes
3. Un-check others
4. Realize that I need a new view mode
5. Follow the "Add new view mode" link
6. Submit the modal form

Whatever I did in Steps 2 and 3 will be lost.

I think it makes more sense if submitting the modal form just creates the new view mode and returns to the form as it is, except that the new option is added. Then the user needs to select the new view mode and submit the form.

One advantage is that this might make the router rebuild unnecessary. In fact, I am not sure that is the right solution in the first place. It looks to me as though there is some sort of race condition. See comments #69, #70, #71: the missing local task problem seems hard to reproduce consistently, and the local task appears if you refresh the page. I see the same behavior. If reloading the page is enough, then rebuilding the router is not the right answer. Probably it just creates enough of a delay to resolve the race condition.

If we do not need to rebuild the router, then we do not need to inject the router service, and that will simplify the changes for this issue.

Addressed all the feedback. It needs a usability review as mentioned in #90 →

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-01-26 Active . That issue will have a link to a recording of the meeting.

Thanks for the updates. We tested with the latest version of the MR.

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

The usability team agreed with the point I made in Comment #90: it is a problem that submitting the modal form will lose any unsaved work on the main form (selecting or de-selecting existing view modes, reordering fields, other changes to fields). It is also a problem that there is no indication that the "Add new view mode" link opens a modal window. Unfortunately, there is no clear choice for how to indicate that. Specifically, we recommend the following changes:

1. Follow-up issue: Add a design for links or buttons that open modals.
2. Until (1) is decided, use some styling (button, action link, perhaps the same styling as the links on the "View modes" page) to indicate that "Add new view mode" is not an ordinary link.
3. Restore the "Manage view modes" link.
4. Follow-up issue: Add anchors to the "View modes" page and make the link in (3) go to the correct part of the page. Consider ways to reduce scrolling on that page: links to the anchors or putting the tables in collapsed <details> elements, for example.
5. When the modal form for this issue is submitted, return to the main page with whatever unsaved changes it may have. In particular, the "Display settings" section should be expanded, and the new view mode should be added.

Opinions were split, but some of us think that the modal form should not have the option to enable the new view mode for other bundles. One reason is that it adds cognitive load. There are multiple view modes and multiple bundles. It makes sense to look at one view mode and be able to select several bundles, or to look at one bundle and select several view modes. But in the context of a particular bundle, it is confusing to manage other bundles. Another reason is that we should try to keep the modal form simple. On a real site, there are often dozens of content types, and that makes the modal form too complicated.

So we leave it up to you whether to keep the list of bundles in the modal form, just as we leave it to you to decide what styling to use in (2). If you decide to make this change, then keep the checkbox for the current bundle, selected by default.

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

Implementation is out of scope for a usability review, but I will repeat what I said in Comment #90: implementing (5) should simplify the current changes since we will not need the route-builder service. You can also consider which approach is simpler when deciding whether to remove the other bundles from the modal form.

@benjifisher
What doing too many things in one form brings up a good point. Users are both choosing that they want a view mode active, and fields of the specific (tab) view mode/dislay they may have unsaved changes to field order, formatter setup..

the page should have separate submits. IF!! those forms had 'ajax on' that would be extra awesome...

in regards of "doing too many things in one form" in #93 📌 Allow user to add display modes from respective field UI's. Needs work . I had some more time to think about things since yesterday. I think the main problem here, with layout builder installed, are the differing scopes:

The Layout Builder field set:
the setting is on a per view mode basis. on each view mode for a content type you are able to either enable or disable layout builder individually

The Display settings field set:
the setting is "global" across all configured view modes for the content type in question. if you enable for example the search index view mode being on the RSS view mode and save, you get redirected to the default view mode tab and the search index tab is shown as well now.

That differing scope might be not THAT apparent and a potential source of confusion. i am not sure if the suggestion raised in #93 📌 Allow user to add display modes from respective field UI's. Needs work by moving the display settings field set on top of the list of available fields would bring in additional clarity or solve that aforementioned issue in regards of scope. the display settings field set would be still within each view mode tab. i am also not sure if having two save buttons is a good idea it is more of an indication that we are having two different forms here (the "doing too many things in one form" problem) and that there should be a separation of concerns

semantically and hierarchically the display settings field set would have to be placed further up between the primary tab bar and the secondary tab bar to clearly communicate that the display settings are for ALL view modes globally. But for one it would be odd to move those display settings in-between the two tab bars and having the display settings field set in between those two tab bars would also complicate the saving. it would require a save button as well and would things only more confusing. probably the better choice is to move the display settings out of the view mode tabs.

one approach might be to add a dedicated "settings" tab (or however it will be called) to the list of available secondary tabs on the Manage display page - as the first or the last option. the other approach might be to add an additional vertical tab on the Edit page of a content type . But all out of the scope of this issue.