Add visibility control conditions to blocks within Layout Builder

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

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?

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.

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.

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.

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.

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.

*sigh*

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 #2773591: New contextual links are not available after a module is installed

   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.

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, 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?)

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 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.

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

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 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?

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!

#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 #2773591: New contextual links are not available after a module is installed 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":

   

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.

@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.

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

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

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...

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?

@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.

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

Re-roll of #144 against 9.3.x branch.

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.

Found this 3 patch set from here: https://www.drupal.org/project/entity_field_condition/issues/3091898#com... (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.

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

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 for this sort of thing):

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