Problem/Motivation

When a user makes a change using the off-canvas tray and saves the message that the change was successful appears on the site page. This is a bit odd since the expected behavior of submitting a form in Drupal is that the messages appear at the top of the form.

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

Proposed resolution

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

Remaining tasks

Move the messages to the tray.

User interface changes

Messages appear in the tray

API changes

Data model changes

None

Comments

tkoleary created an issue. See original summary.

naveenvalecha’s picture

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

Version: 8.2.x-dev » 8.2.0
tkoleary’s picture

Issue tags: +Usability
tkoleary’s picture

Issue tags: +sprint
tedbow’s picture

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

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

Status: Needs review » Needs work

The last submitted patch, 7: 2785047-7.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
14.84 KB
5.74 KB

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);
  }

Status: Needs review » Needs work

The last submitted patch, 9: 2785047-9.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
14.9 KB
2.15 KB

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.

drpal’s picture

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

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.32 KB
15.06 KB

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

Status: Needs review » Needs work

The last submitted patch, 13: 2785047-13.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
662 bytes

Fixing the test

tedbow’s picture

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

tkoleary’s picture

This is way better.

Wim Leers’s picture

Title: In edit mode messages should appear in the off-canvas tray, not the main page. » In Outside In mode, messages should appear in the off-canvas tray, not the main page
Status: Needs review » Needs work
Issue tags: +JavaScript, +Needs issue summary update
  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.

tedbow’s picture

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

tedbow’s picture

Status: Needs work » Needs review
FileSize
17.03 KB
22.11 KB

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.