Cannot delete or edit a block that is placed in a section of the layout_builder

Please note there is no need to run patches on every single test environment, just one test is sufficient for most cases. Normally you should just set the issue to "Needs review" and the relevant test will be selected automatically.

tagging for badcamp 2018

Not sure about hardcoding knowledge of modules into themes, but the patch looks like it would work.

Note that in the original designs, there was never a plan to use LB with admin themes.

Thanks for your work on this, Lennard Westerveld!

The use case where this is helpful for me is a site where there are many entities, some of which are user-facing and some are admin-facing. The admin-facing entities (so far) are using a sub theme that has Seven as its base theme. This works great for the admin folks - they get a clean, dense, distraction-free page. I ran into trouble when using the Layout Builder for these entities and this patch has solved that problem.

I can see what you're saying, Tim, about the theme being overly integrated with the layout module. At the same time, it seems that the Seven theme is already closely coupled to contextual links here, so perhaps that is OK?

I was so confused by this because I have Admin Theme installed and set it to /admin/*. So it was forcing the admin theme for the layout builder interface. I was like, "What do you mean you weren't supposed to use layout builder with admin themes? You use the admin theme to edit the layout!" Everything makes a lot more sense now.

I have the same issue using seven or adminimal_theme by default.

Patch #5 fix it. Thanks @Lennard Westerveld

Tagging for DrupalNorth 2019

Patch in #5 works, thanks @Lennard Westerveld.

Using Drupal 8.7.3 combined with menu_item_extras and menu_block.

Based on comments, I'm setting to RTBC.
If you think differently, feel free to change.

Patch #5 worked for me as well.

Using 8.7.3 with simple_megamenu

The docblock needs updating as * Disables contextual links for all blocks. is now wrong. It feels like this is something that should be tested. Especially as we're about to add a new admin theme. Here's a patch that adds a test for Seven theme and its seven_preprocess_block().

Patch needs review because we now add a test.

Test looks good.

+++ b/core/tests/Drupal/FunctionalTests/Theme/SevenTest.php
@@ -0,0 +1,115 @@
+    $page = $this->getSession()->getPage();
...
+    $page->fillField('settings[label]', 'This is the label');
+    $page->checkField('settings[label_display]');
+    $page->pressButton('Add block');

I think we need to refresh the page object as it is not up to date with the latest response.

@jibran I run tests locally before submitting a patch so that's not the issue. The problem is that between 8.7.x and 8.8.x the text of the button changed from Add Block to Add block :(

Rerunning tests against 8.8.x

Patch #16 works.

Tagging for DrupalCamp Colorado 2019 (Sunday August 4)

This issue also affects Claro. #3079738: Add Claro administration theme to core

One thing we should consider here is the scenario where a theme is used as both the backend and frontend theme. @gabesullice and @zrpnr were testing a site with that setup and were completely stumped by this bug. If the theme is used in both backend and frontend, do we really want to hide the contextual links blocks from paths that are frontend paths?

Also, do we actually still want to stick with #2487025: Remove contextual links in Seven in the first place?

I think we can keep the scope of this issue to fixing Layout Builder since that's a major-almost-critical bug, and open a followup to discuss the broader picture of what we need to do with this hook in the future.

Not sure about hardcoding knowledge of modules into themes, but the patch looks like it would work.

I don't think there's anything wrong about hardcoding knowledge of modules into themes, as long as they still work without those modules. In fact, a lot of existing code in themes is already specific to modules, for example bartik_preprocess_node is specific to node module.

Would be great to see some screenshots how contextual links work in Seven. Since contextual links have been more or less always disabled, we haven't been actively testing them.

Tagging for badcamp2019, thanks! (October 2-5)

Patch in #16 work for me too.

Thanks @alexpott

Patch in #16 applies and works for 8.9.

Hi,

I have reviewed and manually tested below are my updates:

1) I created "layout_builder_test" content type
2) I enabled the "layout_builder" module for layout_builder_test content type
3) Screenshot before applying the patch

4) Screenshot after applying the patch

Worked as designed

I am also attaching screenshot for code sniffer result and PHP unit test result which passed.

Thank you
Ren

Fixing Claro as well - plus adding a test for Claro.

Hi,

I just reroll the patch and it's working great.

Bellow is my output screenshot of Edit "Layout Builder" page for claro as well as seven

1). Seven Theme

2). Claro Theme

Bellow is my output screenshot of test results for claro as well as seven.

1). Seven Theme

2). Claro Theme

Thanks,
Ren

We still need follow-up for #25. 🙏

Filed #3097059: What do we want to do with hook_preprocess_block()?.

+++ b/core/tests/Drupal/FunctionalTests/Theme/SevenLayoutBuilderTest.php
@@ -0,0 +1,120 @@
+      // Note that Seven is not supported as a frontend theme but it is enabled
+      // here to test layout builder.

Have we actually documented somewhere that Seven is not supported as a frontend theme?

Discussed with @alexpott and we agreed to remove the comment mentioned in #38 to not hold this fix to a major bug on that. This can be done on commit.

1. +++ b/core/tests/Drupal/FunctionalTests/Theme/ClaroLayoutBuilderTest.php
   @@ -0,0 +1,110 @@
   +    // Ensure other blocks do not have contextual links.
   +    $assert_session->elementNotExists('css', 'div.block-page-title-block.contextual-region div[data-contextual-id]');
   

   We're not placing the page title block on the Claro test at all. I think we might have a false positive because we are not asserting if the page title exists.

2. +++ b/core/tests/Drupal/FunctionalTests/Theme/SevenLayoutBuilderTest.php
   @@ -0,0 +1,120 @@
   +  protected $defaultTheme = 'seven';
   ...
   +    $this->assertTrue($this->container->get('theme_installer')->install(['seven']));
   +    $this->container->get('config.factory')
   +      ->getEditable('system.theme')
   ...
   +      // here to test layout builder.
   +      ->set('default', 'seven')
   +      ->set('admin', 'seven')
   +      ->save();
   

   Do we need this with the $defaultTheme property?

Thanks for the review @lauriii

RE #40.1 Well Claro ships with block config so it doesn't need to place the block BUT we can improve the test by making a positive assertion that the expected block is present - since having a negative assertion on it's own if very prevalent to false positives in the future.

#40.2 - yep this can all be removed which addresses #38 too.

Since these are test only improvements I'm going to set this to RTBC.

I made minor coding standards improvement. Interdiff attached.

Committed 221ec16 and pushed to 9.0.x and 8.9.x. Thanks!

This is a major bug fix with low risk so I'm leaving this open to be cherry-picked to 8.8.x once the freeze is over.

Cherry-picked to 8.8.x.

I Am still facing this issue while i have all the patches mentioned here.

My issue had nothing to do with Seven Theme, but I'm leaving this comment because this page came up high on search for "Cannot edit or delete block in layout_builder". My issue was related to the Layout Builder UX. See issue and patch at https://www.drupal.org/project/lb_ux/issues/3116402