Blocks do not have a "remove" contextual link

I have to assume that you have #2826728: Block layout action removes instance, but contextual link deletes all instances. running locally, because there are no "Remove" contextual links for blocks yet.

In HEAD it says "Delete" because only custom blocks can be deleted.

I think this and that other issue should be combined into a feature request for "Add a 'Remove' contextual link to placed blocks" or similar.

Too bad this little guy got stalled. Consistency is a platform asset. :)

Feel free to review the patch!

I did. It's pretty straightforward. It failed tests, so I assume that's a blocker? Or no?

It failed tests because the tests were brittle. Those can be fixed.
But did it work when you tried it out?

@tim.plunkett
This patch is working as expected and adding "Remove" contextual link for the blocks.

I've also added additional test cases where we are rendering static blocks like "powered by Drupal".
That block has two links as it is static.

Patch works as intended. Adding video evidence.

Hi @mohit_aghera,

I applied your patch but not adding "Remove" contextual link for the blocks.

i created custom block and placed but didn't find any changes.

please review the attachment.

Thanks!

@djsagar
I am able to see the links on the blocks created via "block_content"
Can you please share the exact steps?

Applied patch #20 and it works fine.

Before patch:

After patch:

RTBC +1

I verified The patch #20 and its working as expected. #22 I think the problem you are facing is because of the block cache, try clearing it once.
Added a screenshot.
Thanks

Applied patch #20 and it's working fine for me.

For #22 comment, I think this issue is replicated after applied patch, old block not adding "Remove" link. We need to add new custom block then it will replicating.

You can check this thing in attached GIF.

patch #20 applied cleanly and working as expected , and have a "remove" contextual link ,screenshot added for reference

Patch #20 applied cleanly and it is working well.
Reviewed the #26 and #27 screenshots, Looks good
screenshot for reference.
Added "remove" action contextual link
Marking as RTBC

1. +++ b/core/modules/block/block.links.contextual.yml
   @@ -2,3 +2,8 @@ block_configure:
   +  title: 'Remove'
   
   +++ b/core/modules/block/block.routing.yml
   @@ -13,7 +13,7 @@ entity.block.delete_form:
   +    _title: 'Remove block'
   

   Similar to the "Configure block" contextual link (and to the "Remove block" one already in Layout Builder), I think both of these should be "Remove block"

2. +++ b/core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php
   @@ -394,8 +394,23 @@ public function testBlockContextualLinks() {
   +    $id_token = Crypt::hmacBase64($id, Settings::getHashSalt() . $this->container->get('private_key')->get());
   ...
   +    $url = '/contextual/render?_format=json,destination=/';
   +    $this->getSession()->getDriver()->getClient()->request('POST', $url, $post);
   ...
   +    $this->assertSame('<ul class="contextual-links"><li class="block-configure"><a href="/admin/structure/block/manage/' . $block_id . '">Configure block</a></li><li class="block-remove"><a href="/admin/structure/block/manage/' . $block_id . '/delete">Remove</a></li></ul>', $json[$id]);
   

   This is very trick test code. Could this instead be added to a FunctionalJavascript test and use a normal assertion?

Thanks @tim.plunkett
I've updated the title and test cases so that it uses FunctionalJavascript tests.

@tim.plunkett
I've refactored tests to separate classes.
Contextual links test in BlockFilterTest was failing for the ClaroBlockFilterTest tests.
The reason is, for the claro theme, we are disabling contextual links for a few blocks.

Update
Tests are passing on local.
Here is the quick output.

ohit@drupal8-web:/var/www/html$ vendor/bin/phpunit -c core --filter=BlockContextualLinksTest
PHPUnit 8.5.13 by Sebastian Bergmann and contributors.

Testing
.                                                                   1 / 1 (100%)

Time: 45.5 seconds, Memory: 808.00 MB

OK (1 test, 3 assertions)

Triggering the test build again.

+++ b/core/modules/block/tests/src/FunctionalJavascript/BlockContextualLinksTest.php
@@ -0,0 +1,54 @@
+    $this->assertSession()->responseContains('<ul class="contextual-links" hidden=""><li><a href="' . base_path() . 'admin/structure/block/manage/' . $this->blockId . '?destination=/user/' . $this->rootUser->id() . '">Configure block</a></li><li><a href="' . base_path() . 'admin/structure/block/manage/' . $this->blockId . '/delete?destination=/user/' . $this->rootUser->id() . '">Remove block</a></li></ul>');

This needs to wait for the contextual links to render before asserting. It's passing locally for you because you are running a single test. The testbot runs 15 test concurrently, so the gap between page render and contextual links render is longer.

Something like

$this->assertSession()->waitForElement('css', '.contextual button');

(selector might need adjusting)

Additionally, can we assert that the page contains what we want without needing all of the markup hardcoded in the assertion?

Updating the suggestions mentioned in #36

Please do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed. I have not reviewed the code.

This feels like a feature request to me, and @tim.plunkett said the same in #3

Restoring status after HEAD was broken.

Code looks straight-forward enough! Here's a review from the UX side.

The screenshot in the issue summary shows "Remove" as the last option, and that also tracks with what I'd expect, since it is very likely to be the least used option.

And yet... Here it is in the middle, on a menu block?

Upon clicking "Remove block," it busts you out of the front-end into a standalone confirm page using the admin theme:

This is at odds with what Quick Edit is intending to do, which is keep all of your admin functions on the front-end. Is there a reason a dialog wasn't used here for the confirm box instead?

And finally, the wording there is super spooky... It makes it sound like you're removing the User account menu block from the block system entirely, but you're really only removing its placement from this one region. And it can easily be undone. Do we have options to rephrase here? For example:

"Are you sure you want to remove the block User account menu from the Secondary menu region?
This will remove the block placement. You will need to place it again in order to undo this action."

Thanks for the review! There are three points of feedback: order of the links, redirecting to admin theme, and the wording of the form.

1)
Contextual links have a weight property, but they also have a group property. The weight only affects their order within their group.
The "Configure block" and new "Remove block" links have a group of block.
The "Edit menu" link has a group of menu.
This means that no matter how high of a weight we set for the new link, it will still be below the "Edit menu" link.
In theory we could implement hook_contextual_links_view_alter() to move it to the bottom... But that's really heavy-handed.

2)
Currently all of the contextual links redirect to a page in the admin theme.
The Settings Tray module was working to stop doing this, but it's
a) not installed by default
b) does not fix these links to use it

We don't use dialogs for any of this right now.

I opened #3215209: After clicking "Remove block" contextual link, open form in off-canvas dialog as a follow-up for this

3)
I changed the wording of the form to match your suggestion. That's the only code change currently...

NOTE: I switched from patches to a new MR!

Took a look at this and I think that @webchick's feedback has been addressed. Restoring RTBC!

Tim walked me through this again today and lookin' good! All feedback addressed.

Committed and pushed to 9.3.x. Thanks!