Add visibility control conditions to blocks within Layout Builder

Clearing out the issues pointing at the implementation instead of the plan.

How about something along these lines?

Eclipse

Sorry for the trash in that patch. I'll fix it in the morning.

Eclipse

The last submitted patch, 5: 2916876-5.patch, failed testing. View results →

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

Developed against preview versions of the patches we were dependent on. It's also dependent on #2922033: Use the Layout Builder for EntityViewDisplays → , so this won't work w/o that.

Eclipse

If I would like to test the patch, I'm unclear as to what other patches I need to apply other than from:

https://www.drupal.org/project/drupal/issues/2922033 →

Not sure what "Developed against preview versions of the patches we were dependent on" means.

I'm not sure, but I believe it's only the last patch from that issue that is needed by this one.

@EclipseGc until that's in, when you post patches can you generate two? One suffixed with -review.patch that is code since the blockers, and one suffixed -do-not-test.patch that contains everything since 8.6.x?

Will do. I'll reroll this shortly.

Eclipse

Ok, I didn't have a chance to reroll against only head, so this patch is dependent on #2922033: Use the Layout Builder for EntityViewDisplays → comment 62. I will get a version of this patch for core Jan 18th.

There are a bunch of interesting bits in the interdiff. Most importantly, I rewrote how SectionComponents are rendered at all. This will likely need to be split out into a parent issue of this issue, but for the sake of explaining what's here, I converted SectionComponent render array building into an event which is dispatched. This is super powerful because it means other modules can get involved in the render array building process. It also means that layouts COULD potentially render things which are not blocks. Tim wrote the original code that way and I doubled down on it here. I don't think it's a feature we'll practically use, but we essentially get it for free moving to the event dispatcher. Furthermore, being able to run before or after the normal build process means that introducing something like "visibility" into that paradigm became a simple event subscriber instead of me modifying the monolith that did the render work for us (which is what the last patch did).

Net 0 for the SectionComponent class at the moment. The interdiff removes the condition manager that the first patch added, but adds in the event dispatcher service. Still, that's a practical service when I felt that the condition manager really wasn't.

I'll post more tomorrow. (I'm pretty certain visibility condition editing, not creating, is broken. So that's one of tomorrow's tasks.)

Eclipse

Also, if you're wanting to try out a full demo of what we're pursuing, this is accurate as of now, and all the patches should layer just fine:

git clone --branch 8.6.x https://git.drupal.org/project/drupal.git layout_builder
cd layout_builder; curl https://www.drupal.org/files/issues/2922033-layout_defaults-62.patch | git apply; curl https://www.drupal.org/files/issues/2916876-13.patch | git apply; curl https://www.drupal.org/files/issues/2937159-layout_drag-2.patch | git apply; curl https://www.drupal.org/files/issues/2936642-context-2.patch | git apply; composer install

So hopefully that helps with bootstrapping review.

Eclipse

Ok, I've rerolled this on top of #2937799: Allow greater flexibility within SectionComponent::toRenderArray() → after abstracting that code out of this patch. Also, this should apply directly to core now with only that 1 patch applied. Layout defaults are a separate thing and I don't think it's affected by this code path at all.

I've not provided an interdiff because I actually wasn't certainly how best to get one given that much of the code I removed from the patch still exists in the code base. Whatever the case, the main patch should be significantly smaller now. Fixed a couple of docblocks I'd missed here and there.

Eclipse

Ok, fixed the missing end line I had in the last patch and made editing a configured visibility condition actually work now.

Eclipse

Hmm... maybe I'm doing something wrong but here's what I did based on comment #14 and patch #16:

git clone --branch 8.6.x https://git.drupal.org/project/drupal.git layout_builder
cd layout_builder; curl https://www.drupal.org/files/issues/2922033-layout_defaults-62.patch → | git apply; curl https://www.drupal.org/files/issues/2916876-16.patch → | git apply; curl https://www.drupal.org/files/issues/2937159-layout_drag-2.patch → | git apply; curl https://www.drupal.org/files/issues/2936642-context-2.patch → | git apply; composer install

And then I added a block but I don't see any visibility info. I don't see it in the contextual links either. Perhaps I'm looking in the wrong place?

Note, contextual links are currently cached *by your browser, per tab*.
After clearing caches, make sure you test it in a new tab.

Yeah, it's a totally annoying thing, but you'll need to follow Tim's directions. :-(

Eclipse

Also, the patches I've provided are against head and no longer dependent on the defaults issue, so apply these patches, and forgo the defaults patch for the time being (until it lands and I reroll).

curl https://www.drupal.org/files/issues/2937799-8.patch | git apply; curl https://www.drupal.org/files/issues/2916876-16.patch | git apply; curl https://www.drupal.org/files/issues/2937159-layout_drag-2.patch | git apply; curl https://www.drupal.org/files/issues/2936642-context-2.patch | git apply

In addition to #21, I went in to try to delete the condition and wasn't able to get the dropdown to show up consistently. And, the contrast is really bad which I imagine you already know.

Thanks for the tips. I see the "Control visibility" in the contextual links now. It works as expected. Here's what I did:

1. Enabled layout modules
2. For a basic page node, edited the layout to show the node body
3. Used the block visibility to only show the body to authenticated users
4. Verified for admin, the body showed up
5. Verified for anonymous, the body did NOT show up

1. I looked at the code as best I could for structure and formatting. Someone else should review for logic. Feedback is out of order in some cases as I went into dreditor a couple times.

   +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
   @@ -0,0 +1,162 @@
   +  protected $plugin_id;
   

   Nitpick: Should be $pluginId instead of $plugin_id? (I noticed that was the convention in other places.)

2. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
   @@ -0,0 +1,162 @@
   +    return $this->t("Are you sure you want to delete this visibility condition?");
   

   Nitpick: Use single quotes?

3. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
   @@ -0,0 +1,162 @@
   +    $response->addCommand(new OpenOffCanvasDialogCommand($this->t("Configure Condition"), $new_form));
   

   Nitpick: Use single quotes?

4. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
   @@ -0,0 +1,162 @@
   +   * Gets the parameters needed for the various url and form invocations.
   

   Nitpick: url => URL?

5. +++ b/core/modules/layout_builder/src/Section.php
   @@ -68,14 +68,16 @@ public function __construct($layout_id, array $layout_settings = [], array $comp
   +   *   (Optional) Determines if we're rendering a preview or not.
   

   Nitpick: Don't use "we're"... perhaps:

   Whether the component is in preview mode or not.

   which is used in SectionComponentBuildRenderArrayEvent.php

6. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -91,12 +91,14 @@ public function __construct($uuid, $region, array $configuration = [], array $ad
   +   *   (Optional) Determines if we're rendering a preview or not.
   

   Nitpick: Same as comment above.

7. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,209 @@
   +   * The block manager.
   +   *
   +   * @var \Drupal\Core\Condition\ConditionManager
   +   */
   +  protected $conditionManager;
   

   Nitpick: Should comment say "The condition manager."?

8. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,209 @@
   +   * The field delta.
   +   *
   +   * @var int
   +   */
   +  protected $delta;
   

   Question: Field delta? Isn't this the block delta?

9. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,209 @@
   +   * Constructs a new BlockVisibilityForm.
   

   Nitpick: Constructor above is "Creates a SectionComponentVisibility object." so it would be good to standardize on either that format or this one.

10. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,209 @@
    +    $response->addCommand(new OpenOffCanvasDialogCommand($this->t("Configure Condition"), $new_form));
    

    Nitpick: Use single quotes?

11. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,248 @@
    +   * The plugin form manager.
    +   *
    +   * @var \Drupal\Core\Plugin\PluginFormFactoryInterface
    +   */
    +  protected $pluginFormFactory;
    

    Nitpick: The plugin form factory.

12. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,248 @@
    +   * The field delta.
    

    Question: Same as above for BlockVisibilityForm.

13. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,248 @@
    +   * The plugin being configured.
    

    Nitpick: The condition plugin being configured?

14. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,248 @@
    +   * Constructs a new ConfigureVisibility.
    

    Nitpick: Same as above regarding constructors.

15. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
    @@ -0,0 +1,162 @@
    +   * The field delta.
    

    Question: Same as above for ConfigureVisibility.

16. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
    @@ -0,0 +1,162 @@
    +   * The uuid of the condition to be removed.
    +   *
    +   * @var string
    +   */
    +  protected $plugin_id;
    

    "uuid" => "plugin id"

17. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
    @@ -0,0 +1,162 @@
    +   * Constructs a new DeleteVisibility.
    

    Nitpick: Same as other constructors above.

Yeah, that's a failing of the css reset on the settings tray. An on-going problem in core. :-( I've reached out to people who would know how best to solve it.

Eclipse

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

My goodness that was a mess of a reroll. A lot has happened since this was being developed and my reroll script couldn't even find a place to cleanly apply this.

Here is a "clean" reroll and I'll take a look at the feedback.

re #21
1-3. Fixed.
4. I think Url() actually. It refers to the Url class.
5 & 6. n/a. was added in another patch.
7. fixed
8. Its actually the section delta. I've updated the comments to reflect this.
9. Fixed to "Constructs a X object" based on DrupalKernel::__construct()
10. fixed
11. fixed
12. same as 8
13. fixed
14. same as 9
15. same as 8
16. fixed
17. same as 9

Re #22
I'll have to look into that more. Not styles I'm familiar with.

The last submitted patch, 27: 2916876-27.patch, failed testing. View results →

Having trouble running this test locally but this fixes the error with explicit tests. Still working out some of the testing but since I can't run it locally see what testbot thinks of the changes.

bah, i guess i was to fast clicking through the uploads. Patch.

The last submitted patch, 30: 2916876-layout-visibility-29.patch, failed testing. View results →

*sigh*

The last submitted patch, 32: 2916876-layout-visibility-32.patch, failed testing. View results →

I have no idea where to start with the final error. Clearly the patch only adds the class once and phpunit runs the unit test suite fine locally... Testbot seems to be handling things in an unexpected way.

+++ b/core/modules/layout_builder/tests/src/Unit/EventSubscriber/SectionComponentVisibilityTest.php
@@ -0,0 +1,144 @@
+    $this->markTestIncomplete('Still working out how to test this.');

When running locally, I get a yellow I. With -v, I get

There was 1 incomplete test:

1) Drupal\Tests\layout_builder\Unit\SectionComponentVisibilityTest::testOnBuildRenderNonPreviewResolveCondition
Still working out how to test this.

Which lead me here.

Yeah that definitely needs work I just put it there so I'd come back while testbot decided if this still integrated ok with everything since those wouldn't run locally.

Thanks for the pointer on the namespace in slack, hopefully that fixes what ever weirdness testbot experienced.

test the bejesus out of the visibility propagation.

No pending "incomplete tests" or anything, this is actually... pretty good I think.

Trivial conflict with #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder → . No interdiff because its not code related, just service definition.

I have not looked at the code yet; but doing manual test.

The base UI still need some work:

Here are some additional opinions on the UI and UX:

When I'm updating a condition, I would like to stay on the Configure visibility, if for instance I would like to do more changes. Same goes with adding a new condition and deleting condition.

The Add a new condition should be moved down to be aligned with the button. Currently added conditions are in the middle of these.

I really, really like this feature though. It can add some additional serious power for the site builders, for instance if Layout Builder could define conditions like "Hide this field block X if field block Y is not empty", etc.

Great work so far. A few questions and nits.

1. +++ b/core/modules/layout_builder/layout_builder.links.contextual.yml
   @@ -17,3 +17,13 @@ layout_builder_block_remove:
   +layout_builder_block_visibility:
   +  title: 'Control visibility'
   +  route_name: 'layout_builder.visibility'
   +  group: 'layout_builder_block'
   +  options:
   +    attributes:
   +      class: ['use-ajax']
   +      data-dialog-type: dialog
   +      data-dialog-renderer: off_canvas
   

   I don't think this contextual link will show up for existing user with existing browser session.

   Maybe related to 🐛 New contextual links are not available after a module is installed Needs work

   Might have to do hook_update_N to do something like we did in settings_tray_install()

   Would we need an update test for this?

2. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
   @@ -0,0 +1,84 @@
   +class SectionComponentVisibility implements EventSubscriberInterface {
   

   Is there reason for this to be it's own event subscriber class rather than handle this in \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray::onBuildRender()
   with something like

   $access = $block->access($this->currentUser, TRUE);
         $access = $access->andIf($this->allowedVisibility($event));
   

   And then move all the logic from \Drupal\layout_builder\EventSubscriber\SectionComponentVisibility::onBuildRender() into the new \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray::allowedVisibility() which would return an AccessResultInterface.

   You won't need to worry about stopping propagation either.

3. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,210 @@
   +   * @return array
   

   Missing return description

4. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
   @@ -0,0 +1,249 @@
   +   * @param string $condition_id
   +   *   A condition UUID, or the plugin ID used to create a new condition.
   +   *
   +   * @param array $value
   +   *   The condition configuration.
   

   Extra blank line

5. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
   @@ -0,0 +1,162 @@
   +   * @return array
   

   Missing return description

This is important. And will likely make it in to 8.7 anyway. But it's not a stable blocker.

1. I'm not sure I understand this completely. Probably because I only skimmed the other issue. We'd be writing an update hook to work around a bug in core? If so I'd say we just skip it since layout builder is unstable and focus on fixing the other bug. Good to know though.
2. Long story. I tried and actually had a patch rolled . I was hesitant because of the large weight gap implied a reason but skipping the forced render in BlockComponentRender seemed worth the loss of flexibility for contrib.

I realized I forgot to get the test sorted out and started looking and I came to the second reason. Or really back around to the first. BlockComponentRender makes a big assumption its working on a block and drops out immediately if it isn't. I started to force the block plugin into the section for the test to get to the right code but this triggered a realization, the gap in the weights is actually there for a reason and an important one if I understand things correctly. Sections can contain different "things" and the render event is in charge of getting it to a render array. SectionComponentVisibility is more generalized and separated to support this so it can do its logic without running into any render listeners.

So... I guess it needs to stay the way it is.
3. fixed
4. fixed
5. fixed

Its at the very least expected functionality by anyone coming from any layout system in any other version of Drupal including just using blocks in 8.x core so I'm going to mark this major. I think we could say layouts is stable but its not really for site builders until this is in but its really moot if it gets in so lets have that drag out fight in 3 or 4 months. :)

Trying this out on a local install of 8.6.1. A couple of UI/UX comments...

• Overall, I agree with #39.
• It's difficult to figure out how to delete a condition. I suspect this is mainly a CSS issue
• When I add a condition, it immediately closes the Visibility Condition tray. If I want to add a second condition (or simply verify my new condition) I need to open it again.
• It would be useful to have a visual indicator on blocks that have conditions, even if just in the form of Control Visibility (2) on the contextual menu.

Further, I'd like to see this facility for multiple conditions in core so it's available for regular blocks as well.

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

I'm glad the reviews seem to be mostly cosmetic and things "work". I think we can move this back to needs work as we all agree it needs some UI love.

I might be able to work on the flow.

Unfortunately, I'm going to need help with the look and feel though. My time is limited and this patch sadly has 0 CSS and the mangling is all happening as part of the default off-canvas form styling which is outside my skill set.

With #2916876-42: Add visibility control conditions to blocks within Layout Builder → applied to 8.7-rc1 the "control visibility" context action will not show up at every block. I have a 25/50/25 layout and in each area a block placed. First i only saw the action at the last block. After moving the blocks around and saving the layout i saw the action now at 2 of 3 blocks. Then i had to move the 3. block into the last area and it showed up there too.

Working on the off-canvas CSS issues identified above + anything else I might run into. Patch on the way.

1. To address the styling issues with the condition listing, I changed the dropbutton to independent "edit" and "delete" links. Since there are only two options, this occupies a similar amount of space as the dropbutton, and has the benefits of un-obscuring the delete option (addressing #43(second point)) and eliminating the need to deal with off-canvas reset CSS.
   
2. Added FunctionalJavascript tests that confirm the conditions actually impact visibility. It also confirms the "Control visibility" links are consistiently present, which helps confirm my suspicion that the inconsistiency in #48 is due to cached contextual links in local storage (which are typically addressed by clearing cache and opening the layout in a new tab). If the inconsistient presence of linkjs continues to be a problem, additional steps to reproduce would be great - including block type and if it is a Layout Default or Layout Override.
3. Added title callbacks for all the forms
4. Per the second point in #39Moved the conditions list so it does not appear between the Conditions dropown and "Add Condition" submit button.
5. There were a few suggestions in above comments that seemed good, but was not immediately clear how feasible they are. If anyone thinks any of the following seem like must-haves or can be implemented without much difficulty, please comment.
   • #39(first suggestion):
     

     When I'm updating a condition, I would like to stay on the Configure visibility, if for instance I would like to do more changes. Same goes with adding a new condition and deleting condition.

     This would deviate from the pattern used by several other layout builder forms that present a second off-canvas dialog to configure the choice made in the first. Keeping things in a single dialog is certainly more challenging than having a followup dialog -- I'd like to see if this was avoid for reasons in addition to implementation overhead.

   • #43(last point)
     

     It would be useful to have a visual indicator on blocks that have conditions, even if just in the form of Control Visibility (2) on the contextual menu.

     The way contextual links are cached may make this tricky to implement. Would be curious if there are other ideas regarding how to visually indicate the presence of visibility rules.

I know this issue is still being iterated on, but here's a code review all the same

1. +++ b/core/modules/layout_builder/layout_builder.routing.yml
   @@ -145,3 +145,40 @@ layout_builder.move_block:
   +  requirements:
   +    _permission: 'configure any layout'
   ...
   +  requirements:
   +    _permission: 'configure any layout'
   ...
   +  requirements:
   +    _permission: 'configure any layout'
   

   Should be _layout_builder_access: 'view' now

2. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
   @@ -0,0 +1,84 @@
   +    $events[LayoutBuilderEvents::SECTION_COMPONENT_BUILD_RENDER_ARRAY] = ['onBuildRender', 255];
   

   Please document the magic number (aka indicate which other subscriber you are trying to run before/after)

3. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
   @@ -0,0 +1,84 @@
   +      $visibility = $event->getComponent()->get('visibility');
   +      $visibility = $visibility ? $visibility : [];
   

   This can be a one-liner, adding ?: []; after the get() call

4. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
   @@ -0,0 +1,84 @@
   +      foreach ($visibility as $uuid => $condition) {
   +        $condition = $this->conditionManager->createInstance($condition['id'], $condition);
   

   Feels wrong to reuse the $condition name for both. Maybe the first one from the loop should be $configuration?

5. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
   @@ -0,0 +1,84 @@
   +    // @todo Add and/or resolution form element.
   

   This should have an issue opened (unless it's intended to be solved in this issue?)

6. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
   @@ -0,0 +1,84 @@
   +      $event->addCacheableDependency($event->getPlugin());
   

   I don't understand this line, probably deserves a comment?

7. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
   @@ -0,0 +1,84 @@
   +      // If conditions do not resolve, do not process other subscribers.
   +      $event->stopPropagation();
   

   Hmmmm I'll have to think about this one

8. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,241 @@
   +
   +    $visibility_conditions = $section_storage->getSection($delta)->getComponent($uuid)->get('visibility');
   

   This reminds me of the helpers like \Drupal\layout_builder\Form\ConfigureBlockFormBase::getCurrentComponent()

9. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,241 @@
   +    $visibility_conditions = $section_storage->getSection($delta)->getComponent($uuid)->get('visibility');
   +    if (!$visibility_conditions) {
   +      $visibility_conditions = [];
   +    }
   

   once again with ?: []

10. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,241 @@
    +    $conditions = [];
    +    foreach ($this->conditionManager->getDefinitionsForContexts($this->getAvailableContexts($section_storage)) as $plugin_id => $definition) {
    +      $conditions[$plugin_id] = $definition['label'];
    +    }
    ...
    +    foreach ($visibility_conditions as $visibility_id => $configuration) {
    

    Not clear at first how $visibility_conditions and $conditions differ. Probably deserves a comment?

11. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,241 @@
    +      $condition->summary();
    

    ?

12. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,241 @@
    +          'data' => ['#markup' => '<b>' . $condition->getPluginId() . '</b><br />' . $condition->summary()],
    

    This looks a little hardcoded

13. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,241 @@
    +    $new_form = $this->formBuilder->getForm('\Drupal\layout_builder\Form\ConfigureVisibility', $this->sectionStorage, $parameters['delta'], $parameters['uuid'], $parameters['plugin_id']);
    +    $new_form['#action'] = (new Url('layout_builder.add_visibility', $parameters))->toString();
    +    $new_form['actions']['submit']['#attached']['drupalSettings']['ajax'][$new_form['actions']['submit']['#id']]['url'] = new Url('layout_builder.add_visibility', $parameters, ['query' => [FormBuilderInterface::AJAX_FORM_REQUEST => TRUE]]);
    

    Wow. That is something. This definitely deserves some inline comments

14. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,241 @@
    +    $response->addCommand(new OpenOffCanvasDialogCommand($this->t('Configure Condition'), $new_form));
    

    Case: "Configure condition"

15. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,241 @@
    +    return $this->t('Configure visibility for the @block_label block', ['@block_label' => $block_label]);
    
    +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,271 @@
    +    return $this->t('Configure visibility for the @block_label block', ['@block_label' => $block_label]);
    

    Why are these the same?

16. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,271 @@
    + * @internal
    

    Here (and probably elsewhere) these should have a one-liner explaining why they are internal. Can copy/paste from other existing classes

17. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,271 @@
    +class ConfigureVisibility extends FormBase {
    

    Usually suffixed with Form

18. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,271 @@
    +  use LayoutRebuildTrait;
    +
    +
    +  /**
    

    Nit, extra blank line

19. +++ b/core/modules/layout_builder/tests/src/Unit/EventSubscriber/SectionComponentVisibilityTest.php
    @@ -0,0 +1,253 @@
    +  public function testGetSubscribedEvents() {
    +    $this->assertEquals([
    +      LayoutBuilderEvents::SECTION_COMPONENT_BUILD_RENDER_ARRAY => [
    +        'onBuildRender',
    +        255,
    +      ],
    +    ], SectionComponentVisibility::getSubscribedEvents());
    +  }
    

    This is overkill, IMO

20. +++ b/core/modules/layout_builder/tests/src/Unit/EventSubscriber/SectionComponentVisibilityTest.php
    @@ -0,0 +1,253 @@
    +    $event->inPreview()
    +      ->shouldBeCalled()
    +      ->willReturn(TRUE);
    

    When writing prophecy mocks, use either predictions (will) or spies (should). In this case, I'd remove the willReturn as we don't care about that value

21. +++ b/core/modules/layout_builder/tests/src/Unit/EventSubscriber/SectionComponentVisibilityTest.php
    index 6301eedc48..724c1f0e5f 100644
    --- a/core/themes/stable/css/layout_builder/layout-builder.css
    
    --- a/core/themes/stable/css/layout_builder/layout-builder.css
    +++ b/core/themes/stable/css/layout_builder/layout-builder.css
    

    Idk that we're allowed to change this now

#51.1

Should be _layout_builder_access: 'view' now

Done.
#51.2

Please document the magic number (aka indicate which other subscriber you are trying to run before/after)

Done.
#51.3

This can be a one-liner

Done.
#51.4

Feels wrong to reuse the $condition name for both. Maybe the first one from the loop should be $configuration?

Agreed - did some renaming.
#51.5

This should have an issue opened (unless it's intended to be solved in this issue?)

Decided to address it here as it seemed important and this will likley get it to users much faster than a followup issue. The current implementaion allows a single and/or operator, as opposed to something like Views where operators can be grouped in and/or groups. If Views-style operator grouping seems necessary, that may be best suited in a followup. To help mitigate user confusion, the current behavior of the operator field is dependent on how many visibility conditions are present. When adding the first condition, the operator field is not visible since its value does not matter on a single condition.

When adding a second condition, the operator field appears above the submit button.

Once 2+ contidions are present, the operator field appears above the conditions, and there is a dedicated submit for updating the operator so it can be changed without having to go to the configuration form for one of the existing conditions

I expanded the FunctionalJavascript tests to confirm switching the operator works.

#51.6

I don't understand this line, probably deserves a comment?

While stepping through the code to determine this line's purpose, I saw that the same call happens in an earlier subscriber BlockComponentRenderArray, so there shouldn't be a need for it.

#51.7

Hmmmm I'll have to think about this one

Left this unchanged - no preferable alternatives immediately came to mind but it certainly inspired a hmmmm on my end as well.

#51.8

This reminds me of the helpers like \Drupal\layout_builder\Form\ConfigureBlockFormBase::getCurrentComponent()

I added a trait for this

#51.9

once again with ?: []

Done

#51.10

Not clear at first how $visibility_conditions and $conditions differ. Probably deserves a comment?

Added comments and changed the variable names to be more self-explanatory.

#51.11 removed this, suspect it was just an artifact of an earlier iteration.

#51.12

This looks a little hardcoded

. Refactored into multiple render elements.

#51.13

Wow. That is something. This definitely deserves some inline comments

Added some comments but not entirely sure what everything there is doing @timplunkett will be following up on this. I did try refactoring to make this match the pattern of other two-dialog configs, but those (AFAIK) all use links to access the second dialog, and do not depend on any form field values from step 1.

#51.14 Changed cases here and on a few other buttons.

#51.15

Why are these the same?

It was an accurate description for both (one deals with configuring a specific rule, and the other configuring a group of rules), but I reworded to make it a little more form-specific.

#51.16 @internal is explained

#51.17 refactored class to suffix with form.

#51.18 removed blank line

#51.19

This is overkill, IMO

, agreed, this is indirectly covered in several other tests so I removed it.

#51.20

When writing prophecy mocks, use either predictions (will) or spies (should). In this case, I'd remove the willReturn as we don't care about that value

Did as recommended (and had to make a few other unit test changes to reflect other changes in this patch)

#51.21

Idk that we're allowed to change this now

Its not a critical CSS change so I removed it. There are a few other minor css changes that would be nice to add - is this something that could be added in a followup?

Also updated the forms to include the layout highlight ID so blocks are outlined when visibility conditions are being configured.

I've changed approach slightly in this patch. As noted in https://www.drupal.org/project/drupal/issues/3056907 ✨ Allow layout builder blocks to be optional depending on whether the required context exists Needs work , using an EventSubscriber is too late to avoid throwing MissingValueContextExceptions and it seems like conditions should be able to prevent that (if user X doesn't have the "staff" role then don't should the staff profile, if entity x doesn't exist then don't render it etc)

This change moves the condition evaluation directly into Section::toRenderArray() so that the component is never touched unless conditions pass.

I also changed the urls to include the {region} to match all the other routes (I also couldn't get the contextual links to appear unless I did this?)

The last submitted patch, 53: 2916876-53.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Neither issue cited above gives insight into how you've managed to achieve this particular bug. Also, I question your premise. Let me explain why:

• \Drupal\layout_builder\Section::toRenderArray() is responsible for getting the render array of all the blocks and regions representative of a given section.
• \Drupal\layout_builder\Section::toRenderArray() calls: \Drupal\layout_builder\SectionComponent::toRenderArray()
• \Drupal\layout_builder\SectionComponent::toRenderArray() dispatches the Event in question.
• Contexts are already provided to the event object which is capable of getting a block plugin via: \Drupal\layout_builder\Event\SectionComponentBuildRenderArrayEvent::getPlugin()
• The event's __construct() method delegates all the instantiation and context setting bits of the block to: \Drupal\layout_builder\SectionComponent::getPlugin()
• Event subscribers are free to get the plugin or configuration details and make use of it as necessary.
• \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray actually calls the build() method of the block. This event has a priority of 100.

With that flow in mind, the block plugin, with its context(s) (set properly or otherwise) exists before the render array is attempted to be generated. This fact means there are two potential solutions to the problem you outline above (though again, how you got there is beyond me since the UI strictly prevents the system from getting into this sort of state in the first place).

1. We can properly nuance the existing event subscribers to try/catch this themselves and return empty build arrays with the appropriate caching logic as necessary.
2. It might be possible to build an event subscriber which considers all the plugins under the section and checks their contextual status w/o invoking them. This sounds like new-api territory, so again without understanding how you got to this point I suspect this is a step further than is necessary.

If I've misinterpreted your issue, please elaborate further. I do not favor moving away from the EventSubscriber approach. It buys us too much.

Eclipse

Hi Eclipse and thanks for your input, hopefully I can answer both your questions.

Where the Exception Gets Thrown

So the order of calls listed is correct.

1. \Drupal\layout_builder\Section::toRenderArray() is responsible for getting the render array of all the blocks and regions representative of a given section.
2. \Drupal\layout_builder\Section::toRenderArray() calls: \Drupal\layout_builder\SectionComponent::toRenderArray()
3. \Drupal\layout_builder\SectionComponent::toRenderArray() dispatches the Event in question.
4. Contexts are already provided to the event object which is capable of getting a block plugin via: \Drupal\layout_builder\Event\SectionComponentBuildRenderArrayEvent::getPlugin()
5. The event's __construct() method delegates all the instantiation and context setting bits of the block to: \Drupal\layout_builder\SectionComponent::getPlugin()
6. Event subscribers are free to get the plugin or configuration details and make use of it as necessary.
7. \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray actually calls the build() method of the block. This event has a priority of 100.

The MissingValueContextExecption gets thrown in point 3. SectionComponent::toRenderArray() creates a new SectionComponentBuildRenderArrayEvent. SectionComponentBuildRenderArrayEvent::__construct() calls $component->getPlugin(), which calls ContextHandler::applyContextMapping() which throws MissingValueContextException. All before the event gets dispatched.

How the situation arises

I'm not sure whether this is possible to do in core, but it might be if you have a block that requires the node route context and then place it on a layout that doesn't have a node route context when its displayed. In my case, I'm using the patch in https://www.drupal.org/project/drupal/issues/3001188 ✨ Make it possible to add relationships to layout builder Needs work to add extra contexts to entity view displays, for example a particular profile related to a user account. If the profile doesn't exist for that particular user then the error gets thrown.

Ok great, so this is not a bug that you've managed to make happen w/o a core patch. That's good news.

Plugins don't require contexts FROM a specific place. They require contexts of a specific type (say, "node" for example). The config saves the expected key from the contexts array that was configured through the UI, so contexts that are available through the admin that cannot be calculated during the runtime of a page are unsafe. There's a fair amount of code elsewhere in core reenforcing that approach. The best thing to do for the patch in question would be to toss a try/catch around the instantiation of the plugin. If we catch, we can return something else and add higher priority event subscriber that logs the error, stops propagation and returns an empty array as the render of the block without invoking the rest of the event subscribers.

I've not looked at the patch in question, however one of the reasons we've not yet gone after "relationship" style contexts is precisely because we have to compensate for this situation in which the context is calculate-able in the administration, but doesn't actually exist during run time. I'd say it's the domain of that patch to fix. But the problem doesn't invalidate the current code path's approach. We know and understand that this is a real thing. We planned for it and have dealt with it similarly elsewhere.

@see:

\Drupal\block\BlockAccessControlHandler::checkAccess lines 92 & 122

It's not a perfect match in that case because we specifically delay context mapping until access checking in the block module approach so we can just return AccessResults in that case. In this case, our new higher priority event subscriber would need to return an empty build array, but it also needs to addCacheableDependency() for an AccessResult::forbidden() on the event itself. It all seems super achievable, but again I'd say it's outside the scope of this issue. However once this issue lands, ✨ Make it possible to add relationships to layout builder Needs work would need to do something similar for visibility plugins to try/catch their context mapping. Still it's 3001188's issue, not this one's.

Eclipse

I agree with @eclipsegc that the changes in #53 address something that should be addressed in ✨ Make it possible to add relationships to layout builder Needs work , so I added a patch that does this ✨ Make it possible to add relationships to layout builder Needs work .

This patch is built off #52 and made a change to the test to account for the test module layout_builder_test_css_transitions being removed since the transition-disabling functionality is now default in core.

#58 ✨ Add visibility control conditions to blocks within Layout Builder Needs work is working for me so far, RTBC +1, and THANK YOU ALL.

Will continue testing and building out some more content types with it, but so far so good.

Edit: I wanted to note that the UI looks like it could use some polish, but is fully functional and straightforward to use.

I've installed the patch from #58 and can see the configure visibility option on my block - but when I attempt to add a condition, after htting the Add condition button, I just get the indicator of something happening, but then nothing does.

It doesn't seem to matter which condition I choose.
I'm using layout builder to configure the /user page.

I tried this on another content type that uses layout builder and had the same result.
I am on core 8.7.8 - do I need to be on 8.8?

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Thanks to everyone who has committed time to this issue!

@jdearie - Yes, you'll want to test on 8.8.

I have tested patch #58 and identified the following issues:

Undefined index in BlockVisibilityForm

The following code in BlockVisibilityForm::successfulAjaxSubmit() results in undefined index notices in certain circumstances:

  if ($triggering_element['#submit'][0] === '::updateOperator') {
    return $this->rebuildAndClose($this->sectionStorage);
  }

Notice: Undefined index: #submit in Drupal\layout_builder\Form\BlockVisibilityForm->successfulAjaxSubmit() (line 267 of /app/web/core/modules/layout_builder/src/Form/BlockVisibilityForm.php)

Proposed change:

- if ($triggering_element['#submit'][0] === '::updateOperator') {
+ if (isset($triggering_element['#submit'][0]) && $triggering_element['#submit'][0] === '::updateOperator') {
    return $this->rebuildAndClose($this->sectionStorage);
  }

Add 'Back' button on nested form.

It would be useful to add a 'Back' button on ConfigureVisibilityForm to return to BlockVisibilityForm.

CSS hover behavior

When hovering over 'Configured Conditions' on BlockVisibilityForm, the off-canvas reset CSS needs overridden:

This is caused by '/core/themes/stable/css/core/dialog/off-canvas.base.css':

#drupal-off-canvas *, #drupal-off-canvas *:not(div) {
    color: #ddd;
    background: #444;
    font-family: "Lucida Grande", "Lucida Sans Unicode", "liberation sans", sans-serif;
}

It looks like the place to override is '/core/modules/layout_builder/css/layout-builder.css'.

Page reload on some actions

In some instances, actions result in a page reload instead of a dialog-based action:

BlockVisibilityForm

Update Operator - dialog closes
Add condition - ConfigureVisibilityForm opens in dialog
Edit link - ConfigureVisibilityForm opens in dialog
Delete link - DeleteVisibilityForm opens in dialog
Close button - dialog closes

DeleteVisibilityForm

Confirm - page reload (expected that dialog closes)
Cancel - PHP Error (see below - expected that dialog closes)
Close button - dialog closes

ConfigureVisibilityForm

Add condition - page reload (expected that dialog closes)
Update - dialog closes

DeleteVisibilityForm 'Cancel' results in PHP error

When clicking the 'Cancel' button on DeleteVisibilityForm, the following error is triggered:

TypeError: Argument 3 passed to Drupal\\layout_builder\\Form\\BlockVisibilityForm::buildForm() must implement interface Drupal\\layout_builder\\SectionStorageInterface or be null, string given in /app/web/core/modules/layout_builder/src/Form/BlockVisibilityForm.php on line 117

'Node Bundle' visibility condition

When would the 'Node Bundle' visibility condition be used? The block instance is already tied to a node via Layout Builder.

'Update Operator' case on BlockVisibilityForm

On BlockVisibilityForm, 'Update Operator' uses title case instead of the sentence case used elsewhere throughout the UI. Propose changing to sentence case: 'Update operator'.

Googlefood.

All seems to be working well from my tests. Question though (not to hijack this thread)... is there an ability to get the current users ID so one can control block visibility on conditions that match up with the current user.

For example on the request_path condition... would be nice to type in /user/[current-user:uid] so blocks will only show on the logged-in user's account pages (or would show for everyone but the logged-in user when clicking NEGATE).

Thanks again for the work on this.

P.S. - If the above isn't an immediate possibility, maybe this is a request for token support instead?

This is about providing a UI within Layout Builder for the existing Conditions.
This makes no changes to the existing conditions or their functionality.

That said, I think the functionality described in #64 would make for a great standalone feature request!

Context tokens are 100% a thing we should be eventually chasing down, but not a thing we really do today. They're especially useful for content blocks, but it's definitely a separate feature.

Eclipse

#58 is working for me too: RTBC +1

Nothing from #61 has been addressed. This will also need a change record before RTBC.

This addresses parts of #61:

1. The undefined index
2. The fatal error on the cancel link from the delete form

I was going to add a test for the fatal error (clicking the cancel link) but the existing test wasn't passing locally. That test should be added still though.

Fixes the test fails in #69

In case you need a version of this on top of #3045171: Form blocks rendered inside layout builder break save → there's this, there was a conflict in services.yml file

I haven't done a detailed line by line review but I wanted to check out the patch, this is really impressive work, nice one folks!

Just a few observations made while taking it for a spin:

1. +++ b/core/modules/layout_builder/layout_builder.links.contextual.yml
   @@ -27,3 +27,13 @@ layout_builder_block_remove:
   +layout_builder_block_visibility:
   +  title: 'Control visibility'
   +  route_name: 'layout_builder.visibility'
   +  group: 'layout_builder_block'
   +  options:
   +    attributes:
   ...
   +      data-dialog-type: dialog
   +      data-dialog-renderer: off_canvas
   

   Testing this was kind of confusing, because all your contextual links get cached in the browser. New blocks got the links and old ones don't.

   Looks like this might be worked on in 🐛 New contextual links are not available after a module is installed Needs work though.

2. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,358 @@
   +    foreach ($this->conditionManager->getDefinitionsForContexts($this->getAvailableContexts($section_storage)) as $plugin_id => $definition) {
   +      $conditions_available_to_block[$plugin_id] = $definition['label'];
   +    }
   

   The condition manager already implements FilteredPluginManagerInterface, does it make sense to use getFilteredDefinitions here instead, so contribs like layout_builder_restrictions can choose to provide users a way of restricting this list.

   Also as a dev, if I decided I didn't want admins adding visibility conditions, could I use the same hook to NULL the list out and expect to see the contextual link disappear? This is perhaps a bit too much for a single issue, but perhaps worth a follow-up.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockVisibilityTest.php
   @@ -0,0 +1,315 @@
   +    $this->clickContextualLink(static::BODY_FIELDBLOCK_SELECTOR, 'Control visibility');
   

   Is "Control" the right label? Seems to use a mixture of "Control" and "Configure":

Drupal 8.9.0-beta1 → was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Updated patch attached. It addresses the following:

• #61 - Add 'Back' button on nested form
• #61 - Page reload on some actions
• #61 - 'Update Operator' case on BlockVisibilityForm
• #71.2 getDefinitionsForContexts() replaced by getFilteredDefinitions()

It doesn't address:

• #61 - CSS hover behavior (appears to need fixed in Stable, not here)
• #72.3 - Control vs configure (need a consensus)

Seems to be broken by service change in #3045171: Form blocks rendered inside layout builder break save → . Rerolled against 9.1.x though the same patch seems to apply to 8.9.x just fine.

The last submitted patch, 75: 2916876-75.patch, failed testing. View results →

@neclimdul - Can you post an interdiff from #73?

Please find the interdiff file for #75 here.

Fixing test case failure issues in #75, please review.

The last submitted patch, 80: 2916876-80.patch, failed testing. View results →

Please ignore patch #80, I have fixed test case failure issue for patch #75 here, please review.

The last submitted patch, 83: 2916876-83.patch, failed testing. View results →

Yeah, generally you don't provide a interdiff when there's no change and you're just resolving a chunk collision like that. Such an interdiff is hard to generate and generally not helpful and confusing so you tell people you're just rerolling.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockVisibilityTest.php
@@ -136,7 +136,7 @@
-        $this->assertElementPresent($block['rendered_block_selector']);
+        $this->assertSession()->fieldExists($block['rendered_block_selector']);

That does not seem to be equivalent. fieldExists claims to need an id|name|value and is failing despite the page containing a div with the class.

This passes and matches the deprecation suggestion for assertElementPresent

$this->assertSession()->elementExists('css', $block['rendered_block_selector']);

sorry, didn't mean to change the status... don't know how that happened.

1. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -220,3 +220,7 @@
   +#drupal-off-canvas #configured-conditions {
   

   No need to use a Id as selector. Use a class instead.

2. +++ b/core/modules/layout_builder/layout_builder.routing.yml
   @@ -145,3 +145,42 @@ layout_builder.move_block:
   +layout_builder.delete_visibility:
   

   For blocks we use "Remove block" - do we want to be consistent with the verb here?

3. +++ b/core/modules/layout_builder/layout_builder.services.yml
   @@ -44,6 +44,11 @@ services:
   +  component_visibility_subscriber:
   

   Should we prefix here with "layout_builder", similar to the other defined subscriber in this module?

4. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
   @@ -0,0 +1,86 @@
   + * Determines component visibility.
   

   s/component/section component

1. +++ b/core/modules/layout_builder/layout_builder.routing.yml
   @@ -145,3 +145,42 @@ layout_builder.move_block:
   +layout_builder.add_visibility:
   
   +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,359 @@
   +            '#url' => Url::fromRoute('layout_builder.add_visibility', $this->getParameters($visibility_id), $options),
   

   Should this route be called "add_visibility" if it used for editing a existing one?

2. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,359 @@
   + * Provides a form for applying visibility conditions to a block.
   

   We're mixing block and section component. Should we be consistent and use one of them?

3. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,359 @@
   +              '#tag' => 'b',
   

   Should this be a heading element instead of bold?

4. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,359 @@
   +        '#empty' => $this->t('No required conditions have been configured.'),
   

   When is this ever displayed?

Working on it

It looks like #72.3 still need addressed - "configure" vs. "control".

Updated patch attached.

The following changes have been made:

#85 - Assert is fixed.
#87.1 - Class used instead of ID
#87.3 - Service name is now prefixed with 'layout_builder.'
#88.4 - Based on the logic, that message will never appear. It's been removed.

The following changes have not been made:

#87.2 - Language remains unchanged pending additional community feedback. The use of "delete" is used for more than just the route name, it's used for the path, for the class name \Drupal\layout_builder\Form\DeleteVisibilityForm, for a form ID, for messages, etc.
#87.4 - I don't understand the proposed change.
#88.1 - I don't see a problem with the route name. We're adding a visibility condition regardless if there's an existing one or not.
#88.2 - My understanding is that blocks become Layout Builder components when they're placed in a layout. As such, I agree; however, I'll let others weigh in at this point. We would need to rename the \Drupal\layout_builder\Form\BlockVisibilityForm class to ComponentVisibilityForm.
#88.3 - I don't think a heading element would be semantically correct here. We're working with a table, so, if anything, I'd think that the condition type and its description should be separate columns with a header row. WAVE is upset that there are no <hr> cells. I'm leaving this one open for more discussion.

Hi, could the visibility be also set on the whole layout section other than on the single block?

Example usecase: I've created a page with a custom layout but for some days I want to hide a section. I dont't want to delete the section because in a couple of days it will be re-added.

Should I open a separate issue with this feature request or could be considered/merged on this thread?

Thank you :-)

@FiNeX, you can add the same visibility rule to each of the blocks in a section. If neither from the blocks in that section are rendered the whole section won't rendered be as well.

Generally speaking having visibility rules per section will be handy for sure.

Tried patch from #92, and preliminary tests worked great.

Test Case:

Layout Override (homepage):
- add a custom block (a custom copyright block in this case)
- add language visibility, only show for English
- works great

Layout Override (homepage):
- add a reusable content block
- add language visibility, only show for English
- works great

Layout Override (homepage):
- add another instance of the above reusable content block in another section
- add language visibility, only show for Dutch
- still works great

So those use cases seem to work at least.

woops

@FiNeX

We'd need the layout section to support such a thing. Definitely a separate issue from this one. It needs its own acceptance criteria and evaluation of whether or not we want to actually do it.

Drupal 9.1.0-alpha1 → will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule → and the Allowed changes during the Drupal 9 release cycle → .

ok, thanks for the feedback @eclipsegc

Patch has failing test on 9.2.

This is just a code review, I have not clicked through it yet.

1. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
   @@ -0,0 +1,86 @@
   +    // Priority is set to 255 so this subscriber is run after the one in
   +    // BlockComponentRenderArray.
   +    // @see \Drupal\layout_builder\EventSubscriber::getSubscribedEvents().
   +    $events[LayoutBuilderEvents::SECTION_COMPONENT_BUILD_RENDER_ARRAY] = ['onBuildRender', 255];
   ...
   +    if ($conditions && !$this->resolveConditions($conditions, $visibility_operator)) {
   +      // If conditions do not resolve, do not process other subscribers.
   +      $event->stopPropagation();
   +    }
   

   This doesn't make sense to me. Why would we stop propagation if it's running *after* the one that does the rendering?

   Also it's redundant to say that this one is 255, that's in the code on the next line. Say instead what the value is in the other subscriber (if needed).

   Finally, @see cannot be parsed in inline comments. Just make it a sentence (See \Drupal...)

   And actually even more finally, that fully-qualified class name is missing the class name.

2. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
   @@ -0,0 +1,86 @@
   +    $conditions = [];
   +    if (!$event->inPreview()) {
   ...
   +
   +    if ($conditions && !$this->resolveConditions($conditions, $visibility_operator)) {
   

   This $conditions check is doing some heavy lifting. It's both guarding against the inPreview check, as well as the emptiness of $visibility.

   Instead, swap the inPreview check to be a true guard condition. if ($event->inPreview()) { return; }

3. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,358 @@
   +              '#type' => 'html_tag',
   +              '#tag' => 'b',
   

   This isn't something we should be doing.

4. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,358 @@
   +        '#prefix' => '<div class="configured-conditions">',
   +        '#suffix' => '</div>',
   +        '#theme' => 'table',
   

   If you use #type => table you should be able to pass #attributes instead of hardcoding this.

5. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,358 @@
   +      '#weight' => count($items) === 1 ? 30 : 3,
   

   Even with the above comment, I don't understand the choices of 3 and 30 here.

6. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,358 @@
   +      $form['actions']['submit']['#ajax']['event'] = 'click';
   ...
   +      $form['update_operator']['#ajax']['event'] = 'click';
   

   I don't think these are needed?

7. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,358 @@
   +    $new_form = $this->formBuilder->getForm('\Drupal\layout_builder\Form\ConfigureVisibilityForm', $this->sectionStorage, $parameters['delta'], $parameters['uuid'], $parameters['plugin_id']);
   

   nit: use ConfigureVisibilityForm::class

8. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,358 @@
   +    $form_state->setRedirectUrl($this->sectionStorage->getLayoutBuilderUrl());
   ...
   +    $response = new RedirectResponse($url->toString());
   +    $form_state->setResponse($response);
   

   Why the difference?

9. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
   @@ -0,0 +1,358 @@
   +   *   The id of the visibility plugin.
   ...
   +   *   List of Url parameters.
   

   nit: ID, URL

10. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,358 @@
    +  public function title(SectionStorageInterface $section_storage, $delta, $uuid) {
    +    $block_label = $section_storage
    +      ->getSection($delta)
    +      ->getComponent($uuid)
    +      ->getPlugin()
    +      ->label();
    
    +++ b/core/modules/layout_builder/src/Form/ConfigureVisibilityForm.php
    @@ -0,0 +1,320 @@
    +  public function title(SectionStorageInterface $section_storage, $delta, $uuid) {
    +    $block_label = $section_storage
    +      ->getSection($delta)
    +      ->getComponent($uuid)
    +      ->getPlugin()
    +      ->label();
    

    Looks familiar :) Any other usages of these that could be refactored to some shared code?

11. +++ b/core/modules/layout_builder/src/SectionComponentTrait.php
    @@ -0,0 +1,51 @@
    +  public function getCurrentSection() {
    ...
    +  public function getCurrentComponent() {
    

    I think this also lives in ConfigureBlockFormBase. Worth a refactor (or follow-up?)

12. +++ b/core/modules/layout_builder/tests/src/Unit/EventSubscriber/SectionComponentVisibilityTest.php
    @@ -0,0 +1,242 @@
    +      ->shouldBeCalledTimes(1)
    

    Is it important that it only be called once? If not, don't test that. This applies to all the other usage of it too.

    When in doubt, don't use shouldBeCalled and willReturn on the same method

13. +++ b/core/modules/layout_builder/tests/src/Unit/EventSubscriber/SectionComponentVisibilityTest.php
    @@ -0,0 +1,242 @@
    +    $event->inPreview()
    +      ->willReturn(FALSE);
    

    I didn't read all the rest coverage, but can you please remove these extraneous return/indents? It just makes it that much harder to read

Adresses #101 partially, hoping to get this going again!

1. That docblock is misleading (wrong), SectionComponentVisibility runs before BlockComponentRenderArray and that's why we stop propagation. I updated the comment.

2. Added true guard condition.

3. Not sure what we should do instead.

5. I changed 3 to 0. I added the weights in an attempt to see if it's more clear, not sure.

6. Removed.

7. Fixed.

9. Fixed.

Didn't have time to address everything now unfortunately...

Leaving at NW intentionally.

Adresses #101

4. That suggested change breaks the design. Leaving as is, given that we still need some design work to be done here.

8. Fixed.

10. Follow up?

11. I think follow up is fine.

12. Don't think so. Changed. Fixed double use of shouldBeCalled/willReturn.

13. Fixed.

Note to self: Remember to Run code check :)

3.

+++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
@@ -0,0 +1,358 @@
+              '#type' => 'html_tag',
+              '#tag' => 'b',

This isn't something we should be doing.

3. Not sure what we should do instead.

The comment may refer to use of the b tag, generally discouraged in HTML5 (see for example the article "Using <b> and <i> elements"). strong?

I mean both the b as well as the html_tag.

I would replace that whole section with
'label' => $this->t('<strong>@condition_name:</strong> @condition_summary', ['@condition_name' => $condition->getPluginId(), '@condition_summary' => $condition->summary()]),

And even consider replacing $condition->getPluginId() with $condition->getPluginDefinition()['label'] or similar.

I've been testing this a bit and it is looking good so far.

Other "User interface changes" that might be interesting to consider could include giving the user (in the preview) a hint that visibility conditions have been applied. I'm not quite sure what that would look like, but thought it might be worth passing along.

Adresses #106 and fixes cspell...

The last submitted patch, 108: 2916876-108.patch, failed testing. View results →

Last nit, which is also the failing test:

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockVisibilityTest.php
@@ -0,0 +1,315 @@
+  public static $modules = [

protected static

That said, this is still tagged needs design

Can someone record a quick walkthrough of the proposed UI, so it can be validated?

I have made the suggested changes. Which can help to turn the patch green to go. I will try to create a walkthrough for the proposed UI changes.

We applied the patch (#92) to a D8 for a project and it worked extremely well. We could use it with no functional changes. We had some UI issues, but I think most of these related to layout builder UI customization we have. Thank you to everyone who has contributed.

The issue fork is empty, @tcrawford were you planning on pushing anything up?

Drupal 9.2.0-alpha1 → will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

@tim.plunket Thank you for your feedback. I apologize .The issue fork was created unintentionally. I only meant to leave a comment to provide our (positive) feedback on the patch. Thank you again.

Confirmed that the interdiff in #111 covers the feedback from #110. Thanks!

Remaining item from #110:

Can someone record a quick walkthrough of the proposed UI, so it can be validated?

The patch file in #111 not applying to 8.9.15, which was released today.

I wish I had time to re-roll this today, sorry!

That makes sense. #111 targets the latest version of Drupal and contains changes to a file added in 9.1.x (cspell/dictionary.txt). You'd need a patch prior to #108 to apply it to earlier versions of Drupal.

Just tried #111 on 9.1.8 and it works great on content type layouts (/admin/structure/types/manage/page/display/default/layout) but the link doesn't appear at all on node layout overrides (node/xxx/layout)

Patch #111 seems to need a reroll for 9.3.x and 9.2.x plus my problem in #120 (link not appearing at all)

9.2.0 using patch #111 fails on dictionary.txt because terms have been added. This seems like a moving target since terms seem to be added regularly. I added 'fieldnid' manually.

fieldnames
+ fieldnid
fieldsets

After that change, this patch does show the "Control visibility" link on blocks at node/xxx/layout for us. Not sure what's stopping it from working for @Anybody.

@darkodev thank you very much for your confirmation. Then it seems to be a problem only in my case. It's a simple blog using Olivero with Drupal 9.2.0. I'll have a deeper look asap. So don't consider my comment as blocker, then. The functionality is important.

Here's a reroll of #111, fixing the issue in dictionary.txt.

Fixed the #124 fails.

Thanks for the patch!
We have the following issue on a multilingual site.
https://drupal.stackexchange.com/questions/304056/how-to-add-language-pr...

In short:
when default language negotiation and source language differ we get a 403 response.
Because the action link does not have a language prefix.
So the solution for us is to hook into that:

/**
 * Implements hook_preprocess_HOOK().
 */
function freitag_layout_visibility_control_preprocess_form(&$variables) {      
  if (isset($variables['attributes']['id']) && $variables['attributes']['id'] === 'layout-builder-configure-visibility') {
    $langcode = \Drupal::languageManager()->getCurrentLanguage()->getId();
    $variables['attributes']['action'] = '/' . $langcode . $variables['attributes']['action'];
  }
}

Also patch 125 is not compatible with 8.16.0

Have a nice day and best regards

Change to needs work for the language context issue.

Ignore this patch, it's 111's patch but with modal instead of off canvas. Speaking of, any chance this patch can be created so people can alter if it's off canvas or modal, etc. We're using https://www.drupal.org/project/layout_builder_modal → with some extras in hook_link_alter, and it's a fairly popular module.

A re-rolled patch against 9.3.x. Please review.

@Suresh Prabhu Parkala re #131 there's no need for a re-roll against 9.3.x, there's a patch applying successfully against 9.3 in #125 . You were making changes to patch #128, which explicitly said ignore this patch, so there's no need to reroll.

To mitigate this confusion, I've changed this to a merge request based on #125.

Re #128 @Grayle

Speaking of, any chance this patch can be created so people can alter if it's off canvas or modal, etc

This issue is about Layout Builder block visibility conditions - incorporating such a change here would be out of scope, but you could create a new issue requesting this feature. If your primary interest in having these forms as modal is the available width, you can also drag the off-canvas dialogs to resize them wider (although this does have to be done every time they open, it wouldn't change the default width).

It looks like current approach doesn't allow a condition plugin to use AJAX on its configuration form. Just because the form is rendered on a route where it doesn't exist here. And it's also the reason why its #action has to be rewritten here.

An alternative would be to list condition types as AJAX-enabled URLs instead. This way there must be no issue with AJAX on the condition settings form.

Submitted an alternative MR !1148. The issue it resolves is described in #134.

Another option is to make a sub-request to the form route. This way AJAX should also work on the condition form.

I think another very useful set of functionality here would be for empty vs non-empty node fields.

ie. If node_type:field_a is empty, do not render this block, and if node_type:field_a is !empty, do render this block.

I believe #137 can be accomplished with Entity Field Condition → .

I agree and think #137 is off-topic. @ZhuRenTongKu please create a separate issue for that, if you think it's helpful and Entity Field Condition → can't solve it. Let's focus on this issue.

@justcaldwell good call, I'll check that out, thanks!

@Anybody, I'll check out Entity Field Condition, seems like it may solve my issue. I did write some code that allows me to do the behavior I mentioned, for both sections and blocks. However, it's a wee bit crude as it only allows for one condition per section/block, naturally as soon as I wrote it I came across the need to have multiple conditions on the same section/block!

@danflanagan8, Thanks for the plug on the Boolean module, perhaps there's an opportunity to collaborate on it!

Adding a diff file here for a stable reference from composer files. It was made from MR !1148 at commit 972afa7c.

The last submitted patch, 141: drupal-layout-builder-visibility-conditions-2916876-141.diff, failed testing. View results →

We noticed an issue that the condition operator is not transfered from the BlockVisibilityForm to the ConfigureVisibilityForm.
We could add an optional parameter to the layout_builder.add_visibility route with "and" as default.
I tried this and this seems to work.

This patch only includes the missing operator parameter.

@justcaldwell, @Anybody, @ZhuRenTongKu
Entity Field Condition would be a very good solution, but it seems to cause an AJAX error when used with patches in this thread. I created a feature request https://www.drupal.org/project/entity_field_condition/issues/3248365 ✨ Core Layout Builder Compatibility - AJAX Error Active

Re-roll of #144 ✨ Add visibility control conditions to blocks within Layout Builder Needs work against 9.3.x branch.

Drupal 9.3.0-rc1 → was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

We've used #144 recently with a custom condition plugin. Haven't tried #146 yet, but the general approach makes sense and seems to work for us.

Thanks.

I agree with @darkodev that block visibility based on field values would seem like a very common use case; but as pointed out it fails due to it's ajax loading of fields. Most likely that is tied into this feature not being able to currently support that.

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Found this 3 patch set from here: https://www.drupal.org/project/entity_field_condition/issues/3091898#com... ✨ Generic Entity Field Value Plugin Needs review (comment #20) to work to provide field value based block visibility control within LB.

Hey all, I was running into migration issues with Panelizer and ran across this issue. This visibility is highly needed for Layout Builder to be a Panelizer replacement in D9 for migrations. How close are we on this current issue?

Confirming this works. One caveat, for existing layouts I don't get the "Control visibility" in block context menus. It's only if I move them to a new section that the menu item is added below "Remove block".

Another obvious problem is that layouts using this patch will break if the patch is removed - e.g.

Error: Class "Drupal\layout_builder\EventSubscriber\SectionComponentVisibility" not found in Drupal\Component\DependencyInjection\Container->createService()

Just leaving this here for future reference.

One caveat, for existing layouts I don't get the "Control visibility" in block context menus. It's only if I move them to a new section that the menu item is added below "Remove block".

I can't confirm this. For me the visibility control also appears for blocks that are inside sections that were added before the patch was applied.

I can confirm the caveat in #155, "Control visibility" doesn't show up for me on a page where blocks were already placed until I move the block to a new section or remove and re-add the block. However, it did work on another page where there was only one section present.

Drupal 9.5.0-beta2 → and Drupal 10.0.0-beta2 → were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Re new contextual links not displaying for existing content, this is a known core bug: 🐛 New contextual links are not available after a module is installed Needs work .

The patch didn't work for me
I add block from block layout and hide it from the home page when i edit the layout builder the block appears on the backend

+1 #154, subscribe

Currently working on 2 versions of the same site d9.4.8, 1 with Panels, 1 with Layout Builder.

I've fixed the MR https://git.drupalcode.org/project/drupal/-/merge_requests/2537
Also attaching the generated patch for Drupal 9.5 here.

Here's a re-roll of #164 that applies to Drupal 10.1.x; I did not attempt to fix the failing tests.

Patch #165 applied on Drupal 10.0.3 but clicking on 'Control visibility' does nothing but log this error,

Error: Call to undefined method Drupal\layout_builder\Form\BlockVisibilityForm::getAvailableContexts() in Drupal\layout_builder\Form\BlockVisibilityForm->buildForm() (line 124 of /home/project-root/public_html/core/modules/layout_builder/src/Form/BlockVisibilityForm.php).

Looks like the error above is due to the change in #3099968: Layout builder incorrectly resolves global contexts values when viewing layouts → .

The attached patch resolves that. This functionally works in D10, though I haven't addressed any of the test issues.

Yippee! Patch #167 is working. Thank you @mark_fullmer

Trying to fix the failures.

#158 suggests that a patch allows the contextual link for this work to show on existing blocks. It does not fix it for me.

I just stumbled across this thread. I wound up creating a module before I saw this (I deal with the Context module a lot more):

https://www.drupal.org/project/layout_builder_context →

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

I'll try to arrange current state of issue.

As per #126, we don't have a translation for LB in the core, I tried the asymmetric translation contribute module, and even with this it works as excepted.

One caveat, for existing layouts I don't get the "Control visibility" in block context menus. It's only if I move them to a new section that the menu item is added below "Remove block".

#155 - This one was already fixed once for removing, and updating context menus. All we need to do is to add another operations key here

file \Drupal\layout_builder\Element\LayoutBuilder::buildAdministrativeSection
...
              // Add metadata about the current operations available in
              // contextual links. This will invalidate the client-side cache of
              // links that were cached before the 'move' link was added.
              // @see layout_builder.links.contextual.yml
              'metadata' => [
                'operations' => 'move:update:remove:VISIBLITY', <--------------- HERE WE SHOULD ADD ANOTHER KEY
              ],
...

Merge request !3627 is missing Ajax fixes from this PR https://www.drupal.org/project/drupal/issues/2916876#mr1148-note42005 ✨ Add visibility control conditions to blocks within Layout Builder Needs work
Same thing for 2916876-167.10x.patch it does not contain Ajax fixes from MR.

Apart from that, I don't know what are blockers here.

Could someone clarify why we cannot move this issue forward?

Sorry folks, I can't tell. Are any of these patches working for 9.4 ?

Sorry! Did not mean to switch version numbers.

Tests fixed ... so Let's review to see what's "left befind".

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

it seems the MR for 10.1.x has conflicts with 11.x

Let's try with a patch file for 11.x

But none of my points from #176 were fixed

1.

Merge request !3627 is missing Ajax fixes from this PR https://www.drupal.org/project/drupal/issues/2916876#mr1148-note42005 ✨ Add visibility control conditions to blocks within Layout Builder Needs work
Same thing for 2916876-167.10x.patch it does not contain Ajax fixes from MR.

As I can see there is still

+    // @todo The changes to #action/actions need to be documented or refactored
+    //   to better resemble other dual-dialog forms in Layout Builder.
...

2.
As I mentioned in #176 we need to add operations so the client side will clear the cache for contextual links.

# 155 ✨ Add visibility control conditions to blocks within Layout Builder Needs work - This one was already fixed once for removing, and updating context menus. All we need to do is to add another operations key here

@dinazaur
i don't think the link is correct
https://www.drupal.org/project/drupal/issues/2916876#mr1148-note42005 ✨ Add visibility control conditions to blocks within Layout Builder Needs work

could you provide the right one?
maybe commit link or some other gitlab change link/url ...

milos.kroulik → made their first commit to this issue’s fork.

let's "regain" some attention. as the last questions didn't get some answers.

Hello @vasilke

could you provide the right one?
maybe commit link or some other gitlab change link/url ...

1148 PR uses LayoutRebuildTrait::rebuildAndClose method, instead your PR contains 'workaround' that is not needed at all.

And still, my second point from #182 was ignored.

@dinazaur

(previously) i only took the latest MR and made it "green".
I wasn't aware that a previous MR has some "extra fixes".

(Now) I updated the MR including the extra commits from !1148 MR

But those changes actually changed the UX/UI for this feature, so the tests were broken.
So i updated the tests ... to pass, but without changing what's tested.

Still not sure about the solution, as there were a lot of patches and MRs, but not really. a plan. maybe a "core maintainer" could lead things here.

But for now i think we're good to review again - temporary.

p.s. also added "Needs issue summary update" tag. as i think we need to have clear info about what are we're trying to achieve.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Let's try with a patch file for 11.x with latest MR updates

Moving to NW for change record and issue summary update mainly.

But applied the patch and may see a bug. For sites already with layout builder and blocks placed the visibility contextual link does not appear.
Cleared cache numerous times but no luck.

If I readd an existing block then it appears.

Following the #146 patch, #167 breaks behaviour such that the context is no longer passed through to subforms.

-    $form_state->setTemporaryValue('gathered_contexts', $this->getAvailableContexts($section_storage));
+    $form_state->setTemporaryValue('gathered_contexts', $this->contextRepository->getAvailableContexts($section_storage));

This is related to the https://www.drupal.org/node/3195121 → change record, and the new code should be making a call to $this->getPopulatedContexts($section_storage) rather than $this->contextRepository->getAvailableContexts($section_storage).

Patch #192, no longer applies, attached patch for D11

Sorry folks, I can't tell... Is this no longer going to be planned to implement on D9? I see. mainly patches for D10 - D11... It's quite a long thread so I 'm not sure which patch I can implement...

Someone may post a patch but D9 is EOL so no more releases.

I tried 194 on 10.1.6 core-recommended yesterday but it didn't apply. Hopefully once it passes for 11 then one for 10 will be made.

@crutch - just a small point of clarification: 11.x is essentially the current main branch all development. Since the current cycle is still targeting 10.x releases, those get created from the 11.x branch.

https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... →

Is it just this patch required for this? Read (way) above that there might be a context patch also required (but if that was the case it would likely be mentioned in the OP).

I have D10.0. The patch from #192 is the most recent which applies. The functionality is there so far as the Control Visibility contextual link is there. Opening it (now) gives me a list of plugin options (it used to be a selector). Selecting Node Field and i get a constantly spinning throbber with what looks like the right form:
- node type (it doesnt know i am on LB for a specific bundle)
- Field
- value source
- etc.

When i pick my node type, the throbber stops but the Field field disappears.

Is it possible this part is simply not supported by core (which would be silly of course as fields are core)? I was previously using a patch to the Entity Field Condition module (i suspect in conjunction with the work here) and that patch is no longer compatible with D10; so i removed it thinking possibly covered by the work here.

#192 and the Entity Field Condition module with this patch work for me on D10.1.6: https://www.drupal.org/project/entity_field_condition/issues/3091898#com... ✨ Generic Entity Field Value Plugin Needs review

1. +++ b/core/misc/cspell/dictionary.txt
   @@ -313,6 +313,7 @@ druplicon
   +eacute
   

   This isn't in the patch? Why is it getting added?

2. +++ b/core/modules/layout_builder/src/EventSubscriber/SectionComponentVisibility.php
   @@ -0,0 +1,88 @@
   +      // If conditions do not resolve, do not process other subscribers.
   +      $event->stopPropagation();
   

   This relies _very_ heavily on the render order. Is that a problem? What happens when modules mess with the render order? Can we make this more resilient or move it down a layer?

- UI is ugly? Don't know if this is something with my admin theme but all the components are on the off canvas overlay but they're hard to understand and interact with.
- Adding visibility closes the entire off canvas area instead of returning to visibility control. This makes it quite awkward to make multiple changes or even confirm your changes where done correctly.
- Sometimes we say control visibility, sometimes we say configure visibility.

@vasike earlier you mentioned questions not getting answered. Are those still relevant? Can we recapture any outstanding issues in the IS?

Fixed: undefined method issue
Error: Call to undefined method Drupal\layout_builder\Form\BlockVisibilityForm::getAvailableContexts() in Drupal\layout_builder\Form\BlockVisibilityForm->buildForm() (line 125 of /var/www/html/docroot/core/modules/layout_builder/src/Form/BlockVisibilityForm.php).

Fixed: undefined method issue
Error: Call to undefined method Drupal\layout_builder\Form\BlockVisibilityForm::getAvailableContexts() in Drupal\layout_builder\Form\BlockVisibilityForm->buildForm() (line 125 of /core/modules/layout_builder/src/Form/BlockVisibilityForm.php).

Fixed: undefined method issue
Error: Call to undefined method Drupal\layout_builder\Form\BlockVisibilityForm::getAvailableContexts() in Drupal\layout_builder\Form\BlockVisibilityForm->buildForm() (line 125 of /core/modules/layout_builder/src/Form/BlockVisibilityForm.php)

Thanks for everyone's work on this, I've been following this thread since starting work on D7 > D10 in 2021. Visibility can be controlled in other ways, should it be major? Though, it would be nice to have this for core blocks and it was the first thing I looked for when moving to layout builder. It was the way to do it in D7 with panels so it was natural to look for this functionality.

We went live with a few sites on D10 July 1, 2023 without it. For core blocks like Body, we recreated them as a View block and then we have control. There are a few blocks where we are using BVG → . I understand the need though, easier and more intuitive to have the functionality within.

Now that we aren't using or needing it, having visibility control conditions for blocks within layout builder may be something that could be disabled as a setting or that could be a separate module?

Patch #205 didn't apply cleanly to 11.x, so this is a reroll.

I just updated to 10.2. We have layouts where we controlled visibility using a patch (" https://www.drupal.org/files/issues/2023-03-10/2916876-167.10x.patch → "). This patch doesn't work with Drupal 10.2 and impacts display in a profound way on a large site.

Are any of the recent patches compatible with 10.2?

Should be converted to an MR.

FYI 11.x is the current development branch.

Moving to NW for CR.

joseph.olstad → made their first commit to this issue’s fork.

@smustgrave, the MR is trying to merge to 10.1.x, should be a merge request for 11.x
with that said, I took what I could from 207 and brought it into MR 3627

The MR is targeting 10.1.x. I don't know how to rebase it for 11.x. Can someone do that, please?

Also, #207 does still apply cleanly.

d70rr3s → changed the visibility of the branch 11.x to hidden.

d70rr3s → changed the visibility of the branch 9.2.x to hidden.

d70rr3s → changed the visibility of the branch 10.1.x to hidden.

Rebased from 11.x into 2916876-11.x, but not getting the MR button on the issue page

@d70rr3s make sure you are logged in on GitLab. Sometimes when you access there it is done as anonymous and therefore you don't see the MR button.

I hope it helps.

It says the MR contains no changes.

Sorry, for some reason I read the git log wrong and thought I pushed the new branch.

Patch #207 has the warning for deprecated code: Deprecated function: Creation of dynamic property Drupal\layout_builder\Form\ConfigureVisibilityForm::$classResolver is deprecated in Drupal\layout_builder\Form\ConfigureVisibilityForm->__construct() (line 123 of /code/web/core/modules/layout_builder/src/Form/ConfigureVisibilityForm.php)
https://www.drupal.org/node/3428661 →

patch #233 left out new files