Revisit the redirect to 'add block' form in the 'add block content' form

Totally agree with this, we alter away this redirect.

Also, when using other contrib modules to provide access to creating block content entities without being able to place those blocks, the redirect can lead to a 403.

I'm curious about the use case for going straight from adding a new Block content entity to placing it in the block layout. In "normal" workflows, this would only happen from a sitebuilder/developer on their local environment as block config should be exported and committed to the codebase. How often are site builders using this workflow?

This may become even more important with #3318110: [meta] Reorganize Block items in the administration menu, specifically #2862564: Move Custom block library to Content. At least right now in 10.0 everything is under the Block Layout area, but with Block Content moving to Content, there will be a significant separation in the information architecture between Content Blocks and the Block Layout, so we definitely need to do something about this.

Could we get some screenshots of the current state to better illustrate the problem?

At the usability group we're actively tracking the work around #3318110: [meta] Reorganize Block items in the administration menu so I'd like to bring this issue up at a future meeting as it's very relevant.

Uploaded some screenshots of what I see.

Also opened an MR with a proposed solution. No tests yet.

@smustgrave When you post screenshots, it is helpful to embed them in the issue summary.

Marking NW and tagging for tests.

Adding the screenshots to the IS.

Hm, except the screenshots also are not of the described UI changes. Were the correct files uploaded, or is the IS wrong?

Usability review

We reviewed this issue at #3336927: Drupal Usability Meeting 2023-02-03, that issue will have a link to the recording.

Those who attended the meeting were: @AaronMcHale, @iszabo, @rkoller, @shaal, and @simohell.

When reviewing this issue we observed how the Taxonomy Term add form currently behaves, how it presents a similar integration pattern. The group was generally in favour of the pattern of more than one "Save" button. The group also observed that when the Term add form has the destination parameter in the URL, the additional "Save" button does not display, only the primary "Save" button is shown.

Instead of "Save and place", the group recommended the text "Save and configure" for the additional action button. The recommendation is based on the fact that after clicking that button, the user is taken to a form titled "Configure block". It was felt that it could be confusing for users if the described action was not consistent with the title of the form they are taken to. The group felt that because the Configure block form uses the term "Place block" for the primary action button helped to convey would what happen once they clicked that button.

It should be noted that the recommendation is based on keeping consistency in the current UI. In light of the recent moves to restructure the blocks area of the admin UI, it was also noted that we may want to review the title of the "Configure block" form, perhaps titling it something like "Place block" would be more appropriate and help to convey that this form is relating to blocks placed in regions and not general configuration of a block. The lack of clarify here was seen as a cause of confusion, for example the visability options on the "Configure block" form could be misinterpreted to apply to the Custom Block entity itself, rather than the single configuration instance of the block currently being placed.

For the avoidance of doubt, if the title of the "Configure block" form is changed, then in the interest of consistency that should also mean that any action buttons leading to that form should also be updated, such as the one proposed here.

If the issue I proposed in the MR sounds good I can get working on tests.

+1 to this change. Reminds me a bit of the debate around where you should go when you save a new media entity. 😅 I think the 2 submit button solution here makes great sense.

There's an MR, so NW is more accurate status.

Thanks!
-Derek

Updated the redirect logic and when the button should appear.

If that looks good we can work on tests.

So changes I made that maybe need discussion.

Changed the new button from "Save and configure" to "Save and place in Block Layout" as Save and configure didn't make much sense to me.

I added the check for "administer blocks" because when #1975064: Add more granular block content permissions lands block layout, block types, and block library will be independent permissions.

Not sure if this is postponed on that.

I've applied the latest patch and made a few observations.

1. when i am on /admin/content/block-content and click the add custom block button, the choose a block type page has the following path /block/add?destination=/admin/content/block-content. the following add custom block page has only the save button. it's path is /block/add/basic?destination=/admin/content/block-content.
if i directly enter the path from the issue summary: /block/add and choose a block type then i see on the add custom block page the save and the save and place in block layout button. but shouldnt the save and place in block layout button also be available if you add a custom block by clicking the add custom block button?

2. on the configure block page i've noticed one other detail. maybe it would be useful to provide the context aka in which theme the placement takes place? either in the h1 or in the region label of the select list at the bottom of the page?

3. in regards of the renaming of the save and place in block layout button label. there are two details to note. before the button was called save and configure and the h1 on the linked page configure block. now the button label is save and place in block layout but the h1 on the linked page is still configure block. there is a disconnect now. shouldn't the h1 sort of be aligned with the new button label?
and in general save and place in block layout has too many words even though it is clearer. the rule of thumb from the strategic writing for ux book for button labels is: buttons work best when they are recognizable, specific, and only one or two words long. An alternative for the button label might be "Save and place"? or should the naming adjustments be done in #3318549: Rename Custom Block terminology in the admin UI

@rkroller

1. Maybe? I would assume if there destination is there the user would expect to go back to where they came from. I tend to find that majority of blocks we create aren't going to placed in the block layout so think this behavior is correct, if the destination is there it shouldn't go to block layout.

2. I noticed that too. I think it should maybe be a follow up that if you go
block/add -> Save and place -> configure
Somewhere before configure you select the theme. Currently defaults to the front end theme.

3. I like "Save and place" a lot better. Configure makes me think I'm editing the block_content and I don't think I'm configuring to place in the block layout.

1. but from my understanding, the only way to create a custom block via the interface is on /admin/content/block-content by clicking the add custom block button. the only other option is in layout builder by creating an inline custom block. /block/add without a destination you are unable to reach without typing that into the address bar arent you? and isnt that the idea of having those two buttons. that the user is able to chose, do i just create the block or do i add config by placing it into a theme? i dont see a problem showing both buttons when the add custom block button is clicked?

2. ah adding an option deciding on the actual theme might be a good idea. and yes definitely something for a followup. how about instead of a save and place in block layout button use a drop button. the default option could be the default theme and the other options the other installed themes (and that feature would be invisible if a site has only one frontend theme installed) . that way it would be easily and clearly decided and communicated in which context a block is placed? but i would still communicate that choice on the config page still.

You can create a custom block via block layout also. If you're going that route then you have "administer blocks" permission and were in the process of placing one.

Regarding the "save and place" or "save and configure" text, to be honest I do prefer the "save and place", but in the usability review (comment #16) we recommended "save and configure".

Whichever is chosen I agree we should be consistent between the button/link text and the name of the page/form the user lands on.

i've played around with it a bit more. i think i am able to articulate the problem i have with the current state better now. the current possible use cases are:

1a. i click the add content block-button on /admin/content/block . when forwarded the destination is appended to the route: /block/add?destination=/admin/content/block

=> on the add content block page i only see the save button and on save i get forwarded back to /admin/content/block

1b. currently not possible to test but the implementation in admin toolbar will probably mimic the behavior in case 1a.

2. i click the add content block button on the place block modal on the block layout page on the consecutive add content block page, ?theme=olivero gets appended.

=> on the add content block page i only see the save button and on save i get forwarded to the configure block-page.

3. if i directly go to /block/add

=> without the destination appended on the add content block page i see the save and save and place in block layout button - the feature this patch adds.

in summary the behavior for case 2 and 3 is correct and expected, but my problem is with case 1a (and potentially with case 1b if implemented as expected). you only have the option to save and return to /admin/content/block. but as a user i either go to the /admin/content/block page and press the add content block button or i use the admin toolbar as a direct way to add a new content block. the new functionality the patch introduces would be hidden most of the time (don't know many who directly use /block/add as a shortcut or bookmark). with the patch the decision about the destination is basically up to the users choice. when save is pressed the destination is /admin/content/block when save and place in block layout is pressed the destination is the configure block page. clear and flexible. for that the appended destination on the add content block-button on /admin/content/block just would have to be stripped out?

and in regards of #29 a good point brought up by aaron. i have to admit i completely forgot about our recommendation from the usability meeting. i only considered the save and place in block layout button label as too long as noted in #25. but i agree with aaron that the choice that is picked should be consistent between button/link text and the h1 of the destination page.

on a side note the h1 on the configure block page is sort of unspecific. the title and h1 says simply configure block. in particular screenreader users are missing the context just based on the h1 and title (ref WCAG 2.1 SC 2.4.2 G88 -> The title can be used to identify the Web page without requiring users to read or interpret page content). With just Configure block as the h1 and page title the user doesnt know which block the page is about. would it make sense to change the pattern to something like Configure [name] content block? and would it make sense to create a follow-up for that?

I think this is blocked by #2950883: Allow form redirects to ignore ?destination query parameter

Even if I fix #1 in #30 it won't work with the destination parameter.

Blocker has been merged.

Added a simple test assertion.

I try to sum up the points left open, in part also a summary of the points already voiced in previous comments in here.

1. Currently the Add content block still uses a destination parameter /block/add?destination=/admin/content/block, that way the Save and Save and place in block layout buttons are unreachable. Instead you just get a Save button and you are redirected to the set destination /admin/content/block.
Only if manually enter the /admin/content/block route without the destination in your browser the new button functionality is accessible. Each of the buttons provides their own destination. Save forwards back to /admin/content/block</cod> while <code>Save and place in block layout forwards to the configuration page for the block and afterwards forwards to the block layout page /admin/structure/block.
Therefore i am not sure why it is necessary to add the destination paramater /block/add?destination=/admin/content/block on the Add content block button on /admin/content/block, cuz both of the new buttons provide clear destinations. I would just use /block/add for the code>Add content block button.

2. One detail i noticed while writing up the comment on Slack. For blocks created via /block/add you have two buttons: Save and Save and place in block layout. If you create a block via the Place block button on /admin/structure/block you just have a single Save button. Option wise that is correct since you are gonna create a block and place it afterwards in a block layoutanyway. Problem is that the button is called Save which might imply the user would be forwarded to /admin/content/block. Instead i would use the same naming like the second block Save and place in block layout for consistency and clarity reasons?

3. In regards of the micro copy of the second button Save and place in block layout it is way too long. Torrey Podmajersky puts it the following way in "Strategic Writing for UX":

The challenge of buttons is that they work best when they are recognizable, specific and only one or two words long.

as @aaronmchale noted in #29 the consensus in the ux meeting back then was Save and configure cuz it is more than just placing the block. for placing the select field at the bottom would already be enough, but you also have the option to change the visibility settings. that was the reasoning behind going with configure instead of place. place might be the better cue for users adding a block for the first time indicating that they gonna place the block after save somewhere. Each of the two variants have their advantages and disadvantages. asking actual users might be helpful in that case. the only thing is that Save and place in block layout should be shortened due to its length in any way.

4. Provide more context on the h1. currently it is just called Configure block. I think the easiest fix might be adding the block name (btw the title is using the same word Configure like Save and configure in point 3).
And in the current state it would make probably sense to also add the theme the block is configured for. but that will be obsolete when the potential follow up in 5 gets in:

5. FOLLOW UP: add a select field to the configure block page right between the visibility settings and the region select field so the user is able to set the theme as well as see the context one is configuring the block for and also knows where the block will be placed in the end.
Blocks created via the Add content block button could have the default theme as the default option. For blocks created via the Place block button i would avoid using a select field. it would have to be disabled which brings a11y related issues and also some usability related ones (a source for potential confusion). instead simply list the theme the block is placed in. but best would be to open a dedicated issue for avoiding disabled ui components in general in drupal core, something i am thinking about for a while (either hide components that you are unable to interact with, progressive disclosure so to say, and for others in cases like this show an interface component that looks like a select field but without the carret so it is just for informational purposes without any affordance to interaction)

#35.1 = removed the condition so the 2nd button appears every time for a new block
#35.2 = Updated so when adding block via block layout it will say "Save and configure"
#35.3 = updated to "Save and configure"
#35.4 = Believe that could be a follow up, as that is Drupal wide and not related to just this ticket (may already be a ticket for it)
#35.5 = Agreed in follow up

Surprisingly didn't fail too many tests.

I've successfully applied the latest changes.

#36.1-3 That looks fab! Awesome!
#36.4 I have no strong opinion if it should happen in this issue or a follow up. So totally fine with the follow up suggestion. and in regards of an already existing ticket. there was a regression ticket for the h1 for content types. the field ux initiative created also one recently. would have to look up the links. so the follow up could orient towards the patterns from there perhaps.

oh and i've noticed one minor detail. happens when the configured block is saved (i "think" i've seen that before). the styling for the active tab is missing :

not sure if the issue should better stay on needs review (cuz i am unable to provide a code review) or better go back to needs work because of the minor detail about the active theme tab. i left the status at needs review for now.

update: took another look and after Save and configure for a block added on /admin/content/block the is-active class is not set for the active secondary tabs link element. the problems does not apply to blocks added via the Place block button on the block layout page.

Added more tests.

I reviewed the code and it looks really clean. Really great usability addition as well!

There is a test failure though:

    1)
    Drupal\Tests\block_content\Functional\BlockContentCreationTest::testBlockContentCreationMultipleViewModes
    Behat\Mink\Exception\ExpectationException: Current page is
    "/block/add?destination=/subdirectory/admin/content/block", but
    "/block/add?destination=/admin/content/block" expected.

And I don't see the "replay" button to re-trigger the test.

That was a pain but think it’s been fixed. Nightwatch seems random

I've went through testing with the latest patch. The only thing that still applies is the issue about the missing active tab mentioned in #37.
And one other detail I am not sure if it was mentioned before (I've quickly skimmed through but haven't found anything) is if you go to /admin/structure/block, switch to for example Claro, click the "Place block" button in the "Content" section at the bottom, then click "Add content block" in the modal, select "Basic block", enter a description, click "Save and configure" => the region isn't set to "Content" where the button was initially pressed instead it is set to "-Select-". As I've said not sure if it was already raised nor if it is in the scope for this issue. But it would definitely convenient having it set. For people with a short working memory it might be helpful in case they get uncertain in which context they pressed the "Place block" button. It is easy to fix for the user afterwards but if it would be possible to prevent in the first place by providing some guard rails it might be even better? Those are the only small details aside that it looks great!

Believe the issue is going to be handled in #2833116: When Placing a Block on 'Configure Block' page the originally selected region is lost which is also on my radar to pivot to.

Ah thank you! yes that linked issue covers the second point perfectly!

So the only open detail is the first point described in #37 where the is-active class isn't added to the active tab when the user ends up on the block layout page at the end of the add and place a block flow.

Btw I got around to the block region ticket and put into review

One interesting detail that makes #2833116: When Placing a Block on 'Configure Block' page the originally selected region is lost probably also relevant to this issue and the detail i've raised in #37 about is-activ.

I've only applied the MR5562 from #2833116: When Placing a Block on 'Configure Block' page the originally selected region is lost to my test project. I've only manually tested and no matter if i add a new content block or if i place an existing block the region the place block button is pressed is kept. From a functional perspective MR5562 works perfectly. But the detail relevant for this issue.

If you add the block for Olivero, after you save the block the primary tab for Olivero is also missing the is-active class. if you add a block for Claro instead the primary tab is still shown. So since it happens in this issue as well in the slightly different context of #2833116: When Placing a Block on 'Configure Block' page the originally selected region is lost maybe the fix for the is-active issue for Olivero would make sense to be moved to a follow-up issue?

Checked #46 and confirming that the bug is not related to this ticket. I.e simply visiting /admin/structure/block/list/THEME where THEME is the default theme doesn't correctly highlight the active primary tab. This would be best fixed in a new issue that redirects the default theme to /admin/structure/block

1 minor nit otherwise this is good to go!

Addressed feedback

Nice work! This is looking solid now

Committed fca8641 and pushed to 11.x. Thanks!

Discussed with @catch and agreed that this is quite harmless to backport to 10.2.x.

Discussed with @catch and agreed that this is quite harmless to backport to 10.2.x.

I am a little surprised at that decision, since the RC for 10.2.0 was already released. If we were keeping Save (and go to the configuration page) as the primary submit button, then I would agree. But this issue makes that action the secondary button and changes the button text. Now the primary button, still labeled Save, goes somewhere else. I am afraid this will lead to some confusion.

I am also concerned that we are not giving the documentation team enough time to update https://www.drupal.org/docs/user_guide/en/block-create-custom.html, which still has a screenshot of the "Add custom block" page before this change.

To me this doesn't seem like a super commonly occurring bug because this would only happen when the ?destination query parameter is not set. If the user navigated to the form using links in the UI, the behavior of the primary submit button would be the same as now. The change they would see is that there's now a secondary button for the alternative action, which also seems like a small enough change to not be disruptive to the UX.

I don't see how this renders the documentation you listed invalid. In fact, it's already out of date because it hasn't been updated since #2317981: Move block content edit and delete routes under admin/content/block to improve IA for editors and fix breadcrumbs.

I think this caused a regression with block translations.

Editing no longer goes anywhere, you stay on that page unless you have a destination. Which is fine enough, but this behaviour now also applies to translations and specifically adding a new translation. Staying on the same page results in an exception as that translation then already exists.

I created #3412283: Editing a block_content entity no longer redirects to the overview as a follow-up.

Panels also has issue because of changed signature of BlockContentForm::actions() #3413891: Panels IPE block content edit form not loading with Drupal 10.2

EDIT

https://git.drupalcode.org/project/drupal/-/commit/26446bd20a596316c0284...

but actions() method is protected in EntityForm

@andypost is there a reason we changed the sig to public? It seems like we should fix that?