Remove contextual links not related to layout administration inside layout builder blocks

This didn't seem to work completely because the button and links would still should up.

So here is another try at CSS solution but I don't think we will be able to fix on this level.

Because:

1. The contextual links will still be on the page so I think the contextual module message will still factor the into the to the message "Tabbing is constrained..."
2. When going into edit mode the contextual button would receive display property directly on the element which will override our CSS.

In chat Sam suggested that instead of hiding the ones we don't care about, that we always show all the ones we DO care about.
Sorta like if they clicked the "Edit" link in toolbar, but just for LB. Almost like a toolbar mode would do...

I think the problem with #4 is that we would still have to solve the problem of keyboard navigation. Someone using the keyboard would still tab through them I think.

Also the contextual Drupal.announce message would be wrong about the number of contextual links unless we changed that.

Here is patch that uses the JS event drupalContextualLinkAdded to remove non layout builder contextual links that are inside of an element with [data-layout-block-uuid]

whoops.

Also adding test only patch to prove the test would fail

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -51,4 +51,30 @@
   +      data.$el.attr('data-contextual-id') &&
   +      !data.$el.attr('data-contextual-id').startsWith('layout_builder_block:')
   ...
   +        data.$el.remove();
   

   Let's store data.$el and data.$el.attr('') in local vars.

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -51,4 +51,30 @@
   +      if (data.$el.parents('[data-layout-block-uuid]').length > 0) {
   

   I think we prefer .closest() instead of .parents().

@jibran thanks for the review!
1. fixed
2. closest() will pick up the element itself. We could change to use it and change to > 1 but this seems more readable. Is there reason we should switch?

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -51,4 +51,30 @@
+    /**
+     * Bind a listener to all 'Quick edit' links for blocks. Click "Edit"
+     * button in toolbar to force Contextual Edit which starts Settings Tray
+     * edit mode also.
+     */

This comment is copied will create a new 1.

See #1400310: Use of .parents() is really .closest() for #10.2.

1. #11 ok thanks for link.
   Changing to closest.
2. also updated the test to make the layout builder's own contextual links are not removed along with the ones that should be removed.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -0,0 +1,108 @@
   +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', "#drupal-off-canvas a:contains('Test Block View: Teaser block')"));
   ...
   +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', "[data-drupal-selector='edit-actions-submit']"));
   ...
   +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', '.block-views-blocktest-block-view-block-2'));
   

   All of these should be single quotes

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -0,0 +1,132 @@
+
+
+}

Extra blank line

I noticed that in Drupal 8.6.4 there are no contextual links at blocks, that placed into layout. I suppose, contextual links should be removed only when you edit layout, but when you just view this, contextual links should be present.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -0,0 +1,132 @@
+    $node = $this->createNode([
+      'type' => 'bundle_with_section_field',
+      'body' => [
+        [
+          'value' => 'The node body',
+        ],
+      ],
+    ]);
+    $node->save();

This currently-unused node should either be removed or the tests should include confirming contextual links are intact on node view. I'd prefer adding the test, although one could argue its out of scope since the contextual-link-removing JS is only loaded when editing layouts.

1. #14 fixed
2. #15 that is actually intentional for how blocks placed in the layout builder work. It would be separate issue if we wanted to change that.

   The contextual links that should show up on the node view(and do from my testing in 8.7.x) are the contextual links inside the blocks note the contextual links at the block level.

   For instance if node gets rendered inside the layout builder either through a view that is placed or an entity referenced field that displays the node those contextual links to edit/quick edit the node do show up. Also the "edit view" contextual link will not show up.

   But placing a field block or the "Powered by Drupal" block you will not get a contextual link for the placed block on the node view. You have to edit the layout to alter those settings.

   In the case of the where the defaults layout for the content type was rendering the node if we offered the contextual link on the block level this would be editing for all of the content(if we could do it).

3. #16 removed. Yeah I think we would just be testing if library loading worked.

Test passed locally for me. I think this is a failure because we need to wait for the off-canvas dialog to close. Also should wait after clicking "Save dialog"

Re-re-reviewed the patch and it looks good. The test coverage is good, but I did some additional manual testing just to be sure

• Confirmed contextual-link-specific screenreader content properly acknowledged the changes in this patch.
• This is nicely covered in the tests, but to put myself at RTBC ease I added a Views block and a block with a Media audio field - once the patch is applied they do not offer contexual links in Layout Builder edit.
• Performed a number of tasks that triggered the drupalContextualLinkAdded js event, and monitored the JS console to confirm no errors were present.

It looks like there was random test failure https://www.drupal.org/pift-ci-job/1181203 for #19

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -0,0 +1,123 @@
+    $page->pressButton('Add Block');
+    $this->waitForNoElement('#drupal-off-canvas');
+    // Ensure the contextual links are correct when the Layout Builder is
+    // replaced via Ajax.
+    $this->assertCorrectContextualLinks();
+    $assert_session->linkExists('Save Layout');

The error happened here clicking "Save Layout".

WebDriver\Exception\UnknownError: unknown error: Element ... is not clickable at point (60, 570). Other element would receive the click: ...
(Session info: headless chrome=62.0.3202.94)
(Driver info: chromedriver=2.33.506092 (733a02544d189eeb751fe0d7ddca79a0ee28cce4),platform=Linux 4.9.0-0.bpo.6-amd64 x86_64)

So 2 things happen in \Drupal\layout_builder\Controller\LayoutRebuildTrait::rebuildAndClose

    $response = $this->rebuildLayout($section_storage);
    $response->addCommand(new CloseDialogCommand('#drupal-off-canvas'));

We need to wait for both the off-canvas to close and for the new layout to be rebuilt. Checking for the new View to be rendered means the Layout has been rebuilt.

Added this wait.

This patch just runs this test 25x.

re #24

Checking for the new View to be rendered means the Layout has been rebuilt.

I was wrong we were already waiting for this inside assertCorrectContextualLinks()
with
$this->assertNotEmpty($assert_session->waitForElementVisible('css', '.block-views-blocktest-block-view-block-2'));

Adding another wait.

I reproduced the inconsistent test failures on a local install DrupalCI, and was able to fix it by disabling CSS transitions for .dialog-off-canvas-main-canvas. If this patch works with official-Testbot, I'll provide a cleaner implementation + interdiff.

Looks like Settings Tray already had a module that almostaddressed removing css transitions. Had to add an additional transition: none !important; for it to override the transition css for

.dialog-off-canvas-main-canvas

I added this module to every Layout Builder test using Settings Tray

Also added @file doc to layout_builder.es6.js for JS standards.

Interdiff from #19 as the changes that followed were related to solving the intermittent test failures, and are not included in this patch.

@bnjmnm re #28

I reproduced the inconsistent test failures on a local install DrupalCI, and was able to fix it by disabling CSS transitions for .dialog-off-canvas-main-canvas.

Nice work!

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/FieldBlockTest.php
@@ -23,6 +23,7 @@ class FieldBlockTest extends WebDriverTestBase {
+    'settings_tray_test_css',

I don't think we should be including this test module from another module in our tests. Especially since that module isn't involved in test at all.

I think we should duplicate the test module to layout_builder_test_css and add a todo to #2901792: Disable all animations in Javascript testing which has a module to remove animations in general.

Also we should not be adding the test module to any other tests because they are not currently failing in head. We need to keep this issue scoped to just the tested need.

In addition with the next patch could we get a x25 patch that is exactly the same except without the new layout_builder_test_css. To prove on drupalci that is the fix.

Created LB specific module for disabling animations + minor JS code standard fix.
There are diffs for the changes in my previous patch + the last patch where it was RTBC

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -51,4 +64,28 @@
+  $(document).on('drupalContextualLinkAdded', (event, data) => {
+    /**
+     * Remove all contextual links inside the Layout Builder except Layout
+     * Builder contextual links.
+     */
+    const element = data.$el;
+    const contextualId = element.attr('data-contextual-id');
+    if (contextualId && !contextualId.startsWith('layout_builder_block:')) {
+      if (element.closest('[data-layout-block-uuid]').length > 0) {
+        element.remove();
+      }
+    }
+  });

So here is my big question...what if this runs _after_ contextual links have been initialized? Will it apply retroactively? Is there any way to test that?

@phenaproxima -- the drupalContextualLinkAdded event is triggered as part of the initialization process for each contextual link, so it this listener should always run during initialization, never after.

It is, of course, possible for this event to run many times after initial pageload. It will also happen any time an AJAX request returns an item with contextual links. This scenario is covered in the existing tests when adding blocks to the layout triggers a rebuild, which includes a re-triggering of drupalContextualLinkAdded. I think the tests can be enhanced to demonstrate these additional calls have no unwanted effects, which I'll take care of.

If I'm misinterpeting #34 definitely let me know so I can incorporate that into the expanded tests.

Locally, the contextual links tests are passing without the fix applied. Adding a test-only patch to confirm this is happening outside of my dev environment before I go too far down the rabbit hole.

The test-only on #36 is passing, so setting this back to needs work.

The earlier "should fail" tests were likely failing because of the other intermittent test failure issues. The views block in the test was not generating a contextual link. This was addressed with the following:

1. Enabled views_ui module
2. Added administer views permission
3. Testing layout UI in node override instead of Content Type defaults so the views block is configurable and not just showing placeholder content.

Also discussed #35 with @phenaproxima and confirmed his concern is not an issue as the even listener is not inside a behavior.

A x25 test is included because of the intermittent failures that were happening with earlier patches.

@bnjmnm great work! RTBC!

The patch looks good and applies successfully. +1 for RTBC.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -160,4 +160,28 @@
   +    if (contextualId && !contextualId.startsWith('layout_builder_block:')) {
   

   .startsWith isn't supported by IE 11 which we still support.

2. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test_css_transitions/css/layout_builder_test_css_transitions.test.css
   @@ -0,0 +1,7 @@
   +* {
   +  /* CSS transitions. */
   +  transition: none !important;
   +}
   

   I'm wondering if this should be applied globally in all JavaScript tests. Definitely not in the scope of this issue but we should open follow-up to explore the idea.

re #43.2

There is an existing issue #2901792: Disable all animations in Javascript testing

I renamed this issue to the patch that is on there

1. #43.1
   

   .startsWith isn't supported by IE 11 which we still support.

   Changed to use substr()

2. #43.2 -- there's an existing @todo to remove layout_builder_test_css_transitions if #2901792: Disable all animations in Javascript testing is completed.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -0,0 +1,146 @@
+    $assert_session->linkExists('Save Layout');
+    $this->clickLink('Save Layout');

This was changed to button in #3004536: Move the Layout Builder UI into an entity form for better integration with other content authoring modules and core features so the test needs to be updated. Also now the "l" is not capitalized.

Otherwise it looks good. I manually tested in again and it does remove the contextual links

Fixed tests per #46

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -160,4 +160,32 @@
+      contextualId.substr(0, 21) !== 'layout_builder_block:'
+    ) {
+      if (element.closest('[data-layout-block-uuid]').length > 0) {

Now that I look at this again, why do we have the nested if? We could just more this condition to the outer diff.

Otherwise looks good

Re #50

Now that I look at this again, why do we have the nested if? We could just more this condition to the outer diff.

Good catch - no reason for the nested if anymore. Refactored in the patch.

Ok looks good!

Would it make sense to add test coverage to ensure that nested contextual links are being removed? As far as I can tell, this is supported by the current solution but it isn't covered by the tests.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -0,0 +1,146 @@
+    // Ensure that the Layout Builder's own contextual links are not removed.
+    $this->assertCount(3, $page->findAll('css', '[data-contextual-id*=\'layout_builder_block:\']'));
+
+    // Ensure that no other contextual links are in Layout Builder.
+    $this->assertCount(3, $page->findAll('css', ' #layout-builder [data-contextual-id]'));

Because we have added the View block which should have the contextual link for editing the View and the Nodes within the View this proves the contextual links have been removed.

Created this issue #3034800: Allow modules to prevent certain Contextual links from being render as a follow up after chatting with @lauriii

I am not adding a todo because it is likely that the new issue will just determine the way we did it here is just fine. But we should explore if a new way should be created.

Reroll. The smaller size compared to #51 is because the layout_builder_test_css_transitions module is now in HEAD

setting to Needs Work to run tests

Reroll looks good! ☄️

I'm concerned about the fact that this patch completely ignores updating Drupal.contextual.collection. For example, Drupal.contextualToolbar.StateModel.countContextualLinks provides false information about the contextual link count on the page. We also can't really make predictions on what kind of custom code there might be built on top of contextual.

Could we look into a solution that would remove the placeholders from the markup before contextual links get initialized? This way the items would never even get created into Drupal.contextual.collection.

From #61

Could we look into a solution that would remove the placeholders from the markup before contextual links get initialized? This way the items would never even get created into Drupal.contextual.collection.

This patch is an entirely new approach, most of what remains is the test (which was also expanded). This is definitely a first try so I won't be surprised if there are more elegant ways to accomplish certain things such as string matching on the current route.

Some explanations of what I did

1. Where possible, recursively remove ['#contextual_links'] from render arrays.
2. To remove contextual links from views, added a function to get the views executable from ViewsBlockBase so ->setShowAdminLinks(FALSE) can be called on the view before build
3. Entity contextual links are removed in layout_builder_entity_view_alter as they aren't available in arrays when inside a block plugin.
4. Created layout_builder_ui cache context since entity Contextual links are rendered when viewing layouts, but not when they're in the Layout Builder UI.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -140,6 +141,26 @@ function layout_builder_entity_view_alter(array &$build, EntityInterface $entity
   +    while ($value = NestedArray::getValue($build, ['#contextual_links'])) {
   

   Nit: $value is not actually used.

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -140,6 +141,26 @@ function layout_builder_entity_view_alter(array &$build, EntityInterface $entity
   +  // Add cache context for if enitity is viewed by Layout Builder UI.
   

   "entity" is misspelled here.

3. +++ b/core/modules/layout_builder/src/Cache/LayoutBuilderUICacheContext.php
   @@ -0,0 +1,54 @@
   +/**
   + * Determines if an entity is being viewed in the Layout Builder UI.
   + *
   + * Cache context ID: 'layout_builder_ui'.
   + */
   +class LayoutBuilderUICacheContext implements CalculatedCacheContextInterface {
   

   Again, chalk this up to my non-understanding of cache contexts, but why does this need to implement CalculatedCacheContextInterface? As far as I can tell, the only real difference is the presence of $parameter in getContext(), but it's not used in this implementation.

4. +++ b/core/modules/layout_builder/src/Cache/LayoutBuilderUICacheContext.php
   @@ -0,0 +1,54 @@
   +    return (strpos($this->routeMatch->getRouteName(), 'layout_builder.') === 0) ? 1 : 0;
   

   Nit: The outer parentheses are superfluous.

5. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -90,6 +92,14 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +      if ($block instanceof ViewsBlock && $event->inPreview()) {
   +        $block->getViewExecutable()->setShowAdminLinks(FALSE);
   +      }
   

   If Views is not installed, will this cause a fatal error (will ViewsBlock be autoloadable)? It might be useful to call class_exists() too.

6. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -98,6 +108,13 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +        while ($value = NestedArray::getValue($content, ['#contextual_links'])) {
   

   Nit: $value is not used.

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -0,0 +1,177 @@
   +  protected function waitForNoElement($selector, $timeout = 10000) {
   +
   +    $start = microtime(TRUE);
   

   Nit: There's a superfluous blank line here.

Thanks @phenaproxima - addressed the nits and a few responses to your items
#64.3

why does this need to implement CalculatedCacheContextInterface?

. Turns out it doesn't -- was just force of habit so I'm glad you spotted that.

#64.5

If Views is not installed, will this cause a fatal error (will ViewsBlock be autoloadable)?

Confirmed via manual testing & checking logs that this code results in no errors or unwelcome events if Views isn't installed.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -140,6 +141,26 @@ function layout_builder_entity_view_alter(array &$build, EntityInterface $entity
   +  // If the entity is displayed within a Layout Builder block and the current
   +  // route is in the Layout Builder UI.
   

   Language nit: "if … " but no "then".

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -140,6 +141,26 @@ function layout_builder_entity_view_alter(array &$build, EntityInterface $entity
   +  if ($display instanceof LayoutBuilderEntityViewDisplay && strpos($route_name, 'layout_builder.') === 0) {
   +    // Remove all contextual link placeholders.
   +    while (NestedArray::getValue($build, ['#contextual_links'])) {
   +      NestedArray::unsetValue($build, ['#contextual_links']);
   +    }
   +  }
   

   🤔 Why do we need to use NestedArray instead of unset()?

3. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -140,6 +141,26 @@ function layout_builder_entity_view_alter(array &$build, EntityInterface $entity
   +  // Add cache context for if entity is viewed by Layout Builder UI.
   

   Language nit: "for if"

   Could use an // @see layout_builder_entity_view_alter()?

4. +++ b/core/modules/layout_builder/layout_builder.services.yml
   @@ -29,6 +29,11 @@ services:
   +  cache_context.layout_builder_ui:
   
   +++ b/core/modules/layout_builder/src/Cache/LayoutBuilderUICacheContext.php
   @@ -0,0 +1,54 @@
   +  public function getContext() {
   +    return strpos($this->routeMatch->getRouteName(), 'layout_builder.') === 0 ? 1 : 0;
   +  }
   

   This 100% depends on the matched route. So route is the parent cache context.

   As described on https://www.drupal.org/docs/8/api/cache-api/cache-contexts, that means this cache context should be route.layout_builder_ui, not layout_builder_ui.

   (Because: if something is already varying by route, then there is no more need to also vary by layout_builder_ui.)

5. +++ b/core/modules/layout_builder/src/Cache/LayoutBuilderUICacheContext.php
   @@ -0,0 +1,54 @@
   +class LayoutBuilderUICacheContext implements CacheContextInterface {
   

   🐫 s/UI/Ui/

6. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -98,6 +108,13 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +      // Remove all contextual links from $build when in the Layout Builder UI.
   +      if ($event->inPreview()) {
   +        while (NestedArray::getValue($content, ['#contextual_links'])) {
   +          NestedArray::unsetValue($content, ['#contextual_links']);
   +        }
   

   1. 🤔I think an // @see \Drupal\block\BlockViewBuilder::preRender() may be appropriate.
   2. 🤔 Also, why do we need to use NestedArray?

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -0,0 +1,176 @@
   +class ContextualLinksTest extends WebDriverTestBase {
   

   I'd like to see this assert the presence of the cache context on these responses.

8. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -0,0 +1,176 @@
   +    // Enable layout builder and overrides.
   ...
   +    // Ensure the contextual links are correct when the Layout Builder is
   ...
   +    // Ensure the contextual links are correct when the Layout Builder is loaded
   ...
   +    // Ensure that no layout builder contextual links are visible on node view.
   

   s/layout builder/Layout Builder/

   This is done in many places, but not everywhere.

9. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -0,0 +1,176 @@
   +    // Add a block that includes entity contextual link.
   ...
   +    // Add a block that includes views contextual link.
   

   🤔 "that includes FOO contextual link" -> "that includes the FOO contextual link"

10. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
    @@ -0,0 +1,176 @@
    +    // Ensure that the Layout Builder's own contextual links are present.
    +    $this->assertCount(4, $page->findAll('css', '[data-contextual-id*=\'layout_builder_block:\']'));
    +
    +    // Ensure that no other contextual links are in Layout Builder.
    +    $this->assertCount(4, $page->findAll('css', '#layout-builder [data-contextual-id]'));
    

    👍

    Except: let's do:

    $contextual_links_count_layout_builder = $page->findAll(…);
    $this->assertNotEmpty($contextual_links_count_layout_builder);
    $this->assertSameCount($contextual_links_count_layout_builder, $page->findAll(…);
    

    That way, this test is not coupled to that current specific count, which will likely change in the future.

11. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
    @@ -0,0 +1,176 @@
    +   * Assert the contextual links are correct when viewing node.
    

    Language nit: "are correct on the canonical entity route"

12. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
    @@ -0,0 +1,176 @@
    +    $this->assertCount(4, $page->findAll('css', '.layout-content [data-contextual-id]'));
    

    Hm, it just happens to be 4? This may need to change in the future. How about assertNotEmpty()?

13. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
    @@ -0,0 +1,176 @@
    +  protected function waitForNoElement($selector, $timeout = 10000) {
    +    $start = microtime(TRUE);
    +    $end = $start + ($timeout / 1000);
    +    $page = $this->getSession()->getPage();
    +
    +    do {
    +      $result = $page->find('css', $selector);
    +
    +      if (empty($result)) {
    +        return;
    +      }
    +
    +      usleep(100000);
    +    } while (microtime(TRUE) < $end);
    +
    +    $this->assertEmpty($result, 'Element was not on the page after wait.');
    +  }
    +
    

    Why another different implementation of this if

      protected function waitForNoElement($selector, $timeout = 10000) {
        $condition = "(typeof jQuery !== 'undefined' && jQuery('$selector').length === 0)";
        $this->assertJsCondition($condition, $timeout);
      }
    

    looks much simpler and already exists in several places in HEAD?

Related: the contextual links which are related to layout builder should be visible all the time, not just on hover focus.

Re #67

1. Nit fixed

2. Why do we need to use NestedArray instead of unset()?

   I was erring on the side of caution in case there were contextual placeholders nested in the array - did not have a specific situation that this addressed. Since this stood out enough to be asked about, I tested a range of scenarios and found no evidence that it was necessary and changed it to unset()

3. Nit fixed, @see added

4. (Because: if something is already varying by route, then there is no more need to also vary by layout_builder_ui.)

   Nice, this has been changed

5. Nit fixed

6. 🤔I think an // @see \Drupal\block\BlockViewBuilder::preRender() may be appropriate.
   🤔 Also, why do we need to use NestedArray?

   I removed this block entirely -- looks like this was an artifact of earlier attempts to fix the issue as is no longer necessary

7. I'm not sure how to check cache context in a FunctionalJavascript test. I gave this a shot in a Functional test using AssertPageCacheContextsAndTagsTrait. Here I could check the page cache context, but still wasn't sure how to check it for the entity, which is what gets the context. Before moving into yet another type of test, I thought I'd see if providing a failing test would be sufficient. 3002608-69-cache-context-test-x10-FAIL comments the addition of the cache context, and ContextualLinksTest fails as a result. It is not a cache context assertion, but it does demonstrate that this context is required for contextual links to work properly with Layout Builder. (if there's a good way to assert this I'm overlooking, point me in the right direction and I'll take care of it)
8. Capitalization nit fixed
9. Comment link fixed (and moved block addition into its own function since there was a bunch of repetition, and you correctly pointed out that this test may have more blocks added in the future)
10. Refactored to not expect a specific number of contextual links.
11. Language nit fixed
12. Refactored to not expect a specific number of contextual links.
13. I pasted in the waitForNoElement test from #2892440: Provide helper test method to wait for an element to be removed from the page. I switched this out for the more compact version that is more commonly used, as it's just as effective for this particular test with fewer lines of code.

Carefully checked each:

1. ✅
2. ✅
3. ✅
4. ✅
5. ✅
6. ✅ — less code, yay!
7. TIL that FunctionalJavascript tests cannot assert response headers! Note that AssertPageCacheContextsAndTagsTrait is being added even though it's not being used.

   So, yes, \Drupal\Tests\system\Functional\Cache\AssertPageCacheContextsAndTagsTrait only works for BrowserTestBase, because it predates BrowserTestBase: it still uses WebTestBase's drupalGetHeader(). But … AFAICT this would work just fine:

   $this->assertTrue(in_array('route.layout_builder_ui', explode(' ', $this->getSession()->getResponseHeader('X-Drupal-Cache-Contexts')));
   

   Or, with a simple change even $this->assertCacheContext('route.layout_builder_ui'); would work fine:

   --- a/core/modules/system/tests/src/Functional/Cache/AssertPageCacheContextsAndTagsTrait.php
   +++ b/core/modules/system/tests/src/Functional/Cache/AssertPageCacheContextsAndTagsTrait.php
   @@ -47,7 +47,7 @@ protected function getCacheHeaderValues($header_name) {
       *   The expected cache context.
       */
      protected function assertCacheContext($expected_cache_context) {
   -    $cache_contexts = explode(' ', $this->drupalGetHeader('X-Drupal-Cache-Contexts'));
   +    $cache_contexts = explode(' ', $this->getSession()->getResponseHeader('X-Drupal-Cache-Contexts'));
        $this->assertTrue(in_array($expected_cache_context, $cache_contexts), "'" . $expected_cache_context . "' is present in the X-Drupal-Cache-Contexts header.");
      }
    

   Or am I missing something?

8. ✅
9. ✅
10. ✅
11. ✅
12. ✅
13. ✅

#71.7

Behat\Mink\Exception\UnsupportedDriverActionException: Response headers are not available from Drupal\FunctionalJavascriptTests\DrupalSelenium2Driver

It's not supported.
Is this important enough that we write a second non-JS Functional test for this assertion?

#72: Wow, you're right:

 public function getResponseHeaders()
    {
        throw new UnsupportedDriverActionException('Response headers are not available from %s', $this);
    }

and apparently the API is even explicitly designed to allow any of the web drivers to throw this exception to indicate the driver doesn't support it. Makes the interface nearly meaningless 😔 But of course that's not this issue's fault.

@bnjmnm's explanation in #69.7 indeed justifies not having an explicit assertion for this: he convincingly shows that the test coverage already proves that the cache context is resulting in the necessary variations (otherwise the tests fail), and hence there is no pressing need to assert the presence of this cache context on the response.

1. I did one final review with the intent of RTBC'ing, and fixed all nits I could find. See interdiff.

2. +++ b/core/modules/layout_builder/layout_builder.services.yml
   @@ -29,6 +29,11 @@ services:
   +  cache_context.route.layout_builder_ui:
   

   I think that route.is_layout_builder_ui would be even clearer. Per https://www.drupal.org/docs/8/api/cache-api/cache-contexts, that'd be consistent with url.path.is_front and user.is_super_user. But I don't feel strongly about that.

3. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -90,6 +91,14 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +      // If the block is a Views Block in the Layout Builder UI, its
   +      // contextual links placeholders must not get rendered. Doing this
   +      // requires setting showAdminLinks to false before the View Block is
   +      // built.
   +      if ($block instanceof ViewsBlock && $event->inPreview()) {
   +        $block->getViewExecutable()->setShowAdminLinks(FALSE);
   +      }
   
   +++ b/core/modules/views/src/Plugin/Block/ViewsBlockBase.php
   @@ -213,4 +213,14 @@ protected function addContextualLinks(&$output, $block_type = 'block') {
   +  public function getViewExecutable() {
   +    return $this->view;
   +  }
   

   This is the only thing I don't think I can RTBC myself. I think that ideally Layout Builder wouldn't need to do something special for one particular block (and then even expand that block plugin's API surface); the block should instead be able to react to the context it's being displayed in.

   I defer to a Views maintainer to sign off on this, and ideally also a Block module maintainer. Fortunately, those can be one and the same person: @tim.plunkett :) Assigning to him.

Responding to #73.3

+++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
@@ -90,6 +91,13 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
+      // If the block is a Views Block in the Layout Builder UI, its contextual
+      // links placeholders must not get rendered. Doing this requires setting
+      // showAdminLinks to false before the View Block is built.

Please make this whole comment an @todo pointing to #3027653: Allow block and layout plugins to determine if they are being previewed, which is required to move this code out of here to where it belongs

@bnjmnm, your call on 73.2

#72.2 renamed the cache context.

#73.3 addressed by adding the @todos per the instruction in #74

Thanks!

+++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
@@ -91,9 +91,12 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
+      /*
+       * @todo revisit after https://www.drupal.org/node/3027653, as this will
+       *   provide a better way to remove contextual links from Views blocks.
+       *   Currently, doing this requires setting showAdminLinks to false
+       *   before the Views block is built.
+       */

This should still use // style comments, and the R in revisit should be capitalized.

Also I think showAdminLinks should get the full \Drupal\views\ViewExecutable::$showAdminLinks()

Re: #76 That was some choice JS/PHP vertigo. Good opportunity to notice the comment would be improved by using \Drupal\views\ViewExecutable::$showAdminLinks(), though!

👌

For #74.

This patch changes only PHP and YAML, so not a frontend issue in its current state. :)

The same thing needed for views ui #3039248: Deprecate views_ui_contextual_links_suppress(), views_ui_contextual_links_suppress_push(), views_ui_contextual_links_suppress_pop()

+++ b/core/modules/layout_builder/src/Cache/LayoutBuilderUiCacheContext.php
@@ -0,0 +1,28 @@
+    return t('Is Layout Builder UI route');

This is rather... gnomic. I looked for other examples in core, but unless I've missed something there are no other examples? Where is this label used?

AFAICS that method is literally never called in core. (\Drupal\Core\Cache\Context\CalculatedCacheContextInterface::getLabel())

We have another one of these cache context classes in LB, \Drupal\layout_builder\Cache\LayoutBuilderIsActiveCacheContext has the following:

  public static function getLabel() {
    return t('Layout Builder');
  }

Which is super not helpful.

#82: The url.path.is_front cache context has a similar label. And Tim is right, this string is not visible anywhere in the UI; this label only exists for a hypothetical future contributed module where a site builder can manually associate cache contexts and/or for a debugging/auditing contrib module such as https://www.drupal.org/project/renderviz.

Nonetheless, can we make it less odd? Simply "Layout Builder user interface"?

+++ b/core/modules/layout_builder/src/Cache/LayoutBuilderUiCacheContext.php
@@ -0,0 +1,28 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getContext() {
+    return strpos($this->routeMatch->getRouteName(), 'layout_builder.') === 0 ? 1 : 0;
+  }

Hmm I also don't think this is doing the right thing. Wim's example in IsFrontPathCacheContext returns either is_front.1 or is_front.0. Sort-of descriptive string machine names. This is returning simply a 1 or 0 with no namespace -- also not great for debugging. :)

1. +++ b/core/modules/layout_builder/src/Cache/LayoutBuilderUiCacheContext.php
   @@ -0,0 +1,28 @@
   + * Cache context ID: 'route.is_layout_builder_ui'.
   

   @effulgentsia suggested this be route.name.is_layout_builder_ui

2. +++ b/core/modules/layout_builder/src/Cache/LayoutBuilderUiCacheContext.php
   @@ -0,0 +1,28 @@
   +    return strpos($this->routeMatch->getRouteName(), 'layout_builder.') === 0 ? 1 : 0;
   

   Should be ? '1' : '0'

1. @effulgentsia is right 😅😊, as he tends to be! 😀 I'd swear I looked for that before and thought that the route.name cache context indeed existed, but I couldn't find it. It must've been late :| My bad, I'm sorry!
2. @xjm's comment in #86 is a good one, the "is front" cache context does this:
   

     public function getContext() {
       return 'is_front.' . (int) $this->pathMatcher->isFrontPage();
     }
   

   I think we can do something similar here. No functional difference, but it may help people doing deep dives on caching problems.

Renamed cache context label per #85
Renamed cache context id per #88.1
Addressed #88.2 + #86 by using the suggestion in #89

Great, thanks!

#90 triggers a minor bit of OCD, fixed that :)

Confirming RTBC.

Thanks @Wim Leers et al; that reduces the cache context confusion.

1. I manually tested this and it's a definite usability improvement. One out-of-scope thing it made me notice is that the contextual links outside the Layout Builder (on "site chrome" blocks) are still somewhat confusing and still a way to accidentally leave the page (because they also contain the same "configure block" as an option, but that takes you to the block UI rather than the LB tray). Do we have a separate issue to discuss removing interactions for the rest of the page, similar to what we do with node previews?

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -0,0 +1,170 @@
   +    // @todo The Layout Builder UI relies on local tasks; fix in
   +    //   https://www.drupal.org/project/drupal/issues/2917777.
   +    $this->drupalPlaceBlock('local_tasks_block');
   

   So this comment is no longer exactly valid; the linked issue has been rescoped and has nothing to do with local tasks. However, I checked with @tim.plunkett and he pointed out that all our tests are still navigating to the node page first, and then clicking the actual layout tab.

   So we should do one of:

   • Change all the tests to navigate directly to the layout page, rather than clicking on the tab, or
   • Keep the "click on the tab" navigation, but just remove the @todo.

   However that should be done in a separate followup since it is everywhere. Tagging "Needs followup" for that.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -0,0 +1,170 @@
   +    // Ensure the contextual links are correct when the Layout Builder is
   +    // replaced via Ajax.
   +    $this->assertCorrectContextualLinksInUi();
   

   This comment is confusing. Reading both this and the protected method it calls, I can't tell what's "when the Layout Builder is replaced via AJAX". ?

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContextualLinksTest.php
   @@ -0,0 +1,170 @@
   +    $page->hasButton('Save layout');
   +    $page->pressButton('Save layout');
   +    $this->drupalGet('node/1/layout');
   +
   +    // Ensure the contextual links are correct when the Layout Builder is loaded
   +    // after being saved.
   +    $this->assertCorrectContextualLinksInUi();
   

   Shouldn't the inline comment here be above those preceding three lines?

#3028191: When using Layout Builder, remove contextual links for blocks outside of the current layout is for #93.1, it has this issue linked as a related issue

Created #3042089: Update Layout Builder functional javascript tests now that local tasks are not required by the UI. for #93.2

#93 item 2 ) For this particular test, the local tasks block was not necessary at all that + the @todo was removed and I created #3042089: Update Layout Builder functional javascript tests now that local tasks are not required by the UI. to follow up on this in the other tests.

#93 items 3 & 4 ) updated/moved comments.

Much clearer, thanks @bnjmnm! This fully addresses #93

Saving issue credit.

Committed and pushed to 8.8.x and 8.7.x. Thanks!