In Outside In mode, form validation messages should appear in the off-canvas tray, not the main page

Ok so this issue is actually really tricky.

Additionally form validation messages do appear in the off-canvas tray which strengthens the expectation that the 'success' message will appear there as well.

I don't think this is actually true. Client-side validation error guess would for required fields but any form validation errors would actually cause the form to be redirected away from the modal to form path directly.

So there are some problems in core that will cause this not to work. I will try to list them here and then find existing issues or create new ones.

1. When a form is in a dialog any server-side validation error will cause the page to redirect to the path that is open the dialog. You can see in the block place form.
2. If you switch the form to use ajax then the form will not be redirected but then you will not see the validation error at all.

Ok here is a start to this.
For now it justs.
Switches the OffCanvas block forms to use ajax.
Displays the messages if any when the form is submitted.

This patch also introduces a fake validation error to test out the validation message because it is actually hard to produce.

There error message actually won't be formatted b/c of this issue #2799611: Style Javascript messages in the offcanvas tray

This also adds tests that tests simple form.

This problem with showing validation messages applies to any forms in dialogs there just aren't many forms in dialogs in core. Related new issue #2857158: Block placement validation errors incorrectly leave the modal

Ok, so this adds the redirect if the form doesn't have any errors. It redirects to the current page and then re-opens the form automatically.

Currently I am checking for form errors in \Drupal\outside_in\Render\MainContent\AjaxRenderer::renderResponse to determine if a redirect should be done. This probably hacky but I didn't see another way.

I just found \Drupal\editor\Form\EditorImageDialog::submitForm() which actually returns and AjaxResponse with attached commands.
It seems like it might be a good idea to do the same in \Drupal\outside_in\Block\BlockEntityOffCanvasForm::submitForm() but I have to look at EditorImageDialog and see whats going on there.

Probably in that case it would make sense to make a new trait OffCanvasFormTrait that submit functions could call. Then something like

public function submitForm(array &$form, FormStateInterface $form_state) {
    parent::submitForm($form, $form_state);
    // Use trait
    return $this->createAjaxResponse($form_state);
  }

Fixed js coding standard, missing semicolon.
Also

-            instance.options.data.formOptions = {messagesSelector: '.messages__wrapper'};
+            instance.options.data.formOptions = {messagesSelector: '.ui-dialog-offcanvas .messages__wrapper'};

The selector needs to be more specific so it doesn't wipe out messages elsewhere.
Though maybe when a ajax validation error comes back other messages outside of the dialog should be removed?

       var qs = drupalSettings.path.currentQuery;
-      if (qs.hasOwnProperty('editable_id')) {
+      if (qs && qs.hasOwnProperty('editable_id')) {

"currentQuery" might not exist.

-      '#markup' => '<div class="messages__wrapper" ></div>',
+      '#type' => 'container',
+      '#attributes' => ['class' => ['messages__wrapper']],

Removed hardcoded html.

From IS

Present the success message inside the off-canvas tray above the form and do not automatically close the tray on save, but let the user close it when the

This is what the current patch does. But since the page has to reload if the form save correctly(to reload the page changes) it feels kind of weird to re-open the dialog. If the save is success if feels like we should not re-open the tray.

I also noticed that because of #2847664: Edit mode behaves differently if entered in by clicking "quick edit" vs toolbar
If you click the contextual link to open the dialog when you save it and get redirected you will be taken out of edit mode. But that is fixed by the patch in the other issue.

1. +++ b/core/modules/outside_in/js/outside_in.js
   @@ -222,21 +224,41 @@
   +          if (instance.hasOwnProperty('dialogType')) {
   

   We're using hasOwnProperty() here.

2. +++ b/core/modules/outside_in/js/outside_in.js
   @@ -222,21 +224,41 @@
   +            if (!('dialogOptions' in instance.options.data)) {
   

   The in keyword here.

3. +++ b/core/modules/outside_in/js/outside_in.js
   @@ -222,21 +224,41 @@
   +      if (qs && qs.hasOwnProperty('editable_id')) {
   

   Again, hasOwnProperty()

Perhaps we can be consistent in our method for checking to see if a property on an Object exists.

@drpal ok changed to use hasOwnProperty instead of "in"

Also fixed failing test. When using ajax submit I guess we need jquery.form library.
and moved messages to the top of the form.

Fixing the test

Whoops for to actually upload the patch. Interdiff still valid for this patch.

1. +++ b/core/modules/outside_in/js/outside_in.js
   @@ -222,21 +224,41 @@
   -            wrapperOffcanvas = instance.options.url.indexOf('drupal_dialog_offcanvas') === -1;
   +            if (rendererOffcanvas) {
   +              wrapperOffcanvas = instance.options.url.indexOf(replaceLink) === -1;
   +            }
   +            else {
   +              if (instance.$form[0] && instance.$form[0].hasAttribute('data-off-canvas-form')) {
   +                rendererOffcanvas = true;
   +                wrapperOffcanvas = instance.options.url.indexOf(replaceForm) === -1;
   +              }
   +            }
   +
              }
              return hasElement && rendererOffcanvas && wrapperOffcanvas;
            })
            .forEach(function (instance) {
              // @todo Move logic for data-dialog-renderer attribute into ajax.js
              //   https://www.drupal.org/node/2784443
   -          instance.options.url = instance.options.url.replace(search, replace);
   -          // Check to make sure existing dialogOptions aren't overridden.
   -          if (!('dialogOptions' in instance.options.data)) {
   -            instance.options.data.dialogOptions = {};
   +          if (instance.hasOwnProperty('dialogType')) {
   +            instance.options.url = instance.options.url.replace(searchLink, replaceLink);
   +            // Check to make sure existing dialogOptions aren't overridden.
   +            if (!instance.options.data.hasOwnProperty('dialogOptions')) {
   +              instance.options.data.dialogOptions = {};
   +            }
   +            instance.options.data.dialogOptions.outsideInActiveEditableId = $(instance.element).parents('.outside-in-editable').attr('id');
   +            instance.options.url += '&editable_id=' + instance.options.data.dialogOptions.outsideInActiveEditableId;
   +            instance.progress = {type: 'fullscreen'};
   +          }
   +          else {
   +            instance.options.url = instance.options.url.replace(searchFrom, replaceForm);
   +            instance.options.data.formOptions = {messagesSelector: '.ui-dialog-offcanvas .messages__wrapper'};
              }
   -          instance.options.data.dialogOptions.outsideInActiveEditableId = $(instance.element).parents('.outside-in-editable').attr('id');
   -          instance.progress = {type: 'fullscreen'};
            });
   +      var qs = drupalSettings.path.currentQuery;
   +      if (qs && qs.hasOwnProperty('editable_id')) {
   +        $('#' + qs.editable_id + ' a' + blockConfigureSelector).once('outside_in_qs').trigger('click');
   +      }
   

   I find this code extremely difficult to understand. Lots of wrapping, no comments explaning what's going on, lots of jQuery-style code.

   I think this can benefit a lot from creating helper methods.

2. +++ b/core/modules/outside_in/src/Render/MainContent/AjaxRenderer.php
   @@ -0,0 +1,93 @@
   +   * @todo Currently a copy of Drupal\Core\Render\MainContent\AjaxRenderer with
   +   * additional logic to display messages in the OffCanvas dialog. Refactor
   +   * parent to include this logic.
   

   Why not refactor the parent class in this class?

   Not doing that now means there is significant risk of the code diverging, and possibly even security fixes in the parent class not being inherited here.

3. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OffCanvasTest.php
   @@ -103,4 +103,19 @@ public function testNarrowWidth() {
   +  public function testFormErrors() {
   

   This says form errors, but the issue says messages. So this is issue is now dealing with both messages and AJAXy form validations, which can show messages.

   Two birds, one stone — great!

   But now we're not testing that messages (e.g. validation errors) for non-AJAXy forms are also shown correctly.

@Wim Leers thanks for the review. I have not had time to address it yet but your points seem right.

re #2 while working on #2866554: Add Quick Edit off canvas form. for the Webform module I see they ran into a similar problem but were able to add the redirect command with doing the kind of hack I was. The code is in WebformDialogTrait which we can probably learn from.

Why not refactor the parent class in this class?

I was attempting to keep all of the changes in the experimental module. Should have made an issue to move it pre the module being stable. But might not need this at all now.

Just wanted to make this note before I forgot.

Ok as mention in #19 in this patch I took WebformDialogTrait and chaning it to \Drupal\outside_in\OffCanvasFormDialogTrait

Hopefully Webform would just be able to use this trait directly. Thanks @jrockowitz for pointing me to this code. Can someone get commit credit if their code was stolen for a fix? ;)

So re @Wim Leers' review in #18
1. outside_in.js is no longer being changed because we don't introduce a new renderer.
2. This renderer is no longer needed. Yay!
3.

But now we're not testing that messages (e.g. validation errors) for non-AJAXy forms are also shown correctly.

So would this be if JS is off. Off-Canvas won't work at all then. I am probably missing something. But haven't had time to look into it.

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

Rerolled.

@Jo Fitzgerald thanks for the re-roll!

I think the only problem remaining are integrating the changes with #2862625: Rename offcanvas to two words in code and comments. which was commited since the patch that needs to re-rolled here.

Basically that issue we were trying to resolve that fact that we(ok maybe me) had been inconsistent about 'offcanvas' vs 'off-canvas' or 'off_canvas' basically now it should always be consider 2 words.

Picked a few examples below. But basically "offcanvas" all lower case should appear in the patch(or module).

So for example the test module should be off_canvas_test not offcanvas_test, affects namespace, file and folder names etc

For the test module you can enable off_canvas_test and manually go to routes in off_canvas_test.routing.yml to see if it is working.

Thanks

1. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,202 @@
   +      'drupal_dialog_offcanvas',
   

   Should be 'drupal_dialog_off_canvas'

2. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,202 @@
   +    $response->addCommand(new CloseDialogCommand('#drupal-offcanvas'));
   

   Should be '#drupal-off-canvas'

3. +++ b/core/modules/outside_in/tests/modules/off_canvas_test/off_canvas_test.routing.yml
   @@ -27,3 +27,10 @@ off_canvas_test.dialog_links:
   +  path: '/offcanvas-form'
   

   Change to '/off-canvas-form'

Split offcanvas into two words in other places.

@Jo Fitzgerald reroll look good now! Thanks!

Ok this patch just removes some local test changes I made locally and left in #20.

I can't get this to fail locally. And I have no idea why these changes would cause this error:

If you don't care about these errors, you can ignore them by
setting js_errors: false in your Poltergeist configuration (see documentation for details).
ReferenceError: Can't find variable: jQuery

$this->assertJsCondition('jQuery("' . $block_selector . ' ' . $label_selector . '").html() == "' . $new_page_text . '"');

Also there is todo to remove this and now #2837676: Provide a better way to validate all javascript activity is completed is fixed so I will create this issue.

Actually I was wrong in #29 that test failure makes total sense.

This patch changes the OffCanvas form to use AJax submit and then redirect.

Also the changes I made in #27 uncommented some of the test cases in OutsideInBlockFormTest::providerTestBlocks() . These cases have 'new_page_text' which triggered the assert.

The assert that was failing already had a todo to remove once #2837676: Provide a better way to validate all javascript activity is completed was fixed.
It is fixed so we can now use \Drupal\FunctionalJavascriptTests\JSWebAssert::waitForElement()

This should make the tests pass.

The only other change I had to make was to remove "?" and "." characters from the test data in 'new_page_text'. These were causing a problem in ":contains()". Since this was just test data it is fine to remove them.

Looks this patch added another @todo for #2837676: Provide a better way to validate all javascript activity is completed

Removing that 1 also. Canceled the previous test.

Ok this re-rolls again then does:

1. Removes padding on message div in tray when it is empty
2. Adds testing submitting a form in the tray when a destination is NOT provided in \Drupal\Tests\outside_in\FunctionalJavascript\OffCanvasTest::testFormErrors
3. Simpifies \Drupal\outside_in\OffCanvasFormDialogTrait::submitFormDialog()

Ok this should get that test passing.

Add the patch in this issue #2883483: Assert that calls to waitForElementVisible() actually return element in OutsideIn javascript tests. because it just fixes a test mess up we did earlier and it is really hard to figure what is actually happen without.

Then I just added an ajax after the form is submitted because now we have different way of submitting forms in the tray.

1. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,196 @@
   +   * Is the current request for an AJAX modal dialog.
   

   Maybe something like "Determines if the current request is for an AJAX modal dialog."

2. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,196 @@
   +    return (in_array($wrapper_format, [
   +      'drupal_ajax',
   +      'drupal_modal',
   +      'drupal_dialog_off_canvas',
   +    ])) ? TRUE : FALSE;
   

   You can have return in_array(); directly, no need for the ternary or extra parens

3. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,196 @@
   +   * Add modal dialog support to a form.
   

   Adds

4. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,196 @@
   +      $form['#prefix'] = '<div id="off-canvas-form">';
   +      $form['#suffix'] = '</div>';
   

   Is this how we do things now? Ideally we wouldn't use #prefix/#suffix, and wouldn't handwrite the HTML. Maybe something like #theme_wrappers?

5. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,196 @@
   +        $response->addCommand(new RedirectCommand($redirect_url->setAbsolute()
   +          ->toString()));
   

   This is a very strange place to break the line.

   One idea would be to have each of these conditionals set up a $command, and add it below right before the return

6. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,196 @@
   +        $response->addCommand(new CloseDialogCommand('#drupal-off-canvas'));
   ...
   +  public function closeDialog(array &$form, FormStateInterface $form_state) {
   +    $response = new AjaxResponse();
   +    $response->addCommand(new CloseDialogCommand('#drupal-off-canvas'));
   +    return $response;
   

   Couldn't this delegate?

7. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,196 @@
   +    if ($destination = $this->getRedirectDestinationPath()) {
   +      return Url::fromUserInput('/' . $destination);
   +    }
   

   Yikes! Is that really how we have to do things?

8. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,196 @@
   +    return NULL;
   ...
   +    return NULL;
   

   I don't think explicitly returning NULL is needed.

9. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,196 @@
   +    if ($this->requestStack->getCurrentRequest()->get('destination')) {
   +      return $this->getRedirectDestination()->get();
   

   Why the extra check first?

10. +++ b/core/modules/outside_in/src/Render/MainContent/OffCanvasRender.php
    @@ -41,6 +41,15 @@ public function __construct(TitleResolverInterface $title_resolver, RendererInte
    +    $main_content = [
    +      'off_canvas_messages' => [
    +        '#type' => 'container',
    +        '#attributes' => ['class' => ['messages__wrapper']],
    +        '#weight' => 100,
    +      ],
    +    ] + $main_content;
    

    Not sure that the reassignment of $main_content is needed here.

    $main_content['off_canvas_messages']
     = [
    ...

    Wouldn't this work just fine?

11. +++ b/core/modules/outside_in/tests/modules/off_canvas_test/src/Form/TestForm.php
    @@ -0,0 +1,69 @@
    +   * Form submission handler.
    +   *
    +   * @param array $form
    +   *   An associative array containing the structure of the form.
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   *   The current state of the form.
    

    {@inheritdoc}

12. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OffCanvasTest.php
    @@ -120,4 +120,57 @@ protected function getTestThemes() {
    +   * Test form errors in the Off-Canvas dialog.
    

    Tests

13. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
    @@ -146,7 +147,7 @@ public function providerTestBlocks() {
    -        'new_page_text' => 'Can you imagine anyone showing the label on this block?',
    +        'new_page_text' => 'Can you imagine anyone showing the label on this block',
    
    @@ -154,7 +155,7 @@ public function providerTestBlocks() {
    -        'new_page_text' => 'The site that will live a very short life.',
    +        'new_page_text' => 'The site that will live a very short life',
    

    I don't understand these changes

14. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInJavascriptTestBase.php
    @@ -140,4 +140,19 @@ protected function getTestThemes() {
    +   * Assert the specified selector is visible after a wait.
    

    Asserts

@tim.plunkett thanks for the review!

1. Fixed
2. fixed
3. fixed
4. I tried adding $form['#theme_wrappers'] = 'outside_in_form_wrapper'; with the an added template and theme hook. But this resulted in the form element not being rendered. not sure why.

The only place I could see this happening core was \Drupal\forum\Form\ForumForm::form() $form['#theme_wrappers'] = ['form__forum'];
I couldn't figure out what was happening there.

I do see in core at least
$form['#prefix'] = '<div id="editor-image-dialog-form">';
and
$form['#prefix'] = '<div id="views-preview-wrapper" class="views-preview-wrapper views-admin clearfix">';
and
$form['#prefix'] = '<div class="views-form">';

#theme_wrappers does seem more correct. Maybe somebody knows what is happening here.

5. I am command will always be set in submitFormDialog() so just assigning to $command then adding in return statement because addCommand() will return back the response.
6. I realized this can be written in 1 line return (new AjaxResponse())->addCommand(new CloseDialogCommand('#drupal-off-canvas'));
And then also submitFormDialog() in the case where the dialog should be closed we can just call closeDialog()
7. Hmm. I was going to use \Drupal\Core\Routing\RedirectDestination::get() directly and follow the example in \Drupal\Core\Routing\RedirectDestinationInterface::get() doc.
\Drupal\Core\Url::fromUserInput(\Drupal::destination()->get())->setAbsolute()->toString()

But I looked at RedirectDestination::get() and if 'destination' is not in the query string it will use
$this->urlGenerator->generateFromRoute('<current>
We only want to do a redirect command if the 'destination' was sent in the query string. Otherwise the path will be the path of the form in the try not the main page the user is looking at.
We do not want to redirect the page to the form.

8. removed/fixed.
9. Same reason as 7. If 'destination' is not in query string then \Drupal\Core\Routing\RedirectDestinationTrait::getRedirectDestination() will actually use the current request(form in tray). We don't want that.
10. I think originally I thought there was problem with '#weight' not working here. Like the sort had already be run at this point and I couldn't get the weight to work.
but testing again I can't get the problem so fixing as you suggested.
11. fixed
12. fixed
13. This is because when adding this line
$new_page_text_locator = "$block_selector $label_selector:contains($new_page_text)";
If $new_page_text has a "." or "?" in it this messes up the css selector. So since they are just test text it is easier to remove those characters.
14. fixed.

4) On second thought, #theme_wrappers won't work here because adding it to a top level $form element will prevent form internals from working. This needs its own issue. However, if the id="off-canvas-form" part doesn't need to be on a div but could be on the form, this would also work:

-      $form['#prefix'] = '<div id="off-canvas-form">';
-      $form['#suffix'] = '</div>';
+      $form['#id'] = 'off-canvas-form';

The more correct fix is this, which includes out-of-scope changes IMO:

diff --git a/core/lib/Drupal/Core/Form/FormBuilder.php b/core/lib/Drupal/Core/Form/FormBuilder.php
index be000dc..cfff76d 100644
--- a/core/lib/Drupal/Core/Form/FormBuilder.php
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -803,7 +803,7 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
       $form['#attributes']['data-drupal-selector'] = Html::getId($form_id);
     }
 
-    $form += $this->elementInfo->getInfo('form');
+    $form = NestedArray::mergeDeep($form, $this->elementInfo->getInfo('form'));
     $form += ['#tree' => FALSE, '#parents' => []];
     $form['#validate'][] = '::validateForm';
     $form['#submit'][] = '::submitForm';
diff --git a/core/modules/outside_in/src/OffCanvasFormDialogTrait.php b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
index e265a8c..ec682be 100644
--- a/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
+++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
@@ -74,8 +74,8 @@ protected function buildFormDialog(array &$form, FormStateInterface $form_state)
 
     if ($ajax_callback_added) {
       $form['#attached']['library'][] = 'core/drupal.dialog.ajax';
-      $form['#prefix'] = '<div id="off-canvas-form">';
-      $form['#suffix'] = '</div>';
+      $form['#theme_wrappers'][] = 'container';
+      $form['#attributes']['id'] = 'off-canvas-form';
     }
   }
 

7) Ugh, I forgot how broken RedirectDestination is. #2745911: Block add links should respect destination had a similar problem. We should document this though, since that's not how the service should work.

4. Ok this actually fixes the problem
$form['#attributes']['id'] = 'off-canvas-form';
Seems like a simple solution.

7. Added a comment on why we can't use \Drupal\Core\Routing\RedirectDestination::get() directly

New changes:
Also added some changes to \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::openBlockForm() because I was getting test failure locally. It has been tricky with this module to make sure the ajax activity is completed before we try another action and this has cause random failures in the past.

In openBlockForm() I added a wait for the contextual links to show up because these are needed for the opening of the block forms to work.
This issue changes the workflow from a regular form submit to an ajax form submit and then a redirect command so test changes were expected.

Thanks for the changes. Manually tested it as well as reviewed the automated tests, looks great!

There were no screenshots for this issue, so I set out to make some while reviewing the behaviour.

I can confirm that the messages show up in the settings tray as intended ("after"):

However, the fact that this screenshot was even possible seems to be a regression, because normally HTML5 required handling would take care of that, and does in the current code ("before"):

@tedbow said this is because the submission handler was switched to AJAX, and there might be a problem with how it's set. Setting back to needs review to take a closer look at that.

Ok looked at the html5 validation issue. Haven't figured out the solution but I think it is problem with using an ajax submit on any form and the clientside validation not be fired because the actually form itself is not being submitted. Talked to @drpal and @tim.plunkett about this and think the solution is to file an issue with the ajax system.

I did find 2 other problems when reviewing it.

1. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,191 @@
   +    return in_array($wrapper_format, [
   +      'drupal_ajax',
   +      'drupal_modal',
   +      'drupal_dialog_off_canvas',
   +    ]);
   +  }
   

   Had to update this to use drupal_dialog.off_canvas because https://www.drupal.org/node/2844261 was committed and changes the string to [DIALOG_TYPE].[DIALOG_RENDER]. Just replaced "_" with ".".

2. +++ b/core/modules/outside_in/src/Render/MainContent/OffCanvasRender.php
   @@ -41,6 +41,13 @@ public function __construct(TitleResolverInterface $title_resolver, RendererInte
   +    // Add place holder form messages from Ajax form requests.
   +    $main_content['off_canvas_messages'] = [
   +      '#type' => 'container',
   +      '#attributes' => ['class' => ['messages__wrapper']],
   +      '#weight' => 100,
   +    ];
   +
   

   We don't actually need this any more.

3. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,191 @@
   +      $form['status_messages'] = [
   +        '#type' => 'status_messages',
   +        '#weight' => -1000,
   +      ];
   

   This element will handle showing any messages. So don't need the container in OffCanvasRenderer.

4. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,191 @@
   +      $command = new HtmlCommand('#off-canvas-form', $form);
   

   This should a ReplaceCommand. Right now this results a
   element inside another
   element both having the same id.

1. +++ b/core/modules/outside_in/css/outside_in.module.css
   @@ -22,6 +22,11 @@
   +.ui-dialog-off-canvas .messages__wrapper:empty {
   +  padding: 0;
   +  margin: 0;
   +}
   

   .messages__wrapper is a class only added by Bartik templates

2. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
   @@ -36,6 +39,7 @@ public function title(BlockInterface $block) {
   +    $form['#attributes']['data-off-canvas-form'] = TRUE;
   

   I don't see this being used anywhere else. Additionally, TRUE doesn't mean anything when rendered out, I see this in the markup data-off-canvas-form=""

3. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
   @@ -109,4 +113,14 @@ protected function getPluginForm(BlockPluginInterface $block) {
   +    // \Drupal\block\BlockForm::submitForm() always redirects to block listing.
   +    // This would doesn't work with Ajax submit.
   

   I don't understand the second sentence

4. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,191 @@
   +   * Get the form's redirect URL.
   ...
   +   * Get the URL from the destination service.
   ...
   +   * Get the redirect destination path if specified in request.
   

   Gets

5. +++ b/core/modules/outside_in/tests/modules/off_canvas_test/src/Form/TestForm.php
   @@ -0,0 +1,64 @@
   +    $form['#attributes']['data-off-canvas-form'] = TRUE;
   

   Also seems to be unnecessary

Also I think the test failure is due to

'data' => $dialog_contents . "<div class=\"messages__wrapper\"></div>\n",

in OffCanvasDialogTest

1. This is not necessary anymore anyways I think this was needed when we adding the container in OffCanvasRender but we aren't anymore as noted in #42 2 & 3
2. Yep removed this and also in \Drupal\off_canvas_test\Form\TestForm
3. Updated sentence. Basically \Drupal\block\BlockForm::submitForm() will always set a redirect which won't work with ajax submit anyways.
4. fixed
5. fixed.

Test failure in #42 was because I forgot update OffCanvasDialogTest when removing the message container from OffCanvasRender
Fixed.

Looks great!
Opened #2895477: Native browser form validation does not fire when submit buttons use #ajax as a follow-up for the validation issue, as it is for all #ajax and not related to outside-in.

I don't know if it's just my computer, but this doesn't seem to work at all on my machine (I tried both Chrome and Firefox). When I QuickEdit the site branding block, upon changing the site name, the page is refreshed with the settings tray closed, and the message in the usual place, just like in HEAD.

Can someone post a screenshot or video of this working?

@effulgentsia It looks like this is only getting triggered when the message is an error, not for other kinds of messages:

+++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
@@ -0,0 +1,191 @@
+    if ($form_state->hasAnyErrors()) {
+      $form['status_messages'] = [
+        '#type' => 'status_messages',
+        '#weight' => -1000,
+      ];
+      $command = new ReplaceCommand('#off-canvas-form', $form);
+    }

Error messages look the same as webchick's screenshots from #41.

Thanks. If #48 is the intention of this issue's scope, then the issue title and summary need updating. Or is that not the intention and the patch needs fixing?

@rootwork @effulgentsia yes this just for form validation message. I updated the summary.

Uploading 2785047-45-forced_errr-do-not-test.patch was a mistake

I've asked in #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait that a dedicated issue be split off for the ModalFormTrait that issue is adding, so we should probably use that one here. Setting NR for now, but probably we can postpone it on this hypothetical issue for the trait, since we should avoid adding more API surface to the "not an API" (reference: #2894584: Settings Tray provides functionality, not an API: mark PHP and JS as internal).

Thanks!

Ok this moves OffCanvasFormDialogTrait to DialogFormTrait.

I updated #2830584-113: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait with the exact trait. Hopefully tests pass in both.

Ok I see the problem \Drupal\Core\Form\DialogFormTrait::getDialogSelector() I need determine which main content format was used to originally make the form not the current that in ajax submit will be "drupal_ajax".

So in buildFormDialog I actually need to save the format when originally building the form. Otherwise you can't if it was modal or the off-canvas tray. Don't have time to fix right now.

Ok I removed getDialogSelector() all together and added $dialog_selector parameter to buildFormDialog() because presumably you can actually change the selector anyways. So this would allow that.

Yay, #57 works correctly on my machine :)

This is an incomplete review, but just getting a couple questions started:

1. +++ b/core/lib/Drupal/Core/Form/DialogFormTrait.php
   @@ -0,0 +1,199 @@
   +  protected function isDialog() {
   +    return in_array($this->getRequestWrapperFormat(), [
   +      'drupal_ajax',
   +      'drupal_dialog',
   +      'drupal_modal',
   +      'drupal_dialog.off_canvas',
   +    ]);
   

   Why is "drupal_ajax" in this list? Also, instead of hard-coding "drupal_dialog.off_canvas", could we test for drupal_dialog.* and drupal_modal.*?

2. +++ b/core/modules/outside_in/outside_in.libraries.yml
   @@ -20,6 +20,8 @@ drupal.outside_in:
   +    - core/drupalSettings
   +    - core/jquery.form
   

   Now that we have the trait, is there a reason we need or want these as explicit dependencies?

+++ b/core/lib/Drupal/Core/Form/DialogFormTrait.php
@@ -0,0 +1,199 @@
+  protected function buildFormDialog(array &$form, FormStateInterface $form_state, $create_cancel = FALSE, $dialog_selector = '#drupal-modal') {

We have no real caller or test coverage of passing TRUE for $create_cancel. Are we sure we want the parameter? If so, we need test coverage for it.

+++ b/core/modules/outside_in/tests/modules/off_canvas_test/src/Form/TestForm.php
@@ -0,0 +1,63 @@
+class TestForm extends FormBase {

We should move this test outside of the outside_in module. We already have a AjaxTestDialogForm test. Perhaps it makes sense to merge into that one? Or, if we want to keep that one without the trait, then maybe an additional class alongside that one that does use the trait?

+++ b/core/lib/Drupal/Core/Form/DialogFormTrait.php
@@ -0,0 +1,199 @@
+    if (!empty($form['actions']['cancel'])) {
+      // Replace 'Cancel' link button with a close dialog button.
+      $form['actions']['cancel'] = [
+        '#submit' => ['::noSubmit'],
+        '#limit_validation_errors' => [],
+        '#ajax' => [
+          'callback' => '::closeDialog',
+          'event' => 'click',
+        ],
+      ] + $form['actions']['cancel'];

Adding #submit assumes that $form['actions']['cancel'] is a button, but for most forms, it's a link. While we do still have some forms that use a Cancel button instead of a link, I think the better pattern is for it to be a link, so we must cover that case. I wonder if for this issue, if it would be enough to simply hide ('#access' => FALSE) $form['actions']['cancel'], regardless of whether it's a button or link, since presumably the dialog itself has its own close button. And then we can have a follow-up for adding code to unhide the form's cancel button/link, and make it work in either case.

[EDIT: I might be wrong on this code not working for links. #ajax['callback'] might be enough to handle the link case and the form-specific stuff would just be ignored. But if that's true and we want to leave it like this rather than just hide the element, let's add test coverage for it, and improve the code comments to clarify that both are handled.]

+++ b/core/lib/Drupal/Core/Form/DialogFormTrait.php
@@ -0,0 +1,199 @@
+      $form['#attributes']['id'] = 'dialog-form';
...
+      $command = new ReplaceCommand('#dialog-form', $form);

For non-modal dialogs, this wouldn't be unique. Besides, messing with the form's HTML ID could have undesired side-effects on contrib/custom code. FormBuilder::prepareForm() sets $form['#attributes']['data-drupal-selector'], so can we use that for our ReplaceCommand selector?

+++ b/core/lib/Drupal/Core/Form/DialogFormTrait.php
@@ -0,0 +1,199 @@
+        'event' => 'click',
...
+          'event' => 'click',

Why are we setting the event instead of letting RenderElement::preRenderAjaxForm() default it to mousedown for buttons and click for links? That function documents why it uses mousedown, and if we feel a need to override that, we need to document why we're doing that. Otherwise, if this trait is used by a form with an autocomplete textfield, it might encounter the kinds of errors that RenderElement::preRenderAjaxForm() is trying to avoid.

+++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
@@ -16,6 +17,8 @@
+  use DialogFormTrait;
...
+    $this->buildFormDialog($form, $form_state, FALSE, '#drupal-off-canvas');

Yay for this patch reducing to only 2 lines of code what's needed for forms in dialogs to fix this bug!

However, it seems to me we shouldn't need these 2 lines at all. Is it possible to make all forms in dialogs "just work", without needing this opt-in? Could DialogRenderer and/or system_form_alter() accomplish the same thing as the trait, but on behalf of all forms in dialogs automatically?

@effulgentsia thanks for the review. No patch attached right now just addressing some of the review.

#64
My thought is that since forms don't work well now if you use ajax submit and the dialog system that people who were using forms in this way in contrib probably had to do there own workarounds. So they might have added their own ajax callbacks, redirects, etc.

So I think opting in all forms if they are in the dialog would be dangerous and could break existing contrib and custom forms.

One option though might be to use some sort of opt in property like
$form['#dialog_form'] = TRUE

Then we could see if DialogRenderer and/or system_form_alter() could handle it.

I am going to mention this idea on #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait which is also using this trait.

#62
That sounds like a good idea

#59
I was creating this trait to work with both this issue and #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait
The other issue used the $create_cancel but it might not make sense there.
But yes if it stays it would need test coverage.

#57.1
'drupal_ajax' is in the list because if it is not the this will not pass during the ajax call back for the submit buttons and the whole process will not work. (just checked to be sure)
But I also just realized that this will also catch forms that are not in a dialog using an ajax call back.
The idea of this trait is that you could use on a form and then if the form is a dialog it will work but also if it is not in dialog it will still work. I think this would mess that up and probably if we want test coverage for both scenarios if it seems like that is good use of the trait.

One thought I had for a solution would be to do something like

$form['reneder_in_dialog'] = ['#value' => TRUE'];

When the form is first rendered.
Then if the request format is drupal_ajax then we check 'reneder_in_dialog'

Yes we should just be checking for drupal_dialog.* and drupal_modal.*. Now that #2844261: Allow dialog links to specify a renderer in addition to a dialog type is in we can be sure this are dialog formats.

Re #64: It'd be awesome if we could make forms in dialogs "just work". The trait was added in #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait because we have 6 forms all being updated to open in dialogs, so as you can imagine there was a lot of code duplication.

As both workflows and outside_in are in a pretty tight deadline for stability in 8.4.0 I would like to see the new trait marked @internal and added in for these two modules to use. We can then start investigating better solutions. This could involve updating, removing, or marking the trait as @api.

I believe the trait in #57 and #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait are now slightly different so we'd need to sync up, or as @xjm suggested, break the trait out into a new issue.

I agree with @timmillwood's idea to mark the trait as @internal for now. I also think as we find more use cases for the tray in core we will have more forms that would use this trait or the integration in the form system.

If we leave the behavior as an @internal trait for now we have the flexibility to change it if we find the next few use cases make use want to change it slightly.

This had to be rerolled after #2872104: Resolve @todo in \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest

A straight re-roll though did makes tests fail
I had to add an extra wait after submit the off-canvas form via and the new text appearing on the screen.

This just adds the changes @timmillwood made to the shared trait here #2830584-120: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait
The changes are in \Drupal\Core\Form\DialogFormTrait::getDestinationUrl

I am pretty sure this will fail as they do locally but not exactly sure why.

'drupal_ajax' is in the list because if it is not the this will not pass during the ajax call back for the submit buttons and the whole process will not work.

Seems like that's a bug with the submit button then, since it should be triggering a "drupal_dialog" request, not a "drupal_ajax" request. I see two possible fixes for that:

One would be to change ajax.es6.js to only do

ajax.options.url += `${Drupal.ajax.WRAPPER_FORMAT}=${wrapper}`;

when there isn't already a wrapper format query parameter in the URL. That would prevent ajax.js from overriding the existing value. This might be worth opening a separate issue for.

Another would be for the submit button to explicitly re-specify its dialogType and dialogRenderer settings. Here's a patch that does this.

1. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
   @@ -109,4 +112,24 @@ protected function getPluginForm(BlockPluginInterface $block) {
   +  public function submitForm(array &$form, FormStateInterface $form_state) {
   +    parent::submitForm($form, $form_state);
   +    // \Drupal\block\BlockForm::submitForm() always redirects to block listing
   +    // via \Drupal\Core\Form\FormStateInterface::setRedirect(). This method
   +    // does not work with Ajax submit.
   +    $form_state->disableRedirect();
   +  }
   

   Do we actually need this? FormBuilder::buildForm() already disables redirects for all AJAX forms, so I don't think this is doing anything, and can be safely removed. Or if not, what's the scenario in which this is doing something? Also, see below for something related.

2. +++ b/core/lib/Drupal/Core/Form/DialogFormTrait.php
   @@ -0,0 +1,221 @@
   +      if ($redirect_url = $this->getRedirectUrl()) {
   

   This trait's implementation of getRedirectUrl() is cumbersome. It would make much more sense to call $form_state->getRedirect(). Except we can't if form redirects are disabled (even for a form we want to redirect). So I opened #2896828: Don't automatically disable redirects for AJAX forms. No need to make any changes to this until that issue gets resolved, and I will certainly not block this issue on that one. Just noting it for the curious. Maybe a @todo comment here linking to that issue would be nice though.

I think #69 isn't testing the cancel button, because the test isn't failing as expected. It started failing in #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait once we added tests for closing the dialog with the cancel button. I'm not sure how to properly redirect to a destination path in a way which works for sites in a subfolder and sites that are not.

As both workflows and outside_in are in a pretty tight deadline for stability in 8.4.0 I would like to see the new trait marked @internal and added in for these two modules to use. We can then start investigating better solutions. This could involve updating, removing, or marking the trait as @api.

I agree with @timmillwood's idea to mark the trait as @internal for now. I also think as we find more use cases for the tray in core we will have more forms that would use this trait or the integration in the form system.

I agree.

@timmillwood #72 yes the dialog provided by Settings Tray don't use a cancel button. It uses the default close button that JQuery Provides

ok. talked to @xjm @effulgentsia and @tim.plunkett(form system maintainer) about this again

It seems the current idea is for each module, Settings Tray and Workflow to add there own trait and mark it as @internal. Since these both experimental modules it doesn't make sense to add code to core/lib, even if we mark it as internal.

So I went back our trait and didn't try to make it work for more than the Off-canvas dialogs. I am assuming since this a trait internal to Settings Trait we don't need to handle any other dialog types and that work will be done in #2896535: Create a helper trait for Forms in ajax dialogs

So started this patch from from #70 but removed common trait.
Then I did:
Added trait from #45 So now it is using OffCanvasFormDialogTrait again.
Marked OffCanvasFormDialogTrait as @internal with @todo to remove in #2896535: Create a helper trait for Forms in ajax dialogs (issue maybe updated soon with better description
Added \Drupal\outside_in\OffCanvasFormDialogTrait::getRequestWrapperFormat to make @effulgentsia patch in #70 work with OffCanvasFormDialogTrait
Changed to isModalDialog to isOffCanvasDialog because since this is internal trait for Settings Tray we don't need to handle other dialogs.
Removed unneeded buildForm as per #71.1

Looking at comments since #54 where we started to use the common trait and sees what still applies.

+++ b/core/lib/Drupal/Core/Form/DialogFormTrait.php
@@ -21,11 +21,16 @@
-  protected function buildFormDialog(array &$form, FormStateInterface $form_state, $create_cancel = FALSE) {
+  protected function buildFormDialog(array &$form, FormStateInterface $form_state, $create_cancel = FALSE, $dialog_selector = '#drupal-modal') {

#57.1 added this but we don't need it because are only dealing with 1 type of type the selector will always be the same.

#58.1 is no longer an issue since are only dealing with drupal_dialog.off_canvas we can simply test for
return $wrapper_format === 'drupal_dialog.off_canvas';

#57.2
Removed depedencies

#59
is now
protected function buildFormDialog(array &$form, FormStateInterface $form_state) {
So no longer applies.

#60
No longer applies because the test should in outside_in since the trait is in the module again.

#61
We still have this logic but should we remove it. Since this is an internal trait we don't need to cover the case where there will be a "cancel" button.

#62 No longer applies because you can't have more than 1 off-canvas for open.

#63 still applies but haven't had time to look into it.

#64 #2896535: Create a helper trait for Forms in ajax dialogs should be updated to decide if a trait is actually the best solution

Ok this patch removes more stuff from OffCanvasFormDialogTrait that were really about making a general trait to work with any forms in the tray.
but since we aren't using it like that we don't need the functionality or the test coverage for that.

1. Removed logic in \Drupal\outside_in\OffCanvasFormDialogTrait::buildFormDialog that dealt with the case where the submit button or cancel button wasn't set. We know which form it is being used with so we know which buttons will be there
2. Removed closeDialog() because this was used when there was no destination redirect. This will be never be the case with the off-canvas dialog in Settings Tray's usage. will always need to redirect to see the updated changes.
3. Removed noSubmit() this was no longer being used.
4. In \Drupal\off_canvas_test\Controller\TestController::linksDisplay and \Drupal\off_canvas_test\Form\TestForm removed test coverage for the form being opened up with a destination.

Question, do we even need isOffCanvasDialog() since this an internal trait? This forms will always be in a off-canvas dialog. I think if javascript is disable the contextual links will not work anyway.
This would let us remove getRequestWrapperFormat() getDialogType() and getDialogRenderer().

This patch removes isOffCanvasDialog() along the other functions need for it in response to my own question in #76

+++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
@@ -0,0 +1,114 @@
+    if ($this->requestStack->getCurrentRequest()->get('destination')) {

This should not use $this->requestStack

It should just use $this->getRequest()

+++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
@@ -58,33 +39,6 @@ protected function buildFormDialog(array &$form, FormStateInterface $form_state)
-    $wrapper_format = $this->getRequest()

The above was only working because this had been called before it which set $this->requestStack.

When this function was removed it fails.

Whether we roll back my last 2 patches this change should remain.

Nice simplification! But I think we can simplify further, and at the same time have stronger test coverage, which then will also make it easier to drop the custom code once #2896535: Create a helper trait for Forms in ajax dialogs lands, because we'll have stronger confidence that things keep working, because we've got test coverage for all the edge cases that Settings Tray cares about!

1. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
   @@ -18,6 +20,8 @@
   +  use OffCanvasFormDialogTrait;
   

   This is the only place where this trait is being used. Why then make it a trait? Why not just add it to BlockEntityOffCanvasForm?

2. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,114 @@
   + * Classes that use this trait should implement \Drupal\Core\Form\FormInterface.
   

   This comment should be removed: nothing else should use this.

3. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,114 @@
   +  protected function getRedirectUrl() {
   +    return $this->getDestinationUrl();
   +  }
   +
   +  /**
   +   * Gets the URL from the destination service.
   +   *
   +   * @return \Drupal\Core\Url|null
   +   *   The destination URL or NULL no destination available.
   +   */
   +  protected function getDestinationUrl() {
   +    if ($destination = $this->getRedirectDestinationPath()) {
   +      return Url::fromUserInput('/' . $destination);
   +    }
   +  }
   +
   +  /**
   +   * Gets the redirect destination path if specified in request.
   +   *
   +   * \Drupal\Core\Routing\RedirectDestination::get() cannot be used directly
   +   * because it will use <current> if 'destination' is not in the query string.
   +   *
   +   * @return string|null
   +   *   The redirect path or NULL if it is not specified.
   +   */
   +  protected function getRedirectDestinationPath() {
   +    if ($this->getRequest()->get('destination')) {
   +      return $this->getRedirectDestination()->get();
   +    }
   +  }
   

   What's the point of these methods?

   getRedirectUrl() only calls getDestinationUrl(), so it's just a wrapper. We can definitely remove this one.

   getDestinationUrl() is a wrapper for getRedirectDestinationPath() that has a tiny bit of additional logic.

   Finally,
   getRedirectDestinationPath() is a wrapper for getRedirectDestination() that has a tiny bit of additional logic.

4. +++ b/core/modules/outside_in/src/OffCanvasFormDialogTrait.php
   @@ -0,0 +1,114 @@
   +      if ($redirect_url = $this->getRedirectUrl()) {
   

   This is where it's finally used.

   I think a single method would be far clearer. And you're still relying on \Drupal\Core\Routing\RedirectDestination::get() implicitly, with additional layers on top. I think having your own code would actually be simpler — something like:

   protected function someMethodName() {
     $query = $this->requestStack->getCurrentRequest()->query;
     if ($query->has('destination') && !UrlHelper::isExternal($query->get('destination'))) {
       return Url::fromUserInput('/' . $query->get('destination'));
     }
     return NULL;
   }
   

5. +++ b/core/modules/outside_in/tests/modules/off_canvas_test/src/Form/TestForm.php
   @@ -0,0 +1,59 @@
   +  use OffCanvasFormDialogTrait;
   

   Ok so this is the only other place where the trait is used — but why aren't we testing something artificial test-only form, rather than the actual form?

6. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OffCanvasTest.php
   @@ -120,4 +120,32 @@ protected function getTestThemes() {
   +  public function testFormErrors() {
   +    $web_assert = $this->assertSession();
   +    $page = $this->getSession()->getPage();
   +
   +    // Submit form with no error and sending a destination.
   +    $this->drupalGet('/off-canvas-test-links');
   +    $page->clickLink('Show form!');
   +    $this->waitForOffCanvasToOpen();
   +    $page->pressButton('Submit');
   +    $web_assert->assertWaitOnAjaxRequest();
   +    // Make sure the changes are present.
   +    $this->assertNotEmpty($web_assert->waitForElement('css', 'div.messages.messages--status:contains(submitted)'));
   +    $web_assert->elementNotContains('css', 'body', 'Validation error');
   +
   +    // Submit form with an error and sending a destination.
   +    $this->drupalGet('/off-canvas-test-links');
   +    $page->clickLink('Show form!');
   +    $this->waitForOffCanvasToOpen();
   +    $page->checkField('Force error?');
   +    $page->pressButton('Submit');
   +    $web_assert->assertWaitOnAjaxRequest();
   +    $web_assert->elementNotContains('css', 'body', 'submitted');
   +    $web_assert->elementContains('css', '#drupal-off-canvas', 'Validation error');
   +  }
   

   In other words: why not provide a custom block whose off-canvas form has this kind of form, and then apply this test coverage to the actual Settings Tray form for that block?

   Then we have all this test coverage for the edge cases, and it'll be applied to the actual form where we'll want it to be working :)

@Wim Leers thanks for the review.
re #80 1,2
Originally I got the trait, in another form from @jrockowitz's work in Webform because he was using something similar in #2866554: Add Quick Edit off canvas form.

The original idea was that other modules in contrib that put form in the tray and would run into the same problem. But as know now this a dialog problem not a off-canvas dialog problem so #2896535: Create a helper trait for Forms in ajax dialogs

Yes it makes sense now to move this logic back to into BlockEntityOffCanvasForm. Since it is internal trait contrib can't use it anyways.

2. trait gone
3, 4. Simplified this down to \Drupal\outside_in\Block\BlockEntityOffCanvasForm::getRedirectUrl()
Not checking UrlHelper::isExternal($query->get('destination')) because other modules should not be using this because of #2894584: Settings Tray provides functionality, not an API: mark PHP and JS as internal
Not sure when this would be external.
5. Removed /core/modules/outside_in/tests/modules/off_canvas_test/src/Form/TestForm.php this was added when I thought the trait might be used by other modules so I thought generic testing would be good. No longer needed.
6. The only thing that is not already tested in \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks() is validation messages shown in off-canvas dialog. The redirect is testing when saving the form.
Added \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testValidationMessages()

Thanks for the backstory! Adding those as related issues.

YAY, so much simpler now!

1. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
   @@ -111,4 +117,80 @@ protected function getPluginForm(BlockPluginInterface $block) {
   +    if ($this->getRequest()->get('destination')) {
   

   Using \Symfony\Component\HttpFoundation\Request::get() is a bad practice, because it inspects the query string, request attributes and request body. Using getRequest()->query->has('destination') seems better.

2. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
   @@ -111,4 +117,80 @@ protected function getPluginForm(BlockPluginInterface $block) {
   +      if ($destination = $this->getRedirectDestination()->get()) {
   

   This if-test can be combined with the previous one.

3. +++ b/core/modules/outside_in/tests/modules/outside_in_test/outside_in_test.module
   @@ -0,0 +1,30 @@
   +function outside_in_test_form_block_off_canvas_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   +  $form['settings']['label']['#element_validate'][] = '_outside_in_test_validate_title';
   +
   +
   +}
   +
   +
   +/**
   + * Element validate callback test form validation messages.
   + *
   + * @param $element
   + *   An associative array containing the properties and children of the
   + *   generic form element.
   + * @param $form_state
   + *   The current state of the form.
   + * @param array $complete_form
   + *   The complete form structure.
   + */
   +function _outside_in_test_validate_title(&$element, FormStateInterface $form_state, &$complete_form) {
   +  if ($form_state->getValue(['settings', 'label']) == 'Block label') {
   +    $form_state->setError($element, 'Meta label error.');
   +  }
   +}
   

   Why not instead add this to a \Drupal\outside_in_test\Plugin\Block\OutsideInTestBlock?

1. fixed
2. fixed.
3 Moved a block plugin. change to always through error. Improved test error message.

This patch removes unused use statements and fixes #62, #63, and #71.1.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -970,7 +970,7 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
-    else {
+    elseif (!isset($element['#attributes']['data-drupal-selector'])) {

This fixes a pre-existing FAPI bug where HEAD is clobbering the pre-existing data-drupal-selector that got set in prepareForm(). Possibly this should be split into a separate issue, in which case we can go back to hard-coding the HTML ID in BlockEntityOffCanvasForm until that separate issue is fixed. However, I posted #85 to see if it passes tests.

I was testing manually I found this weird problem. When submitting form provided by \Drupal\outside_in_test\Plugin\Block\ValidationErrorBlock

This form always produces an error for testing.
First time generating form

You can see here it is directly nested under the 1 div.

but then each time you submit it gets wrapped in a new div. So eventually

😦

This happens with both #84 and #85.

@tedbow - #87 should be fixed with #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs.

@drpal thanks for info.

Ok setting it backs to need review. I tested the patch mentioned in #88 and it fixes the issue.

I opened #2897377: $form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose to address #86, and updated this patch to work around the bug in the meantime.

@drpal thanks for pointing to that issue, agreed that that's the cause.
@effulgentsia thanks for #85 + #86 + #90! Agreed that it makes sense to add a work-around here and have a separate issue at #2897377: $form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose. Also: I'd never have found that bug :O

This is looking great — I'm only finding nits now :) After these are addressed, I'll RTBC this.

1. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
   @@ -111,4 +116,79 @@ protected function getPluginForm(BlockPluginInterface $block) {
   +        throw new \Exception("No destination provide for Settings Tray form");
   

   s/provide for/provided by/

2. +++ b/core/modules/outside_in/tests/modules/outside_in_test/src/Plugin/Block/ValidationErrorBlock.php
   @@ -0,0 +1,33 @@
   +    $form_state->setError($form['label'], "\u{1F525} Sorry system error. Please save again. \u{1F61C}");
   

   Note that this is using PHP 7 syntax … which means it doesn't actually work in PHP 5.5 & 5.6. In those versions, it'll just print these literally.

   It doesn't really matter since this is never asserted.

   But it's probably wise to just use the Unicode characters directly here.

   See for yourself at https://3v4l.org/Of6HV

3. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -490,4 +503,27 @@ protected function isLabelInputVisible() {
   +      // Use label that will trigger validation error.
   +      // @see _outside_in_test_validate_title
   

   This comment is outdated.

4. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -490,4 +503,27 @@ protected function isLabelInputVisible() {
   +      $page->fillField('settings[label]', 'Block label');
   

   And even this can be removed — the validation error will occur 100% of the time now.

@effulgentsia #90 thanks for opening issue and the fix.

@Wim Leers thanks for (final?🙃) review!
1. fixed
2. ☹️ Ok tried HTML entities but not all are supported. Not using emojis, core will be a little less fun. Fixed
3. Removed this comment and added 1 about the error always happening
4. Removed

2. You could have just used emojis directly!

Ticking credit boxes for reviewers.

Pushed to 8.4.x. Thanks all!

@effulgentsia thanks for your feedback and improvement to the issue and committing! Thanks all!

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