The Page Title block's title behaves in a confusing way with Settings Tray and the Help block incorrectly has Settings Tray styling

Taking another look at this in the current state of the module I think I have a simple solution. The user who wants to edit the title should have a contextual lin k from the page title block "edit title".

We have another issue to change the 'quick edit' on the block to 'open settings tray' so that would give us three links in the page title block dropdown:

• Configure block
• Open settings tray
• Edit title

Configure block will go to the backend form, Open settings tray will show the settings for the block in-place (in this case the block title name which is still confusing...), and edit title should trigger quick edit mode with focus on the title field.

This removes the most annoying WTF of this experience which is that I need to go to the third contextual link down to edit the title of the node. 'Configure block' and 'Open settings tray' are still not crystal clear but at least with the third option the user can do some trial and error and get to where they need to be.

I think once #2782915: Standardize the behavior of links when Outside In editing mode is enabled lands(hopefully soon) this problem will be easier with better UX.

With that issue in "Edit Mode" if you click an area that is covered by QuickEdit then you the quick edit toolbar will be invoked.
We could extend this functionality to the Title block if it for a QuickEdit entity. The title in this block has the attribute data-quickedit-field-id if it is for a QuickEdit entity.

Then I think we should remove the Settings Tray functionality altogether for this block. It is a form without any real functionality. It has 2 elements. A checkbox for "Display title" that in all other blocks shows the title of the block. If you click it shows the page title twice. The textfield has even less functionality. It literally has no functionality. It updates the title of the block but because the way the checkbox behaves the textfield changes will never show on the site.

You would still be able to get the advanced block form where you could delete the block or changes its region if needed.

Thought a little more about it and no reason to wait for #2782915: Standardize the behavior of links when Outside In editing mode is enabled

Removing the page title block from the "Edit mode" makes sense regardless. Adding the trigger for QuickEdit could be a follow up issue.

This page remove the page title block from the Setting Tray module functionality.

Setting needs review.

Also couple question about my patch

1. +++ b/core/modules/outside_in/outside_in.module
   @@ -55,6 +60,12 @@ function outside_in_block_view_alter(array &$build) {
   +    // If this block should not be editable flag for contextual link removable.
   +    // The individual links are not available here to remove.
   +    $build['#contextual_links']['block']['metadata']['remove_outside_in'] = TRUE;
   

   I could find a better to flag that the block should not have the contextual link. In hook_contextual_links_view_alter we don't have plugin_id any more. We could check from the block id = "*_page_title" but that seems hacky and nothing would stop someone from using that block id pattern for another block.

2. +++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
   @@ -15,4 +15,15 @@
   +  public function isBlockEditable($plugin_id);
   

   Wasn't sure if this belonged in the interface but fits the description "Provides an interface for managing information related to Outside-In."

Tested in simplytest.me. Passes usability review.

1. +++ b/core/modules/outside_in/outside_in.module
   @@ -34,6 +34,11 @@ function outside_in_help($route_name, RouteMatchInterface $route_match) {
   +    if (!empty($element['#contextual_links']['block']['metadata']['remove_outside_in'])) {
   +      unset($element['#links']['outside-inblock-configure']);
   +      unset($items['outside_in.block_configure']);
   +      return;
   +    }
   
   @@ -55,6 +60,12 @@ function outside_in_block_view_alter(array &$build) {
   +  if (!\Drupal::service('outside_in.manager')->isBlockEditable($build['#plugin_id']) && isset($build['#contextual_links']['block'])) {
   +    // If this block should not be editable flag for contextual link removable.
   +    // The individual links are not available here to remove.
   +    $build['#contextual_links']['block']['metadata']['remove_outside_in'] = TRUE;
   +  }
   

   This needs some documentation. I needed to re-read this 5 times to figure out what's going on.

   The second hunk sets a "fake" bit of contextual link metadata.

   The first hunk detects the presence of that, and if so, unsets a particular contextual link.

2. +++ b/core/modules/outside_in/src/OutsideInManager.php
   @@ -63,4 +63,12 @@ public function isApplicable() {
   +    $non_editable_blocks = ['page_title_block', 'system_main_block'];
   

   Nit: $readonly_blocks probably makes more sense?

3. +++ b/core/modules/outside_in/src/OutsideInManager.php
   @@ -63,4 +63,12 @@ public function isApplicable() {
   +    return !in_array($plugin_id, $non_editable_blocks);
   

   Let's make this strict (in_array(…, …, TRUE)

4. +++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
   @@ -15,4 +15,15 @@
   +   * Checks whether a block should be covered by this module.
   ...
   +  public function isBlockEditable($plugin_id);
   

   Nit: comment does not match function name.

5. +++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
   @@ -15,4 +15,15 @@
   +   *   The plugin id for the block to check.
   

   Nit: s/id/ID/

6. +++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
   @@ -15,4 +15,15 @@
   +   *   TRUE if the block should the "Quick Edit" link provided by this module.
   

   Incomplete sentence.

The reason this is so surprisingly complex: the Contextual Links API was never designed for conditional contextual links. The Quick Edit module generates contextual links in JS (on the client side), so it can do this conditionally quite easily. This patch opts to do it on the server side. Hence it's fairly convoluted.

The approach in this patch can work. I think it's okay… but I can't help but wonder whether it wouldn't be a whole lot simpler to do this on the client side instead, much like Quick Edit.

@Wim Leers thanks for the review.

Yes I think you right it is complicated to do the removal on the server side.

This patch just sets an attribute if the link should be removed. Then the actual removal is done via Javascript.

From the review
1. Mostly removed now. I add @see comment to point the .js file and to the the module file. Hopefully clearer what is going on.
2. I like "non_editable" because it directly says what the user is restricted by.
3. fixed
4. fixed
5. fixed

1. +++ b/core/modules/outside_in/outside_in.module
   @@ -55,6 +55,14 @@ function outside_in_block_view_alter(array &$build) {
   +    // to remove the link on the client-side.
   

   s/client-side/client side/

2. +++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
   @@ -15,4 +15,16 @@
   +  public function isBlockEditable($plugin_id);
   

   Are you sure you want to add a block-specific method to the interface? Outside-In is not restricted to blocks AFAIK?

   Related: I'd strongly recommend doing what BigPipe did here too: #2835604: BigPipe provides functionality, not an API: mark all classes & interfaces @internal + #2835758: Remove BigPipeInterface and move all of its docs to the implementation.

re #14
1. fixed
2. Removed this from the interface and moved to a function in the module file.

On the related note I think that makes sense and will make follow up issues.

Ok adding test for the contextual link for the title block being excluded.

Also attaching a TEST_ONLY patch.

+++ b/core/modules/outside_in/outside_in.module
@@ -154,3 +164,18 @@ function outside_in_css_alter(&$css, AttachedAssetsInterface $assets) {
+  $non_editable_blocks = ['page_title_block', 'system_main_block'];

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -364,4 +364,16 @@ protected function pressToolbarEditButton() {
+   * Tests that title block does not have a outside_in contextual link.

Shouldn't we also test the system_main_block block?

Other than that, RTBC.

@Wim Leers thanks for the review!

Yes we should check the content block also. I also realized that not only should we be checking for the contextual link being excluded we should also make sure that the data-drupal-outsidein attribute is not added which would mark the the block as "editable".

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -368,12 +368,23 @@ protected function pressToolbarEditButton() {
+    // We do not need to place the main content block.
+    // It's ID will just be 'content'.

Well, that's just the fallback behavior in \Drupal\block\Plugin\DisplayVariant\BlockPageVariant::build(). In that case, it's not even an actual block.

So I'm not sure that this patch is correct. I think we should place the block. And we should have a separate test ensuring that you also don't get outside in functionality in case no "main content" block is placed.

So I'm not sure that this patch is correct. I think we should place the block.

Ok now placing both the blocks that should be excluded in the same way.

And we should have a separate test ensuring that you also don't get outside in functionality in case no "main content" block is placed.

I am not sure we need this \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks tests Settings Tray edit mode functionality with explicitly placing the main content block.

Needs re-roll because of #2862625: Rename offcanvas to two words in code and comments. and other recent commits

Re-rolling patch for test & review after rebase.

@RajeevK reroll looks great, Thanks!!!

Leaving as needs review because changes in #21(rerolled in #23 still need be reviewed.

+++ b/core/modules/outside_in/outside_in.module
@@ -163,3 +173,18 @@ function outside_in_css_alter(&$css, AttachedAssetsInterface $assets) {
+function _outside_in_is_block_editable($plugin_id) {

Perhaps add an explicit @internal?

Re-rolled #23 plus Wims' idea in #25

Need a re-roll. No changes.

Ok although the patch in #27 applied cleaning there were 3 problems.

1. It needed es6.js changes.
2. \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::setUp changed so there "Powered by Drupal" block is no longer being place there. We have to place it in testBlockExcluded() as an example of a non-excluded block - This caused the test fail.
3. The getBlockSelector() helper function was added so we should now use that in our test.

Ok so messed the interdiff on #32.

Here is the correct interdiff between #30 and #32.
You can see it has the 3 changes I described in #32

That all made sense. I was confused by the *.js vs *.es6.js changes at first, but they actually make sense :)

Just re-roll after #2882729: In off-canvas block form hide Title input unless it will be displayed and change label to Block Title

Thanks for the issue summary updates!

Talked about this some with @tedbow. Both @xjm and I felt a bit ooky about committing a new underscore-prefixed function for this. @tedbow explained that the reason this is wrapped in a function is to avoid inlining the same array twice. Makes sense. Another option which @tedbow thought of was, rather than _outside_in_is_block_editable(), add a method to OutsideInManagerInterface isBlockExcluded() because the phpDoc for the interface says “Provides an interface for managing information related to Outside-In.” which presumably would cover this. Marking needs work for that change.

This also addresses another my concerns which was "if contrib defines a block that wants to opt-out as well for whatever reason, what do they do?" Bearing in mind that we want to avoid making an explicit API, per #2894584: Settings Tray provides functionality, not an API: mark PHP and JS as internal.

I have created \Drupal\outside_in\OutsideInManagerInterface::isBlockEditable() and _outside_in_is_block_editable()

isBlockEditable is actually the exact same code that was in the earlier patch up till #13 and was reviewed by @Wim Leers

I removed in for the _outside_in_is_block_editable() in #15 because of his comment in #14.2

Are you sure you want to add a block-specific method to the interface? Outside-In is not restricted to blocks AFAIK?

Now after working with the Settings Tray module more and it almost stable I realize it's Edit only deals with Editing Block forms(plus other config added the form). So these seems like a good place for it.
The dialog tray itself could be used by other modules but Settings Tray Edit mode is always triggered through block contextual links and shows the Off-Canvas form for block plugin.

+++ b/core/modules/outside_in/outside_in.module
@@ -174,19 +174,3 @@ function outside_in_css_alter(&$css, AttachedAssetsInterface $assets) {
diff --git a/core/modules/outside_in/src/OutsideInManager.php b/core/modules/outside_in/src/OutsideInManager.php

diff --git a/core/modules/outside_in/src/OutsideInManager.php b/core/modules/outside_in/src/OutsideInManager.php
index 666a93837b..26bda75685 100644

index 666a93837b..26bda75685 100644
--- a/core/modules/outside_in/src/OutsideInManager.php

--- a/core/modules/outside_in/src/OutsideInManager.php
+++ b/core/modules/outside_in/src/OutsideInManager.php

+++ b/core/modules/outside_in/src/OutsideInManager.php
+++ b/core/modules/outside_in/src/OutsideInManager.php
@@ -63,4 +63,12 @@ public function isApplicable() {

@@ -63,4 +63,12 @@ public function isApplicable() {
     return $this->account->hasPermission('administer blocks') && !$is_admin_route && !$is_admin_demo_route;
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function isBlockEditable($plugin_id) {
+    $non_editable_blocks = ['page_title_block', 'system_main_block'];
+    return !in_array($plugin_id, $non_editable_blocks, TRUE);
+  }
+
 }
diff --git a/core/modules/outside_in/src/OutsideInManagerInterface.php b/core/modules/outside_in/src/OutsideInManagerInterface.php

diff --git a/core/modules/outside_in/src/OutsideInManagerInterface.php b/core/modules/outside_in/src/OutsideInManagerInterface.php
index 684adb3f0d..054074e4c8 100644

index 684adb3f0d..054074e4c8 100644
--- a/core/modules/outside_in/src/OutsideInManagerInterface.php

--- a/core/modules/outside_in/src/OutsideInManagerInterface.php
+++ b/core/modules/outside_in/src/OutsideInManagerInterface.php

+++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
+++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
@@ -15,4 +15,16 @@

@@ -15,4 +15,16 @@
    */
   public function isApplicable();
 
+  /**
+   * Checks whether a block is editable by this module.
+   *
+   * @param string $plugin_id
+   *   The plugin ID for the block to check.
+   *
+   * @return bool
+   *   TRUE if the block should have the "Quick Edit" link provided by this
+   *   module.
+   */
+  public function isBlockEditable($plugin_id);
+

I don't understand how this is an explicit API as described in #38.

This is

1. adding more to the interface that #2894584: Settings Tray provides functionality, not an API: mark PHP and JS as internal is planning to remove
2. still not extensible — only a single module is able to override this

Why not instead make this something that can be specified in the @Block annotation? That'd make for an elegant solution: if outside_in_block_alter() would be altering the annotation of every block plugin rather than the two it's currently modifying, and would then skip these two, then there would be nothing special anymore, and no need for an extra API.

So instead of:

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') {
      $definition['forms']['off_canvas'] = SystemMenuOffCanvasForm::class;
    }
  }
}

do this:

function outside_in_block_alter(&$definitions) {
  foreach ($definitions as &$definition) {
    switch ($definition['id']) {
      // Don't provide Outside-In capabilities for these two very special blocks.
      // @see \Drupal\Core\Block\MainContentBlockPluginInterface
      // @see \Drupal\Core\Block\TitleBlockPluginInterface
      case 'page_title_block':
      case 'system_main_block':
        continue;

      // Use specialized off-canvas forms when they're available.
      // @todo move these into the corresponding block plugin annotations when Outside In becomes stable.
      case 'system_menu_block':
        $definition['forms']['off_canvas'] = SystemMenuOffCanvasForm::class;
        break;
      case 'system_branding_block':
        $definition['forms']['off_canvas'] = SystemBrandingOffCanvasForm::class;
        break;

      // Otherwise fall back to the default off-canvas form.
      default:
        $definition['forms']['off_canvas'] = BlockEntityOffCanvasForm::class;
        break;
    }
  }
}

EDIT: this would probably even mean that you can remove all the JS changes in this patch.

I have updated the "outside_in_block_alter" function as mentioned in #41.
@Wim Leers, please update me about initial part of #41. We should remove these changes or you are saying something else?

@Wim Leers' proposal was sort of pseudocode

Indeed it was.

I'll review this once @tedbow rerolls this.

After some investigation I don't think the approach in #41 will work.

The default off_canvas form handler class is actually set in outside_in_entity_type_build() not outside_in_block_alter()
Settings it in outside_in_entity_type_build() is similar to setting in the annotation of core/modules/block/src/Entity/Block.php

Presumably we could remove it from outside_in_entity_type_build() and only set it per definition in outside_in_block_alter() but then we no longer would have a default off_canvas form handler for Block entity.

So if removed it from outside_in_entity_type_build() but had it only outside_in_block_alter() setting it for all blocks except the 2 we want to exclude then we would have no information on the form Entity type level for the block off_canvas form handler.

So this would affect:
\Drupal\Core\Entity\EntityTypeInterface::getFormClass()
\Drupal\Core\Entity\EntityTypeInterface::getHandlerClasses()

I have tried removing the logic in outside_in_entity_type_build() you get this error(in the logs) when trying to open block quick edit form

if (!$class = $this->getDefinition($entity_type, TRUE)->getFormClass($operation)) {
      throw new InvalidPluginDefinitionException($entity_type, sprintf('The "%s" entity type did not specify a "%s" form class.', $entity_type, $operation));
    }

From \Drupal\Core\Entity\EntityTypeManager::getFormObject().

Ok. here is patch that adds
$definition['outside_in_exclude'] = TRUE;
To the block plugin definitions that should be excluded.

it adds a api.php file to document how to do this on other blocks and a \Drupal\outside_in_test\Plugin\Block\ExcludedTestBlock which tests that this works.

Then every things is driven by this. It does add a new "_foo" function to the .module file but this is just to avoid duplicate logic.

Don't ever press option+command+W. It caused me to lose hours worth of time spent on a comment I'd been writing here. Hours. And if there's one thing I hate, it's repeating work. I got pretty close to kicking my computer.

#48: yep, I understand it now. class BlockEntityOffCanvasForm extends BlockForm, so it's an alternative for the default block form. SystemBrandingOffCanvasForm and SystemMenuOffCanvasForm are "plugin forms". BlockForm displays the "default" or built-in form of a block plugin. BlockEntityOffCanvasForm displays the "off canvas" form if it exists, and only those two exist.

#49: My key concern still stands: we're doing work to then later undo it again. It's a step in the right direction though. But I think it can be simpler: rather than adding yet another annotation, allow forms = { "off_canvas" = FALSE' }.

So here's what I propose.

Step 1: stop duplicating (commits 1+2+3)

outside_in_preprocess_block() in HEAD is duplicating the existing logic in system_block_view_system_main_block_alter(), and it's forgetting about doing the same for the Help block (because help_block_view_help_block_alter()).
They both do unset($build['#contextual_links']);.

So, just like #2784853: Determine when Outside In library should be loaded: piggyback on contextual_toolbar() piggybacked on what the Contextual Links module is already doing, I'm proposing to do the same here. Which means relying on the fact that the Contextual Links module adds the .contextual-region class. So rather than depending on the selector [data-drupal-outsidein="editable"] (in most of Settings Tray's JS) and .outside-in-editable (in all of Settings Tray's CSS), we rely on that too.

Commit 1 fixes the PHP, commit 2 updates the JS selectors, commit 3 updates the CSS selectors.

P.S.: there's no reason for the .outside-in-editable class — it should either all use the data- attribute, or the class, we shouldn't be adding both. And in fact, we shouldn't be adding either one.

Step 2: conditional contextual links (commit 4)

The reason we're doing all this, is because Settings Tray has a need for conditional contextual links. Rather than reinventing how that is supposed to work (which is what Settings Tray is doing today), it can just rely on the already established mechanisms, either:

1. client-side: Quick Edit, which means no contextual link defined in a *.links.contextual.yml file — see core/modules/quickedit/js/views/ContextualLinkView.es6.js
2. server-side: 403 response for the entity.block.off_canvas_form route for the contextual link would mean the contextual link would be inaccessible when appropriate

Since Settings Tray is already defining a contextual links in outside_in.links.contextual.yml, I'm going with the server-side approach. Proof that this works can be found in the tests at \Drupal\Tests\contextual\FunctionalJavascript\ContextualLinksTest
and \Drupal\Tests\Core\Menu\ContextualLinkManagerTest::testGetContextualLinksArrayByGroupAccessCheck().

Includes test coverage.

Step 3: marking page_title_block to not have an 'off_canvas' form (commit 5)

This builds upon the cleaned up infrastructure to solve what this issue is actually about very simply/elegantly.

Doesn't need extra test coverage because relies on Contextual Links module infrastructure (see previous step).

Step 4: setting the .outside-in-editable class in JS (NOT YET DONE)

Right now we're unconditionally setting this class, which means that the Page Title block still is clickable. We need to have Outside In's JS add this class to the closest .contextual-region DOM node if the outside_in.block_configure link is present.

Conclusion

End result:

1. maximally uses Contextual Links module's infrastructure, which Settings Tray is building upon in the first place
2. therefore reduces amount of code
3. is still wholly controllable via a single declarative thing: forms[off_canvas] in a Block plugin annotation

I think this is the least confusing API, and also the smallest possible API.

In case it helps, these are my notes to capture the high-level reasoning:

specify "off_canvas" form plugin for all block plugins by default, i.e. set it to equal the default block plugin
EXCEPT for the page title one, there we set it to FALSE
         RESULT: declarative
Change \Drupal\outside_in\Block\BlockEntityOffCanvasForm::getPluginForm() from <code>return $this->pluginFormFactory->createInstance($block, 'off_canvas', 'configure'); to return $this->pluginFormFactory->createInstance($block, 'off_canvas'), i.e. no automatic fallback
        RESULT: off_canvas=FALSE can actually result in not falling back to a fallback form
Add custom access check to entity.block.off_canvas_form route. This calls logic similar to BlockEntityOffCanvasForm::getPluginForm(). If a InvalidPluginDefinitionException exception is thrown: access forbidden, otherwise access allowed
        RESULT: 403 => no Settings Tray contextual link => no Settings Tray interactivity

Whoops submitted before I commented.

@Wim Leers yes I like the idea of throwing a 403 and then the Contextual link not being produced at all.

This all looks very good!

P.S.: there's no reason for the .outside-in-editable class — it should either all use the data- attribute, or the class, we shouldn't be adding both. And in fact, we shouldn't be adding either one.

I thought we tried to drive CSS off classes and JS functionality off data attributes.

So a theme could still alter the class names and the JS functionality would still work. Like maybe they already have general highlighted-area the would rather use instead but the JS functionality would still work because it all based of data attributes(except Contextual module CSS classes because it is not using data attributes)

We already changed to using this method in previous issues.

RE

Step 4: setting the .outside-in-editable class in JS (NOT YET DONE)

Couldn't we just do this by not adding the class in the first place by checking if plugin doesn't have an 'off_canvas' form?
Seems much simplier than relying on inspecting the DOM in JS.
Reading https://www.drupal.org/core/d8-bc-policy it seems doing it on the server side and not relying on DOM or CSS classes at all is more future proof.

Uploading a patch for this.

In the terse, re-typed version of #50, I failed to mention again that the 403 idea was in fact proposed by @tedbow in a call we had yesterday about this issue :)

So a theme could still alter the class names and the JS functionality would still work.

Well, contextual_preprocess() sets .contextual-region. It's a class that's "functional" and not "aestethic". Themes don't modify this class. So you can choose either a class or a data- attribute. The latter is perhaps the more "modern" approach, but there's no clear standards for this. Whichever you prefer :)

Couldn't we just do this by not adding the class in the first place by checking if plugin doesn't have an 'off_canvas' form?

But then we're still duplicating other logic. We're doing the same calculations in two places. In one case, to generate the contextual links (= markup), and then in this case to determine whether to set a certain class (= markup).

It's better to derive additional markup from existing markup. Besides, if we'd …………………………

Seems much simplier than relying on inspecting the DOM in JS.

Well, now that you mention that: there's another issue we need to open for Settings Tray: it's doing its JS magic to transform the existing Edit button + toolbar always, even on pages where Drupal.contextual.collection is empty, i.e. on pages where there are zero contextual links.

Reading https://www.drupal.org/core/d8-bc-policy it seems doing it on the server side and not relying on DOM or CSS classes at all is more future proof.

Why/where? https://www.drupal.org/core/d8-bc-policy#themes is referring to themes, and CSS for themes. It's not referring to attributes. Besides, #2894584: Settings Tray provides functionality, not an API: mark PHP and JS as internal is explicitly marking everything as internal. The end goal (which this issue helps to achieve) is that there is only one single API: forms = { "off_canvas" = "FQCN" } on block plugin annotations!

The interdiff I uploaded for 51 was wrong

I like how #51/#55 is a pragmatic solution that minimizes change, yet still helps settle on the forms[off_canvas] annotation as the single source of truth.

Improving this further can totally be done in a follow-up. OTOH, it means that we still are duplicating logic. #51/#55 duplicates in outside_in_preprocess_block() what \Drupal\outside_in\Access\BlockPluginHasOffCanvasFormAccessCheck does. And for a not-so-clear reason.

Ted mentioned in chat:

but also doing it on clientside won’t that let to more pontential flickr if there was alot going on in JS. the page can start in edit mode

But Settings Tray's JS is already waiting for Contextual Links JS. To which Ted responded:

well but if you started in Edit Mode. and we did it in the JS then the class would NOT be on all blocks from the start. then we wait for contextual links to load. then we can added the class. So the toolbar Edit mode change would be there from the start. then highlight blocks don’t show up until contextual links load. As opposed to doing on server side. Toolbar and block formatting from the start.

While not yet proven, that is a clear reason. D8's Toolbar definitely suffered from this problem until very recently: #2542050: Toolbar implementation creates super annoying re-rendering..

But if we make the change that #51/#55 makes, it's overriding commit 1 from #50, which means we can also revert commits 2 and 3. So the entire "step 1: stop duplicating" from #50 is then undone. I think that's fine though: this patch then becomes smaller, its scope becomes tighter, and this patch can then add documentation as to why it's being done this way. It's an implementation detail anyway — it can be improved later!

So, embracing #51/#55, reverting commits 1+2+3 from #50.

D'oh, that doesn't work either, because that means the system_block_view_system_main_block_alter() + help_block_view_help_block_alter() hooks aren't respected! I forgot about that for a moment. Which means that for example the main content block shows up as a Settings Tray-clickable region :(
So… let's not do that. The .contextual-region class that was added to the selectors in commits 2+3 is set on the server side anyway (by contextual_preprocess()), so that can't cause flicker. The only thing that can cause flicker, is setting .outside-in-editable and [data-drupal-outsidein] in JS, because that'd have to wait on Contextual Links' JS.

Which means that all that remains to be done here, is adding documentation to outside_in_preprocess_block(), to document why it's done here on the server side.

I'm comfortable with this patch being committed. But I can't RTBC this, I did most of the work. I think @tedbow should be the one to RTBC this eventually.

I did spot one mistake in the ES6 vs ES5 JS file. yarn run watch:js is kinda brittle sadly:

/Users/wim.leers/Work/d8/core/scripts/js/compile.js:21
        throw new Error(err);
        ^

Error: SyntaxError: modules/outside_in/js/outside_in.es6.js: Unexpected token (31:4)
    at babel.transformFile (/Users/wim.leers/Work/d8/core/scripts/js/compile.js:21:15)
    at /Users/wim.leers/Work/d8/core/node_modules/babel-core/lib/api/node.js:141:7
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:416:3)

This is how the ES6 vs ES5 files got out of sync.

1. +++ b/core/modules/outside_in/outside_in.module
   @@ -100,8 +100,16 @@ function outside_in_entity_type_build(array &$entity_types) {
   +  // Only blocks that have an off_canvas form will have a "Quick Edit" link. We
   +  // 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 means that it will be impossible to show Settings Tray's clickable
   +  // regions immediately when the page loads. When latency is high, this will
   +  // cause flicker. Therefore, for now, we choose to duplicate some logic to
   +  // guarantee a smooth experience.
   +  // This is an implementation detail that may change in the future.
   +  // @see \Drupal\outside_in\Access\BlockPluginHasOffCanvasFormAccessCheck
   +  if (\Drupal::service('plugin.manager.block')->createInstance($variables['plugin_id'])->hasFormClass('off_canvas')) {
   

   Thanks for the detailed comment here.

2. +++ b/core/modules/outside_in/outside_in.module
   @@ -139,17 +147,40 @@ function outside_in_toolbar_alter(&$items) {
   +      // @todo move these into the corresponding block plugin annotations when Settings Tray becomes stable.
   

   Create the follow up issue #2896356: Move 'settings_tray' forms out of Settings Tray and into respective modules and annotations we now need to update the todo

3. +++ b/core/modules/outside_in/outside_in.module
   @@ -139,17 +147,40 @@ function outside_in_toolbar_alter(&$items) {
   +      // 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.
   +      case 'page_title_block':
   +        $definition['forms']['off_canvas'] = FALSE;
   +        break;
   

   Should we have @todo and an issue to move this to the PageTitleBlock annotation?

1. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -498,4 +499,33 @@ protected function isLabelInputVisible() {
   +  /**
   +   * Tests that title block does not have a outside_in contextual link.
   +   */
   +  public function testBlockExcluded() {
   

   This test function was removed. Should we add it and the corresponding test module back in. We don't have functional test that proves you can exclude block though we are adding that functionality.

2. +++ b/core/modules/outside_in/tests/modules/outside_in_test/src/Plugin/Block/ExcludedTestBlock.php
   @@ -0,0 +1,32 @@
   + *   outside_in_exclude = true
   

   If we did add it back we would need to change this to

   forms {
    off_canvas = false,
   }
   

   Or something like that. Otherwise the test should still pass I think

1. +++ b/core/modules/outside_in/outside_in.module
   @@ -100,8 +100,16 @@ function outside_in_entity_type_build(array &$entity_types) {
   +  if (\Drupal::service('plugin.manager.block')->createInstance($variables['plugin_id'])->hasFormClass('off_canvas')) {
   
   +++ b/core/modules/outside_in/src/Access/BlockPluginHasOffCanvasFormAccessCheck.php
   @@ -0,0 +1,29 @@
   +    return AccessResult::allowedIf($block_plugin->hasFormClass('off_canvas'));
   

   Now that I look again, there's even more robust fallback code in \Drupal\Core\Plugin\PluginFormFactory::createInstance(). These two different has FormClass checks are different, and that is odd. Not the fault of this code though...

2. +++ b/core/modules/outside_in/outside_in.module
   @@ -139,17 +147,40 @@ function outside_in_toolbar_alter(&$items) {
   +      // Otherwise fall back to the built-in form for the block plugin.
   +      default:
   +        $definition['forms']['off_canvas'] = $definition['class'];
   +        break;
   

   This is an interesting change from HEAD.

   \Drupal\Core\Plugin\PluginWithFormsTrait::getFormClass() already provides it's own fallback mechanism, and in HEAD that is relied upon. This changes it so that we explicitly declare the "configure" form as used by "off_canvas" operation.

   This isn't bad, but possibly slightly confusing.
   That said, the entire concept of multiple plugin forms was added to core to support off_canvas.

#58:

2. Thanks! Will update.
3. Great point, will do.
4+5: yep, we should, and will do.

#59.2: Yep, I am proposing this change because rather than relying on run-time logic that might change, I think it's clearer to rely on metadata (declarative) instead. Are you +1, or do you have doubts about that?

+1, but can you open a follow-up to discuss the differences and a way to resolve them? As the only implementation of this, we need to be clear about how others should use multiple forms

#58: All done. Brought over test coverage & API docs from #49, and adjusted it quite a bit, which makes you still eligible for review/RTBC. Also added test blocks for the other two possible kinds of annotations. Big interdiff because big test coverage expansion.

Fixed nits. Should also cause the patch to pass tests again.

#61: Done: #2896952: Discuss/resolve/document differences in fallback handling between PluginWithFormsTrait::getFormClass() and outside_in_toolbar_alter(). I hope that is a good title + issue summary.

1. +++ b/core/modules/outside_in/outside_in.api.php
   @@ -0,0 +1,65 @@
   + * The goal of the Settings Tray module is to make administering blocks easier,
   + * and therefore to make the site building experience faster and more pleasant.
   + *
   + * It achieves this by building on the infrastructure that the Contextual
   + * Links module provides: Settings Tray provides a "Quick Edit" contextual link
   + * for blocks. Clicking this contextual link opens the Settings Tray, which
   + * allows to change the contents of the block (not the visibility conditions).
   + * This means blocks can be modified without leaving the page.
   + *
   + * The Settings Tray uses a variant of a standard dialog: an off-canvas dialog.
   + *
   

   Not sure if we should include such extensive description of the module in this patch or add this a follow. I agree it is good idea to include the parts in the this patch that affect the excluding of the blocks.

   For instances there are other things besides the visibility conditions that aren't shown.

2. +++ b/core/modules/outside_in/outside_in.api.php
   @@ -0,0 +1,65 @@
   + * However, many blocks would benefit from a tailored form which limits the form
   + * to only the form items that affect the content of the rendered block: this
   + * results in a better experience.
   

   The tailored experience at least in the 2 forms that are provided by this module is not about "limits the form" but actually adding relevant config to the form.

3. +++ b/core/modules/outside_in/outside_in.module
   @@ -100,8 +100,16 @@ function outside_in_entity_type_build(array &$entity_types) {
   +  // @see \Drupal\outside_in\Access\BlockPluginHasOffCanvasFormAccessCheck
   +  if (\Drupal::service('plugin.manager.block')->createInstance($variables['plugin_id'])->hasFormClass('off_canvas')) {
   
   +++ b/core/modules/outside_in/src/Access/BlockPluginHasOffCanvasFormAccessCheck.php
   @@ -0,0 +1,29 @@
   +    return AccessResult::allowedIf($block_plugin->hasFormClass('off_canvas'));
   

   This 2 lines do the same check. We could add back a service(without an interface this time)

   With

   public function isSettingsTraySupportedBlockPlugin(BlockPluginInterface $block_plugin) {
   return $block_plugin->hasFormClass('off_canvas');
   }
   

   Then of course we would want to rename BlockPluginHasOffCanvasFormAccessCheck to BlockPluginSupportSettingsTrayAccessCheck

   Actually since BlockPluginHasOffCanvasFormAccessCheck is already a service could we just call it in outside_in_preprocess_block directly?
   I know it implements AccessInterface but is there any reason we can't add another public function, isSettingsTraySupportedBlockPlugin, that checks access also but with plugin id or BlockPluginInterface instead of a BlockInterface. Especially considering #2266817: Deprecate empty AccessInterface and remove usages

4. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -176,6 +181,24 @@ public function providerTestBlocks() {
   +        'button_text' => 'Save Settings Tray test block: forms[off_canvas]=class',
   ...
   +        'button_text' => 'Save Settings Tray test block: forms[off_canvas] is not specified',
   

   'button_text' here can be set to NULL for each of these. $button_text in testBlocks() is only used if 'new_page_text' is specified.

   Actually 'label_selector' should also be set to NULL. Neither of these tests case will save the form and check for updates on the page after the form is saved.

   (I realize this is actually true for 'block-search' test case but that is out of scope for this issue)

#66:

1. Agreed, removed. Created #2897272: Fix module description, hook_help(), and document module scope in *.api.php file.
2. Reworded.
3. Done.
4. Done.

+++ b/core/modules/outside_in/src/Access/BlockPluginHasOffCanvasFormAccessCheck.php
@@ -20,9 +21,22 @@ class BlockPluginHasOffCanvasFormAccessCheck implements AccessInterface {
-  public function access(BlockInterface $block) {
+  public function accessBlock(BlockInterface $block) {

You do actually have to have a callable "access" method @see \Drupal\Core\Access\CheckProvider::loadCheck
the errors for this test

Drupal\Core\Access\AccessException: Access check method access in service access_check.outside_in.block.off_canvas_form must be callable.

So I am not sure if we leave "access" the way it was and still add accessBlockPlugin(). that would work

I implemented #66.3 incorrectly anyway: I forgot to update outside_in_preprocess_block()!

Regarding #69: #2266817: Deprecate empty AccessInterface and remove usages is pretty misleading then, seems that AccessInterface does have a purpose… :( Done.

+++ b/core/modules/outside_in/src/Access/BlockPluginHasOffCanvasFormAccessCheck.php
@@ -9,6 +9,8 @@
+ *
+ * @internal

I also added this in #70, I realized that since #2894584: Settings Tray provides functionality, not an API: mark PHP and JS as internal has landed, we should mark any new classes @internal too.

Rerolling…

+++ b/core/modules/outside_in/src/Access/BlockPluginHasOffCanvasFormAccessCheck.php
@@ -0,0 +1,48 @@
+  public function accessBlockPlugin(BlockPluginInterface $block_plugin) {
+    return AccessResult::allowedIf($block_plugin->hasFormClass('off_canvas'));
...
diff --git a/core/modules/outside_in/tests/modules/outside_in_test/outside_in_test.info.yml b/core/modules/outside_in/tests/modules/outside_in_test/outside_in_test.info.yml

So just looking at \Drupal\outside_in\Block\BlockEntityOffCanvasForm::getPluginForm I realized that BlockPluginInterface does not implement PluginWithFormsInterface so it is not guaranteed to have hasFormClass().

Should we check for instanceof PluginWithFormsInterface like \Drupal\outside_in\Block\BlockEntityOffCanvasForm::getPluginForm does?

The disallow access if it doesn't implement the interface.

Otherwise looks done!

So just looking at \Drupal\outside_in\Block\BlockEntityOffCanvasForm::getPluginForm I realized that BlockPluginInterface does not implement PluginWithFormsInterface so it is not guaranteed to have hasFormClass().

Interesting point! However … it's literally impossible to implement blocks by implementing the interface, you must extend \Drupal\Core\Block\BlockBase, which does always implement that interface. But doing what you said prepares us better for a smooth future without unpleasant surprises, so let's do it!

Expanded test coverage.

+++ b/core/modules/outside_in/src/Access/BlockPluginHasOffCanvasFormAccessCheck.php
similarity index 26%
rename from core/modules/outside_in/tests/src/Unit/Access/BlockPluginHasOffCanvasFormAccessCheckTest.php

rename from core/modules/outside_in/tests/src/Unit/Access/BlockPluginHasOffCanvasFormAccessCheckTest.php
rename to core/modules/outside_in/tests/src/Unit/Access/BlockPluginHasOffCanvasFormAccessCheckTestFormsWithForms.php

Ugh this rename should never have happened. PHPStorm--

Also fixing coding standards violations that I added in #76.

@Wim Leers thanks for these last changes. RTBC! 🎉

As #78 failed to apply.
I am submitting patch again after updating it.

@pk188 Thanks!

#78 failed because Settings Tray patches have been committed in the mean time (yay!).

I diffed #78 and #81. The reasons the size is different (31 vs 29 KB):

1. core/modules/outside_in/tests/modules/outside_in_test/outside_in_test.info.yml has already been added in another issue — so this patch no longer needs to add it
2. #78 includes a diffstat at the top of the file #81 does not

The patches are functionally identical. Therefore back to RTBC.

Thanks! @Wim Leers for reviewing the patch.
And yes you are right.

#78 failed because Settings Tray patches have been committed in the mean time (yay!).

As the patch was not applying.
So, I rerolled it.

I manually diffed #81 and #86, I can confirm it's a straight rebase, no other changes. So back to RTBC.

Fixed according to #88.

For #91
1.done
2.done
3.pending
4.done
5.done
6.done
7.done
8.pending
9…
10.pending
11.pending
12.the comment was removed but css and js changes wasn’t done
#92
1.done
2.done
3.pending (11 by #91)
4.done
5.done
6.done
7.done
8.pending
9.done
10.done
11.nit: & done, non-nit 12 is the answer

#91.3 Added an example for this 1. For the 1 before this limiting the form items actually happens in BlockEntityOffCanvasForm which is not used for the block plugin form but for the block entity form. So I am not sure if the comment actually makes sense here because this is talking about the block plugin form.
8. This happens in outside_in_block_alter() added a @see link
10. Fixed comment.
This would happened because contextual links aren't actually on the page when the page loads so we would have to wait till they are to tell which blocks would need the class. Contextual links html is actually stored in the localstorage of the user's browser. When when new blocks are placed or visible for the first time(when could mean multiple blocks at once) each block must make a ajax call back to get the rendered contextual links html. When the page was already in "Edit Mode" and the page first loads then the existing blocks that had the contextual links html stored on the client side would have the class from this module applied quickly but new blocks would have to make individual ajax calls back and it could not be determined if each block should get the class until the ajax call came back.

So the existing blocks would have "editable" look at first page load but new blocks would get the "editable" look 1 at a time as ajax calls come back.
11. Removing "for now" because I don't actually think this is not the desired logic. This limitation of how Contextual links work.
12. I don't CSS and JS should change see 11

#92.1 changed the comment to say " by Settings Tray in the off-canvas dialog"
3. Removing this comment about duplicated logic. Earlier in the patch we were duplicating the logic inside BlockPluginHasOffCanvasFormAccessCheck(or what is was previously named) but now since we are actually getting the service itself we are not duplicating the logic.
7. Just changed comment to use "Settings Tray" instead of "Outside In"
8. The access checker is used in the route "entity.block.off_canvas_form" so it will not be removed.
I am removing the @todo comment about removing the code because I don't think it needs to be. @see my comment above about #91.11

chatted with @xjm she pointed there were out of scope change regarding the css selector .contextual-region

Removed

Just to clear for any further reviewers

I have checked @Adita's review and all the changes look unless I noted a change in #94

So together with #93 to #95 we I think have covered @xjm reviews in #91 and #92

1. +++ b/core/modules/outside_in/js/outside_in.es6.js
   @@ -142,7 +142,7 @@
   -      $editables = $('[data-drupal-outsidein="editable"]').once('outsidein');
   +      $editables = $('.contextual-region[data-drupal-outsidein="editable"]').once('outsidein');
   
   @@ -176,7 +176,7 @@
   -      $editables = $('[data-drupal-outsidein="editable"]').removeOnce('outsidein');
   +      $editables = $('.contextual-region[data-drupal-outsidein="editable"]').removeOnce('outsidein');
   

   Can this change be removed, similar to the removal of the CSS changes?

2. +++ b/core/modules/outside_in/tests/src/Unit/Access/BlockPluginHasOffCanvasFormAccessCheckTest.php
   @@ -0,0 +1,100 @@
   +class BlockPluginHasOffCanvasFormAccessCheckTest extends UnitTestCase {
   

   Rewriting this test class to not need the empty implementations.

#97.1 Look like in #95 I removed these changes from outside_in.js but not outside_in.es6.js version. That was a mistake. Removing. This should effect tests because in #95 they were already removed from outside_in.js which is what is actually loaded.

#97.2 look good.

Looks great, thanks!

#100
1) Done
2) Done
3) Done
4) Done
5) Done
6) Done
7) Done
8) Done
9) Skipped (not actionable?)
10) Done
11) Done
12) Done
13) Done
14) Done
15) Done

#101
Skipped

#102
Done

As all of those changes were to docs nits, I think I can safely RTBC this.

#92.1 was not actually fixed by #93, even though it claimed it did. Nor in #94, where @tedbow wrote changed the comment to say " by Settings Tray in the off-canvas dialog", but I don't see that in the actual patch. We kept using the off_canvas name. That's fine while this module is in alpha, but we either need to explain why we keep using that name, or we need to change it, before reaching beta.
(I share @xjm's concern, but didn't raise it here, because it's out of scope to change here.)

So, opened a new issue for that: #2904134: Settings Tray uses the off-canvas dialog type, but "off_canvas" is not an accurate form plugin name, "settings_tray" is.

#95's reversal of .contextual-region CSS changes caused a regression and #98's related reversal in JS makes this a definite regression. See #50. Nope, it doesn't cause a regression, because of our decision to duplicate some of the logic to avoid flicker. +1 for this change then!

I missed #2903198: Use more specific CSS when attaching Settings Tray links to contextual links because it wasn't added as a related issue. Fixed that.

Note that per #111, I don't think we need that follow-up at all.

Changing to new settings_tray.module component. @drpal thanks for script help! :)