Allow any contextual link to opt-in to being displayed via offcanvas

@tim.plunkett thanks for this patch.

Looking at that patch I realized that outside.js is written so that any contextual link that used the offcanvas tray will always trigger edit mode. Long term we don't want this.

This patch separates this.

It also changes it so that a contextual link can designate it should trigger edit mode by

  options:
    outside-in-edit: true

This will take care of other options. It is also possible to just specify that it should be a dialog.

If a link just needs to open in the offcanvas then:

options:
    dialog-type: dialog
    dialog-renderer: offcanvas

Of course it could also specify 'modal' only.

I think in #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
We should move the dialog-* options handling to contextual modal because it should be possible for contextual link to work with dialog system which they cannot now regardless anything in Setting Tray module.

I doing my interdiff against #3 because I got rid of most of my changes in #4.
RE: #6

I think this is out of scope.

I changed the title/scope of this issue back to "Allow any contextual link to opt-in to being displayed via offcanvas"
Without of the change to blockConfigureSelector it actually won't be possible to opt in to offcanvas without also triggering Settings Tray module Edit Mode.
Since we are trying to keep OffCanvas dialog and the Settings Tray module separate we need this to keep in scope with what the issue is trying to do. It was never an issue or noticed before because only this modules links were using offcanvas.

Blocked by #2821709: Allow links to open content in Offcanvas tray without edit mode being enabled.

Marking as child of #2815831: Move Off-canvas related CSS from drupal.outside_in library to drupal.off_canvas

wrong parent

Blocker #2821709: Allow links to open content in Offcanvas tray without edit mode being enabled. fixed.

Will work on patch

Ok here is patch.

Interdiff from #3.

@tim.plunkett moved comments.

Discussing this with the team, it seems like this is a nice-to-have and so far we haven't seen any actual use cases for it. Switching to feature request.

Oops. :) Actually, this is pretty important from the standpoint of standardizing on ways for modules (e.g. Field Layout) to interact with the Settings Tray, however, the existing dialog system has the same limitations, so it remains a normal task.

+++ b/core/modules/outside_in/outside_in.module
@@ -29,18 +30,27 @@ function outside_in_help($route_name, RouteMatchInterface $route_match) {
+  $add_library = FALSE;

Nit: s/add/attach/

Setting back to RTBC because it was DrupalCI error

#22 was random test failure and now passing

Random failure back to RTBC, rinse and repeat ;)

1. +++ b/core/modules/outside_in/tests/src/Unit/ContextualLinksViewAlterTest.php
   @@ -0,0 +1,143 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static $modules = ['outside_in'];
   ...
   +    require_once $this->root . '/core/modules/outside_in/outside_in.module';
   

   This shouldn't do anything. Since this is a unit test. However as we're loading code - let's make this a kernel test and be done. Then we don't need to worry about including code and we don't need to worry about other tests and code inclusion issues.

2. +++ b/core/modules/outside_in/tests/src/Unit/ContextualLinksViewAlterTest.php
   @@ -0,0 +1,143 @@
   +  public function testValidDialogOptions() {
   ...
   +  public function testNoDialogOptions() {
   

   This looks like we could use an @dataProvider here instead. Which would make adding future test cases simpler.

Here is patch that leaves it as a Unit test but uses a dataprovider

Setting back to RTBC. Thanks @alexpott's concerns have been addressed.

#27 sounds reasonable to me, but kicking back to @alexpott to make sure he thinks so, too. :)

Reviewed the patch and nothing stuck out to me though.

+++ b/core/modules/outside_in/tests/src/Unit/ContextualLinksViewAlterTest.php
@@ -0,0 +1,147 @@
+    require_once $this->root . '/core/modules/outside_in/outside_in.module';

This affects the test environment of every single unit test - we shouldn't be adding includes in unit tests. The @covers reason definitely is not a good enough reason to break this. If anything it means we need to file a bug against the @covers standards checker (which to be honest I'm not a huge fan of).

Ok this changes it to a Kernel test to get of the "required_once"

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

Attaching patch for re-roll.

Resolved conflict in core/module/outside_in/outside_in.module found in rebasing 8.4.x on last passed version.

+++ b/core/modules/outside_in/outside_in.links.contextual.yml
@@ -2,3 +2,6 @@ outside_in.block_configure:
+    dialog-renderer: offcanvas

Think this will have to be "off_canvas" not "offcanvas"

Actually I think all the occurrences of "offcanvas" in the patch should be replaced with "off_canvas". They are deal with the renderer.

See #2862625: Rename offcanvas to two words in code and comments.

Changes made as per comment #41.

@Yogesh Pawar thanks for the patch.

there are a couple other occurrences of ''offcanvas''

Also couple other things I didn't catch before.

1. +++ b/core/modules/outside_in/tests/src/Kernel/ContextualLinksViewAlterTest.php
   @@ -0,0 +1,137 @@
   +          'dialog-renderer' => 'offcanvas',
   

   Should be off_canvas

2. +++ b/core/modules/outside_in/tests/src/Kernel/ContextualLinksViewAlterTest.php
   @@ -0,0 +1,137 @@
   +            'data-dialog-renderer' => 'offcanvas',
   

   Should be off_canvas

3. +++ b/core/modules/outside_in/outside_in.module
   @@ -29,27 +30,27 @@ function outside_in_help($route_name, RouteMatchInterface $route_match) {
   -  if (isset($element['#links']['outside-inblock-configure'])) {
   -    // Place outside_in link first.
   -    $outside_in_link = $element['#links']['outside-inblock-configure'];
   -    unset($element['#links']['outside-inblock-configure']);
   -    $element['#links'] = ['outside-inblock-configure' => $outside_in_link] + $element['#links'];
   

   This logic cannot be removed. It causes a test failure.

   See \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks

   $this->assertEquals('Quick edit', $link->getText(), "'Quick edit' is the first contextual link for the block.");

4. +++ b/core/modules/outside_in/outside_in.module
   @@ -29,27 +30,27 @@ function outside_in_help($route_name, RouteMatchInterface $route_match) {
   -    // If this is content block change title to avoid duplicate "Quick Edit".
   -    if (isset($element['#links']['block-contentblock-edit'])) {
   -      $element['#links']['outside-inblock-configure']['title'] = t('Quick edit settings');
   

   This logic cannot be removed:

   See \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testCustomBlockLinks

Thanks @tedbow, for the suggestions. Added updated patch with interdiff, hope so test will not fail this time.

@Yogesh Pawar hopefully this will fix it.

I just have to move the new code outside of the if statement it was within.

Setting back to RTBC. I have write some contextual links to test the patch #48 manually, it works great.

any update on this issue ? it's it RTBC since last 2 weeks.

I don't think we will need special logic for this once #2892942: Contextual links support options but not use them to generate links is supported.

That issue was committed and then reverted because it is needs test. If you want this functionality then working on that issue will get it.

I briefly did #2893978: Use newly supported 'options' in contextual links in Settings Tray contextual links before the revert but this will show you how it works.

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