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.

tedbow’s picture

Issue tags: +Needs reroll

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

tedbow’s picture

Status: Needs review » Needs work
Jo Fitzgerald’s picture

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

Rerolled.

Status: Needs review » Needs work

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

tedbow’s picture

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

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
7.26 KB
17.14 KB

Split offcanvas into two words in other places.

tedbow’s picture

@Jo Fitzgerald reroll look good now! Thanks!

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

Status: Needs review » Needs work

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

tedbow’s picture

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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.01 KB
17.44 KB

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.

Status: Needs review » Needs work

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

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.74 KB
17.88 KB

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.