Issue #2782891 by tedbow, Wim Leers, pk188, tim.plunkett, Adita, RajeevK, xjm, webchick: The Page Title block's title behaves in a confusing way with Settings Tray and the Help block incorrectly has Settings Tray styling

core/modules/outside_in/outside_in.api.php

@@ -0,0 +1,61 @@
+<?php
+
+/**
+ * @file
+ * Documentation for Settings Tray API.
+ */
+
+/**
+ * @defgroup outside_in Settings Tray API
+ * @{
+ * Settings Tray API
+ *
+ * @section sec_api The API: the form in the Settings Tray
+ *
+ * By default, every block will show its built-in form in the Settings Tray.
+ * However, many blocks would benefit from a tailored form which either:
+ * - limits the form items displayed in the Settings Tray to only items that
+ *   affect the content of the rendered block
+ * - adds additional form items to edit configuration that is rendered by the
+ *   block. See \Drupal\outside_in\Form\SystemBrandingOffCanvasForm which adds
+ *   site name and slogan configuration.
+ *
+ * These can be used to provide a better experience, so that the Settings Tray
+ * only displays what the user will expect to change when editing the block.
+ *
+ * Each block plugin can specify which form to use in the Settings Tray dialog
+ * in its plugin annotation:
+ * @code
+ * forms = {
+ *   "off_canvas" = "\Drupal\some_module\Form\MyBlockOffCanvasForm",
+ * },
+ * @encode
+ *
+ * In some cases, a block's content is not configurable (for example, the title,
+ * main content, and help blocks). Such blocks can opt out of providing an
+ * off-canvas form:
+ * @code
+ * forms = {
+ *   "off_canvas" = FALSE,
+ * },
+ * @encode
+ *
+ * Finally, blocks that do not specify an off-canvas form using the annotation
+ * above will automatically have it set to their plugin class. For example, the
+ * "Powered by Drupal" block plugin
+ * (\Drupal\system\Plugin\Block\SystemPoweredByBlock) automatically gets
+ * this added to its annotation:
+ * @code
+ * forms = {
+ *   "off_canvas" = "\Drupal\system\Plugin\Block\SystemPoweredByBlock",
+ * },
+ * @encode
+ *
+ * Therefore, the entire Settings Tray API is just this annotation: it controls
+ * what the Settings Tray does for a given block.
+ *
+ * @see outside_in_block_alter()
+ * @see \Drupal\Tests\outside_in\Functional\OutsideInBlockTest::testPossibleAnnotations()
+ *
+ * @}
+ */

core/modules/outside_in/outside_in.module

@@ -94,8 +94,20 @@ function outside_in_entity_type_build(array &$entity_types) {
  * Implements hook_preprocess_HOOK() for block templates.
  */
 function outside_in_preprocess_block(&$variables) {
-  // The main system block does not contain the block contextual links.
+  // Only blocks that have an off_canvas form will have a "Quick Edit" link. We
-  if ($variables['plugin_id'] !== 'system_main_block') {
+  // could wait for the contextual links to be initialized on the client side,
+  // and then add the class and data- attribute below there (via JavaScript).
+  // But that would mean that it would be impossible to show Settings Tray's
+  // clickable regions immediately when the page loads. When latency is high,
+  // this will cause flicker.
+  // @see \Drupal\outside_in\Access\BlockPluginHasOffCanvasFormAccessCheck
+  /** @var \Drupal\outside_in\Access\BlockPluginHasOffCanvasFormAccessCheck $access_checker */
+  $access_checker = \Drupal::service('access_check.outside_in.block.off_canvas_form');
+  /** @var \Drupal\Core\Block\BlockManagerInterface $block_plugin_manager */
+  $block_plugin_manager = \Drupal::service('plugin.manager.block');
+  /** @var \Drupal\Core\Block\BlockPluginInterface $block_plugin */
+  $block_plugin = $block_plugin_manager->createInstance($variables['plugin_id']);
+  if ($access_checker->accessBlockPlugin($block_plugin)->isAllowed()) {
     // Add class and attributes to all blocks to allow Javascript to target.
     $variables['attributes']['class'][] = 'outside-in-editable';
     $variables['attributes']['data-drupal-outsidein'] = 'editable';
@@ -108,7 +120,9 @@ function outside_in_preprocess_block(&$variables) {
  * Alters the 'contextual' toolbar tab if it exists (meaning the user is allowed
  * to use contextual links) and if they can administer blocks.
  *
- * @todo Remove the "administer blocks" requirement in https://www.drupal.org/node/2822965
+ * @todo Remove the "administer blocks" requirement in
+ *   https://www.drupal.org/node/2822965.
+ *
  * @see contextual_toolbar()
  */
 function outside_in_toolbar_alter(&$items) {
@@ -120,8 +134,7 @@ function outside_in_toolbar_alter(&$items) {
 
     // Set a class on items to mark whether they should be active in edit mode.
     // @todo Create a dynamic method for modules to set their own items.
-    //   https://www.drupal.org/node/2784589
+    //   https://www.drupal.org/node/2784589.
-
     $edit_mode_items = ['contextual', 'block_place'];
     foreach ($items as $key => $item) {
       if (!in_array($key, $edit_mode_items) && (!isset($items[$key]['#wrapper_attributes']['class']) || !in_array('hidden', $items[$key]['#wrapper_attributes']['class']))) {
@@ -133,17 +146,54 @@ function outside_in_toolbar_alter(&$items) {
 
 /**
  * Implements hook_block_alter().
+ *
+ * Ensures every block plugin definition has an 'off_canvas' form specified.
+ *
+ * @see \Drupal\outside_in\Access\BlockPluginHasOffCanvasFormAccessCheck
  */
 function outside_in_block_alter(&$definitions) {
-  if (!empty($definitions['system_branding_block'])) {
-    $definitions['system_branding_block']['forms']['off_canvas'] = SystemBrandingOffCanvasForm::class;
-  }
-
-  // Since menu blocks use derivatives, check the definition ID instead of
-  // relying on the plugin ID.
   foreach ($definitions as &$definition) {
-    if ($definition['id'] === 'system_menu_block') {
+    // If a block plugin already defines its own off_canvas form, use that form
-      $definition['forms']['off_canvas'] = SystemMenuOffCanvasForm::class;
+    // instead of specifying one here.
+    if (isset($definition['forms']['off_canvas'])) {
+      continue;
+    }
+
+    switch ($definition['id']) {
+      // Use specialized forms for certain blocks that do not yet provide the
+      // form with their own annotation.
+      // @todo Move these into the corresponding block plugin annotations in
+      //   https://www.drupal.org/node/2896356.
+      case 'system_menu_block':
+        $definition['forms']['off_canvas'] = SystemMenuOffCanvasForm::class;
+        break;
+
+      case 'system_branding_block':
+        $definition['forms']['off_canvas'] = SystemBrandingOffCanvasForm::class;
+        break;
+
+      // No off-canvas form for the page title block, despite it having
+      // contextual links: it's too confusing that you're editing configuration,
+      // not content, so the title itself cannot actually be changed.
+      // @todo Move these into the corresponding block plugin annotations in
+      //   https://www.drupal.org/node/2896356.
+      case 'page_title_block':
+        $definition['forms']['off_canvas'] = FALSE;
+        break;
+
+      case 'system_main_block':
+        $definition['forms']['off_canvas'] = FALSE;
+        break;
+
+      case 'help_block':
+        $definition['forms']['off_canvas'] = FALSE;
+        break;
+
+      // Otherwise, use the block plugin's normal form rather than
+      // a custom form for Settings Tray.
+      default:
+        $definition['forms']['off_canvas'] = $definition['class'];
+        break;
     }
   }
 }

core/modules/outside_in/outside_in.routing.yml

@@ -5,3 +5,4 @@ entity.block.off_canvas_form:
     _title_callback: '\Drupal\outside_in\Block\BlockEntityOffCanvasForm::title'
   requirements:
     _permission: 'administer blocks'
+    _access_block_plugin_has_offcanvas_form: 'TRUE'

core/modules/outside_in/outside_in.services.yml

@@ -4,3 +4,8 @@ services:
     arguments: ['@title_resolver', '@renderer']
     tags:
       - { name: render.main_content_renderer, format: drupal_dialog.off_canvas }
+
+  access_check.outside_in.block.off_canvas_form:
+    class: Drupal\outside_in\Access\BlockPluginHasOffCanvasFormAccessCheck
+    tags:
+      - { name: access_check, applies_to: _access_block_plugin_has_offcanvas_form }

core/modules/outside_in/src/Access/BlockPluginHasOffCanvasFormAccessCheck.php

@@ -0,0 +1,48 @@
+<?php
+
+namespace Drupal\outside_in\Access;
+
+use Drupal\block\BlockInterface;
+use Drupal\Core\Access\AccessResult;
+use Drupal\Core\Block\BlockPluginInterface;
+use Drupal\Core\Plugin\PluginWithFormsInterface;
+use Drupal\Core\Routing\Access\AccessInterface;
+
+/**
+ * Determines whether the requested block has an 'off_canvas' form.
+ *
+ * @internal
+ */
+class BlockPluginHasOffCanvasFormAccessCheck implements AccessInterface {
+
+  /**
+   * Checks access for accessing a block's 'off_canvas' form.
+   *
+   * @param \Drupal\block\BlockInterface $block
+   *   The block whose 'off_canvas' form is being accessed.
+   *
+   * @return \Drupal\Core\Access\AccessResultInterface
+   *   The access result.
+   */
+  public function access(BlockInterface $block) {
+    /** @var \Drupal\Core\Block\BlockPluginInterface $block_plugin */
+    $block_plugin = $block->getPlugin();
+    return $this->accessBlockPlugin($block_plugin);
+  }
+
+  /**
+   * Checks access for accessing a block plugin's 'off_canvas' form.
+   *
+   * @param \Drupal\Core\Block\BlockPluginInterface $block_plugin
+   *   The block plugin whose 'off_canvas' form is being accessed.
+   *
+   * @return \Drupal\Core\Access\AccessResultInterface
+   *   The access result.
+   *
+   * @see outside_in_preprocess_block()
+   */
+  public function accessBlockPlugin(BlockPluginInterface $block_plugin) {
+    return AccessResult::allowedIf($block_plugin instanceof PluginWithFormsInterface && $block_plugin->hasFormClass('off_canvas'));
+  }
+
+}

core/modules/outside_in/tests/modules/outside_in_test/src/Form/OffCanvasFormAnnotationIsClassBlockForm.php

@@ -0,0 +1,44 @@
+<?php
+
+namespace Drupal\outside_in_test\Form;
+
+use Drupal\Core\Form\FormStateInterface;
+use Drupal\Core\Plugin\PluginFormBase;
+
+/**
+ * @see \Drupal\outside_in_test\Plugin\Block\OffCanvasFormAnnotationIsClassBlock
+ */
+class OffCanvasFormAnnotationIsClassBlockForm extends PluginFormBase {
+
+  /**
+   * The block plugin.
+   *
+   * @var \Drupal\Core\Block\BlockPluginInterface
+   */
+  protected $plugin;
+
+  /**
+   * {@inheritdoc}
+   */
+  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
+    $form = $this->plugin->buildConfigurationForm($form, $form_state);
+
+    $form['some_setting'] = [
+      '#type' => 'select',
+      '#title' => t('Some setting'),
+      '#options' => [
+        'a' => 'A',
+        'b' => 'B',
+      ],
+      '#required' => TRUE,
+    ];
+
+    return $form;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {}
+
+}

core/modules/outside_in/tests/modules/outside_in_test/src/Plugin/Block/OffCanvasFormAnnotationIsClassBlock.php

@@ -0,0 +1,27 @@
+<?php
+
+namespace Drupal\outside_in_test\Plugin\Block;
+
+use Drupal\Core\Block\BlockBase;
+
+/**
+ * Block that explicitly provides an "off_canvas" form class.
+ *
+ * @Block(
+ *   id = "outside_in_test_class",
+ *   admin_label = "Settings Tray test block: forms[off_canvas]=class",
+ *   forms = {
+ *     "off_canvas" = "\Drupal\outside_in_test\Form\OffCanvasFormAnnotationIsClassBlockForm",
+ *   },
+ * )
+ */
+class OffCanvasFormAnnotationIsClassBlock extends BlockBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function build() {
+    return ['#markup' => '<span>class</span>'];
+  }
+
+}

core/modules/outside_in/tests/modules/outside_in_test/src/Plugin/Block/OffCanvasFormAnnotationIsFalseBlock.php

@@ -0,0 +1,27 @@
+<?php
+
+namespace Drupal\outside_in_test\Plugin\Block;
+
+use Drupal\Core\Block\BlockBase;
+
+/**
+ * Block that explicitly provides no "off_canvas" form, thus opting out.
+ *
+ * @Block(
+ *   id = "outside_in_test_false",
+ *   admin_label = "Settings Tray test block: forms[off_canvas]=FALSE",
+ *   forms = {
+ *     "off_canvas" = FALSE,
+ *   },
+ * )
+ */
+class OffCanvasFormAnnotationIsFalseBlock extends BlockBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function build() {
+    return ['#markup' => '<span>FALSE</span>'];
+  }
+
+}

core/modules/outside_in/tests/modules/outside_in_test/src/Plugin/Block/OffCanvasFormAnnotationNoneBlock.php

@@ -0,0 +1,24 @@
+<?php
+
+namespace Drupal\outside_in_test\Plugin\Block;
+
+use Drupal\Core\Block\BlockBase;
+
+/**
+ * Block that does nothing explicit for Settings Tray.
+ *
+ * @Block(
+ *   id = "outside_in_test_none",
+ *   admin_label = "Settings Tray test block: forms[off_canvas] is not specified",
+ * )
+ */
+class OffCanvasFormAnnotationNoneBlock extends BlockBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function build() {
+    return ['#markup' => '<span>none</span>'];
+  }
+
+}

core/modules/outside_in/tests/src/Functional/OutsideInTest.php

@@ -0,0 +1,109 @@
+<?php
+
+namespace Drupal\Tests\outside_in\Functional;
+
+use Drupal\block\Entity\Block;
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * Tests opening and saving block forms in the off-canvas dialog.
+ *
+ * @group outside_in
+ */
+class OutsideInTest extends BrowserTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static $modules = [
+    'outside_in',
+    'outside_in_test',
+  ];
+
+  /**
+   * Gets the block CSS selector.
+   *
+   * @param \Drupal\block\Entity\Block $block
+   *   The block.
+   *
+   * @return string
+   *   The CSS selector.
+   */
+  protected  function getBlockSelector(Block $block) {
+    return '#block-' . $block->id();
+  }
+
+  /**
+   * Tests the three possible forms[off_canvas] annotations: class, FALSE, none.
+   *
+   * There is also functional JS test coverage to ensure that the two blocks
+   * that support Settings Tray (the "class" and "none" cases) do work
+   * correctly.
+   *
+   * @see OutsideInBlockFormTest::testBlocks()
+   */
+  public function testPossibleAnnotations() {
+    $test_block_plugin_ids = [
+      // Block that explicitly provides an "off_canvas" form class.
+      'outside_in_test_class',
+      // Block that explicitly provides no "off_canvas" form, thus opting out.
+      'outside_in_test_false',
+      // Block that does nothing explicit for Settings Tray.
+      'outside_in_test_none',
+    ];
+
+    $placed_blocks = [];
+    foreach ($test_block_plugin_ids as $plugin_id) {
+      $placed_blocks[$plugin_id] = $this->placeBlock($plugin_id);
+    }
+
+    $this->drupalGet('');
+    $web_assert = $this->assertSession();
+    foreach ($placed_blocks as $plugin_id => $placed_block) {
+      $block_selector = $this->getBlockSelector($placed_block);
+
+      // All blocks are rendered.
+      $web_assert->elementExists('css', $block_selector);
+
+      // All blocks except 'outside_in_test_false' are editable. For more
+      // detailed test coverage, which requires JS execution, see
+      // \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks().
+      if ($plugin_id === 'outside_in_test_false') {
+        $web_assert->elementNotExists('css', "{$block_selector}[data-drupal-outsidein=\"editable\"]");
+      }
+      else {
+        $web_assert->elementExists('css', "{$block_selector}[data-drupal-outsidein=\"editable\"]");
+      }
+    }
+  }
+
+  /**
+   * Tests that certain blocks opt out from Settings Tray.
+   */
+  public function testOptOut() {
+    $web_assert = $this->assertSession();
+
+    $non_excluded_block = $this->placeBlock('system_powered_by_block');
+    $excluded_block_plugin_ids = ['page_title_block', 'system_main_block', 'outside_in_test_false'];
+    $block_selectors = [];
+    // Place blocks that should be excluded.
+    foreach ($excluded_block_plugin_ids as $excluded_block_plugin_id) {
+      // The block HTML 'id' attribute will be "block-[block_id]".
+      $block_selectors[] = $this->getBlockSelector($this->placeBlock($excluded_block_plugin_id));
+    }
+    $this->drupalGet('');
+    // Assert that block has been marked as "editable" and contextual that
+    // should exist does.
+    $web_assert->elementExists('css', $this->getBlockSelector($non_excluded_block) . "[data-drupal-outsidein=\"editable\"]");
+    // Assert that each block that has a "forms[off_canvas] = FALSE" annotation:
+    // - is still rendered on the page
+    // - but is not marked as "editable" by outside_in_preprocess_block()
+    // - and does not have the Settings Tray contextual link.
+    foreach ($block_selectors as $block_selector) {
+      $web_assert->elementExists('css', $block_selector);
+      $web_assert->elementNotExists('css', "{$block_selector}[data-drupal-outsidein=\"editable\"]");
+      $web_assert->elementNotExists('css', "$block_selector [data-outside-in-edit]");
+    }
+  }
+
+}

core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php

@@ -5,6 +5,8 @@ namespace Drupal\Tests\outside_in\FunctionalJavascript;
 use Drupal\block\Entity\Block;
 use Drupal\block_content\Entity\BlockContent;
 use Drupal\block_content\Entity\BlockContentType;
+use Drupal\outside_in_test\Plugin\Block\OffCanvasFormAnnotationIsClassBlock;
+use Drupal\outside_in_test\Plugin\Block\OffCanvasFormAnnotationNoneBlock;
 use Drupal\user\Entity\Role;
 
 /**
@@ -36,6 +38,7 @@ class OutsideInBlockFormTest extends OutsideInJavascriptTestBase {
     // Add test module to override CSS pointer-events properties because they
     // cause test failures.
     'outside_in_test_css',
+    'outside_in_test',
   ];
 
   /**
@@ -110,6 +113,10 @@ class OutsideInBlockFormTest extends OutsideInJavascriptTestBase {
           // Fill out form, save the form.
           $page->fillField('settings[site_information][site_name]', $new_page_text);
           break;
+
+        case 'outside_in_test_class':
+          $web_assert->elementExists('css', '[data-drupal-selector="edit-settings-some-setting"]');
+          break;
       }
 
       if (isset($new_page_text)) {
@@ -176,6 +183,26 @@ class OutsideInBlockFormTest extends OutsideInJavascriptTestBase {
         'button_text' => 'Save Search form',
         'toolbar_item' => NULL,
       ],
+      // This is the functional JS test coverage accompanying
+      // \Drupal\Tests\outside_in\Functional\OutsideInTest::testPossibleAnnotations().
+      OffCanvasFormAnnotationIsClassBlock::class => [
+        'block_plugin' => 'outside_in_test_class',
+        'new_page_text' => NULL,
+        'element_selector' => 'span',
+        'label_selector' => NULL,
+        'button_text' => NULL,
+        'toolbar_item' => NULL,
+      ],
+      // This is the functional JS test coverage accompanying
+      // \Drupal\Tests\outside_in\Functional\OutsideInTest::testPossibleAnnotations().
+      OffCanvasFormAnnotationNoneBlock::class => [
+        'block_plugin' => 'outside_in_test_none',
+        'new_page_text' => NULL,
+        'element_selector' => 'span',
+        'label_selector' => NULL,
+        'button_text' => NULL,
+        'toolbar_item' => NULL,
+      ],
     ];
     return $blocks;
   }

core/modules/outside_in/tests/src/Unit/Access/BlockPluginHasOffCanvasFormAccessCheckTest.php

@@ -0,0 +1,96 @@
+<?php
+
+namespace Drupal\Tests\outside_in\Unit\Access;
+
+use Drupal\block\BlockInterface;
+use Drupal\Core\Access\AccessResultAllowed;
+use Drupal\Core\Access\AccessResultInterface;
+use Drupal\Core\Access\AccessResultNeutral;
+use Drupal\Core\Block\BlockPluginInterface;
+use Drupal\Core\Plugin\PluginWithFormsInterface;
+use Drupal\outside_in\Access\BlockPluginHasOffCanvasFormAccessCheck;
+use Drupal\Tests\UnitTestCase;
+use Prophecy\Argument;
+
+/**
+ * @coversDefaultClass \Drupal\outside_in\Access\BlockPluginHasOffCanvasFormAccessCheck
+ * @group outside_in
+ */
+class BlockPluginHasOffCanvasFormAccessCheckTest extends UnitTestCase {
+
+  /**
+   * @covers ::access
+   * @covers ::accessBlockPlugin
+   * @dataProvider providerTestAccess
+   */
+  public function testAccess($with_forms, array $plugin_definition, AccessResultInterface $expected_access_result) {
+    $block_plugin = $this->prophesize()->willImplement(BlockPluginInterface::class);
+
+    if ($with_forms) {
+      $block_plugin->willImplement(PluginWithFormsInterface::class);
+      $block_plugin->hasFormClass(Argument::type('string'))->will(function ($arguments) use ($plugin_definition) {
+        return !empty($plugin_definition['forms'][$arguments[0]]);
+      });
+    }
+
+    $block = $this->prophesize(BlockInterface::class);
+    $block->getPlugin()->willReturn($block_plugin->reveal());
+
+    $access_check = new BlockPluginHasOffCanvasFormAccessCheck();
+    $this->assertEquals($expected_access_result, $access_check->access($block->reveal()));
+    $this->assertEquals($expected_access_result, $access_check->accessBlockPlugin($block_plugin->reveal()));
+  }
+
+  /**
+   * Provides test data for ::testAccess().
+   */
+  public function providerTestAccess() {
+    $annotation_forms_off_canvas_class = [
+      'forms' => [
+        'off_canvas' => $this->randomMachineName(),
+      ],
+    ];
+    $annotation_forms_off_canvas_not_set = [];
+    $annotation_forms_off_canvas_false = [
+      'forms' => [
+        'off_canvas' => FALSE,
+      ],
+    ];
+    return [
+      'block plugin with forms, forms[off_canvas] set to class' => [
+        TRUE,
+        $annotation_forms_off_canvas_class,
+        new AccessResultAllowed(),
+      ],
+      'block plugin with forms, forms[off_canvas] not set' => [
+        TRUE,
+        $annotation_forms_off_canvas_not_set,
+        new AccessResultNeutral(),
+      ],
+      'block plugin with forms, forms[off_canvas] set to FALSE' => [
+        TRUE,
+        $annotation_forms_off_canvas_false,
+        new AccessResultNeutral(),
+      ],
+      // In practice, all block plugins extend BlockBase, which means they all
+      // implement PluginWithFormsInterface, but this may change in the future.
+      // This ensures Settings Tray will continue to work correctly.
+      'block plugin without forms, forms[off_canvas] set to class' => [
+        FALSE,
+        $annotation_forms_off_canvas_class,
+        new AccessResultNeutral(),
+      ],
+      'block plugin without forms, forms[off_canvas] not set' => [
+        FALSE,
+        $annotation_forms_off_canvas_not_set,
+        new AccessResultNeutral(),
+      ],
+      'block plugin without forms, forms[off_canvas] set to FALSE' => [
+        FALSE,
+        $annotation_forms_off_canvas_false,
+        new AccessResultNeutral(),
+      ],
+    ];
+  }
+
+}