Problem/Motivation
With a clean install and enabling the admin theme as default you cannot remove blocks seperately then fully removing the section.
So if you make a mistake by adding a wrong block you need to start over again because you cannot edit or remove blocks separately.
1. Go to https://simplytest.me or install a new Drupal 8.6.1 instance locally
2. Make Seven your default theme
2. Enable layout_builder
3. Create a content type
4. Enable layout_builder on the newly created content type by view display -> Default -> Check "Use Layout Builder" and
"Allow each content item to have its layout customized."
5. Go to manage display and "Manage Layout"
6. Add 2 blocks and try to remove 1 of the 2 blocks.
Proposed resolution
-
Original report by Lennard Westerveld
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff.txt | 1.57 KB | lauriii |
#41 | 3005403-2-41.patch | 8.34 KB | alexpott |
#41 | 35-41-interdiff.txt | 2.15 KB | alexpott |
#35 | interdiff.txt | 8.57 KB | rensingh99 |
#35 | 3005403-35.patch | 8.56 KB | rensingh99 |
Comments
Comment #2
Lennard WesterveldFound out that it is an issue with the "Seven" theme in seven.theme\seven_preprocess_block all contextual_links gets deleted.
Comment #3
longwavePlease 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.
Comment #4
Lennard WesterveldOh, awesome I didn't now that feature.
Comment #5
Lennard WesterveldCreated a patch based on the /core/themes path instead of /themes.
Comment #6
Lennard WesterveldComment #7
Lennard WesterveldComment #8
volkswagenchicktagging for badcamp 2018
Comment #9
tim.plunkettNot 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.
Comment #10
gregglesThanks 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?
Comment #11
jwineichen CreditAttribution: jwineichen commentedI 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.
Comment #12
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedI have the same issue using seven or adminimal_theme by default.
Patch #5 fix it. Thanks @Lennard Westerveld
Comment #13
volkswagenchickTagging for DrupalNorth 2019
Comment #14
bisonbleu CreditAttribution: bisonbleu commentedPatch 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.
Comment #15
PCate CreditAttribution: PCate commentedPatch #5 worked for me as well.
Using 8.7.3 with simple_megamenu
Comment #16
alexpottThe 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().Comment #17
alexpottPatch needs review because we now add a test.
Comment #20
jibranTest looks good.
I think we need to refresh the page object as it is not up to date with the latest response.
Comment #21
alexpott@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
toAdd block
:(Rerunning tests against 8.8.x
Comment #22
g089h515r806 CreditAttribution: g089h515r806 commentedPatch #16 works.
Comment #23
volkswagenchickTagging for DrupalCamp Colorado 2019 (Sunday August 4)
Comment #24
xjmComment #25
xjmThis 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.
Comment #26
lauriiiI 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.
Comment #27
volkswagenchickTagging for badcamp2019, thanks! (October 2-5)
Comment #28
Kris77 CreditAttribution: Kris77 commentedPatch in #16 work for me too.
Thanks @alexpott
Comment #30
capysara CreditAttribution: capysara at Bounteous commentedPatch in #16 applies and works for 8.9.
Comment #31
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedComment #32
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedHi,
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
Comment #33
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedComment #34
alexpottFixing Claro as well - plus adding a test for Claro.
Comment #35
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedHi,
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
Comment #36
lauriiiWe still need follow-up for #25. 🙏
Comment #37
gregglesFiled #3097059: What do we want to do with hook_preprocess_block()?.
Comment #38
lauriiiHave we actually documented somewhere that Seven is not supported as a frontend theme?
Comment #39
lauriiiDiscussed 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.
Comment #40
lauriiiWe'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.
Do we need this with the $defaultTheme property?
Comment #41
alexpottThanks 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.
Comment #43
lauriiiI 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.
Comment #46
lauriiiCherry-picked to 8.8.x.
Comment #48
kumar_kundan CreditAttribution: kumar_kundan commentedI Am still facing this issue while i have all the patches mentioned here.
Comment #49
GarChris CreditAttribution: GarChris commentedMy 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