Problem/Motivation

The Outside In prototype introduced in #2753941: [Experimental] Create Outside In module MVP to provide block configuration in Off-Canvas tray and expand site edit mode allows a user to configure page elements in context. Something very frustating happens when trying to edit the page title.

This is partly an existing usability issue with the page title block (the title and display title make no sense for that block), but it's made much worse with the introduction of a module that lets me click on those words and get a sidebar to do everything but change those words. There are several problems:

  1. The "Display title" checkbox makes no sense for this block.
  2. The "Display title" checkbox does not display the thing in the title box for this block.
  3. The contents of the title box are ignored.
  4. When I click on this, I don't actually want to configure this block at all. I want to edit the node title!

Proposed resolution

Since the nothing you can do to the page title block will actually have any visible effect it should be not get the "Quick Edit" contextual link provided by this module.

  1. Add new function _outside_in_is_block_editable() that will be called to determine if a block should exclude from getting the Quick Edit link. This sets the Main Content(which was already excluded) and Page Title block.
  2. Set an attribute data-outside-in-exclude on blocks should not include a Quick Edit link which will remove the link via Javascript. (It is very complicated to remove contextual links on the server side see @Wim Leers' comment in #12)

Remaining tasks

None

User interface changes

No "Quick Edit" contextual link for the Page Title block.
User doesn't get access of the Off-canvas form for the Page Title block which would have no effect anyways.

API changes

None

Data model changes

None

Files: 
CommentFileSizeAuthor
#64 2782891-64.patch28.04 KBWim Leers
#55 interdiff-2782891-50-51.txt1.11 KBtedbow
#51 2782891-51.patch13.3 KBtedbow
#50 2782891-50.patch13.82 KBWim Leers
#50 all-commits.patch15.99 KBWim Leers
#49 2782891-49.patch8.92 KBtedbow
#42 interdiff-40-42.txt1.67 KBpk188
#42 2782891-42.patch8.55 KBpk188
#40 2782891-40.patch7.06 KBtedbow
#40 interdiff-35-40.txt3.37 KBtedbow
#35 2782891-32-reroll.patch6.15 KBtedbow
#33 interdiff-2782891-30-32.txt3.43 KBtedbow
#32 2782891-32.patch6.09 KBtedbow
#32 interdiff-2782891-27-32.txt0 bytestedbow
#30 2782891-30.patch5.07 KBtedbow
#27 2782891-27-reroll.patch5.14 KBtedbow
#23 2782891-23.patch5.18 KBRajeevK
#21 2782891-21.patch5.11 KBtedbow
#21 interdiff-19-21.txt2.16 KBtedbow
#19 2782891-19.patch4.91 KBtedbow
#19 interdiff-16-19.txt1.86 KBtedbow
#16 2782891-16.patch4.3 KBtedbow
#16 2782891-16-TEST_ONLY.patch1.02 KBtedbow
#16 interdiff-13-16.txt1.02 KBtedbow
#15 2782891-15.patch3.28 KBtedbow
#15 interdiff-13-14.txt3.48 KBtedbow
#13 interdiff-9-13.txt3.99 KBtedbow
#13 2782891-13.patch4.04 KBtedbow
#8 2782891-8.patch3.45 KBtedbow
display_title_on_title_block_makes_no_sense.png163.56 KBxjm

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm’s picture

Issue summary: View changes
tedbow’s picture

Component: quickedit.module » outside_in.module
tkoleary’s picture

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.

tkoleary’s picture

Issue tags: +Usability
tedbow’s picture

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.

tedbow’s picture

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.

tedbow’s picture

Status: Active » Needs review

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

tkoleary’s picture

Tested in simplytest.me. Passes usability review.

Version: 8.3.x-dev » 8.4.x-dev

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

Wim Leers’s picture

Status: Needs review » Needs work
  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.

tedbow’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript
FileSize
4.04 KB
3.99 KB

@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

Wim Leers’s picture

  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.

tedbow’s picture

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.

tedbow’s picture

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

Also attaching a TEST_ONLY patch.

The last submitted patch, 16: 2782891-16-TEST_ONLY.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Needs work
+++ 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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
4.91 KB

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

Wim Leers’s picture

Status: Needs review » Needs work
+++ 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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
5.11 KB

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.

tedbow’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

RajeevK’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.18 KB

Re-rolling patch for test & review after rebase.

tedbow’s picture

@RajeevK reroll looks great, Thanks!!!

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: 2782891-23.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
5.14 KB

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: 2782891-27-reroll.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.07 KB

Need a re-roll. No changes.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: 2782891-30.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
0 bytes
6.09 KB

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.
tedbow’s picture

FileSize
3.43 KB

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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

tedbow’s picture

Issue summary: View changes
tedbow’s picture

Issue summary: View changes
webchick’s picture

Status: Reviewed & tested by the community » Needs work

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 everything (PHP+JS+CSS+HTML) as @internal.

xjm’s picture

Edit: Wrong issue, too many tabs, etc.

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.37 KB
7.06 KB

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.

Wim Leers’s picture

Status: Needs review » Needs work
+++ 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 everything (PHP+JS+CSS+HTML) 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.

pk188’s picture

Status: Needs work » Needs review
FileSize
8.55 KB
1.67 KB

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?

Status: Needs review » Needs work

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

xjm’s picture

  1. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -9,6 +9,8 @@
    +  const excludedLinksSelector = '[data-outside-in-exclude] [data-outside-in-edit]';
    +
    
    @@ -21,6 +23,10 @@
    +    // Remove contextual links that should be excluded.
    +    // @see outside_in_block_view_alter().
    +    $(excludedLinksSelector).remove();
    
    +++ b/core/modules/outside_in/js/outside_in.js
    @@ -11,8 +11,11 @@
    +  var excludedLinksSelector = '[data-outside-in-exclude] [data-outside-in-edit]';
    ...
    +    $(excludedLinksSelector).remove();
    +
    
    +++ b/core/modules/outside_in/outside_in.module
    @@ -64,6 +64,14 @@ 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 set an attribute which will be used
    +    // to remove the link on the client side.
    +    // The individual links are not available here to remove.
    +    // @see outside_in.js
    +    $build['#attributes']['data-outside-in-exclude'] = TRUE;
    +  }
    
    @@ -102,7 +110,9 @@ function outside_in_entity_type_build(array &$entity_types) {
    -  if ($variables['plugin_id'] !== 'system_main_block' && \Drupal::service('outside_in.manager')->isApplicable()) {
    +  /** @var \Drupal\outside_in\OutsideInManagerInterface $outside_in_manager */
    +  $outside_in_manager = \Drupal::service('outside_in.manager');
    +  if (\Drupal::service('outside_in.manager')->isBlockEditable($variables['plugin_id']) && $outside_in_manager->isApplicable()) {
    
    +++ b/core/modules/outside_in/src/OutsideInManager.php
    @@ -63,4 +63,12 @@ public function isApplicable() {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function isBlockEditable($plugin_id) {
    +    $non_editable_blocks = ['page_title_block', 'system_main_block'];
    +    return !in_array($plugin_id, $non_editable_blocks, TRUE);
    +  }
    +
    
    +++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
    @@ -15,4 +15,16 @@
    +  /**
    +   * 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);
    +
    

    All of these changes would need to be removed from the patch for Wim's approach.

  2. +++ b/core/modules/outside_in/outside_in.module
    @@ -138,15 +148,28 @@ function outside_in_toolbar_alter(&$items) {
    -  // Since menu blocks use derivatives, check the definition ID instead of
    -  // relying on the plugin ID.
    

    This comment doesn't need to be removed.

However, it looks like the test is failing; at first I assumed it was because the patch in #42 is incomplete, but on reading the test I couldn't say for sure that any of the now-dead code was causing the problem.

xjm’s picture

On #44, not actually sure on the contextual links changes. Maybe that's what's failing? But the goal of the approach is to remove the need for the API additions.

xjm’s picture

Ah here we go; this is the issue:
seText: The website encountered an unexpected error. Please try again later.Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException: The "system_powered_by_block" plugin did not specify a valid "off_canvas" form class, must implement \Drupal\Core\Plugin\PluginFormInterface in Drupal\Core\Plugin\PluginFormFactory->createInstance() (line 57 of core/lib/Drupal/Core/Plugin/PluginFormFactory.php).

@Wim Leers' proposal was sort of pseudocode; maybe there is a small bug in the alter hook?

Wim Leers’s picture

@Wim Leers' proposal was sort of pseudocode

Indeed it was.

I'll review this once @tedbow rerolls this.

tedbow’s picture

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

tedbow’s picture

Status: Needs work » Needs review
FileSize
8.92 KB

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.

Wim Leers’s picture

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
tedbow’s picture

tedbow’s picture

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.

Wim Leers’s picture

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 :)

Wim Leers’s picture

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 everything (PHP+JS+CSS+HTML) 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!

tedbow’s picture

FileSize
1.11 KB

The interdiff I uploaded for 51 was wrong

Wim Leers’s picture

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.

Wim Leers’s picture

FileSize
825 bytes
13.87 KB

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.

tedbow’s picture

Status: Needs review » Needs work
  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 'off_canvas' forms out of Settings Tray and into respective modules 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

tim.plunkett’s picture

  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.

Wim Leers’s picture

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

tim.plunkett’s picture

+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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
15.6 KB
28.15 KB

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

Status: Needs review » Needs work

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
28.04 KB

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