Problem/Motivation

When YAML forms are attached to a page via an entity reference or a block it would help user experience to allow the form to be submitted using AJAX.

Proposed resolution

Allow YAML forms to be submitted using AJAX

Remaining tasks

TBD

User interface changes

TBB

Development Notes

  • Branch Name: 2757491-submit-via-ajax
  • Commit message: Issue #2757491: Allow YAML forms to be submitted using AJAX

References

Comments

jrockowitz created an issue. See original summary.

jrockowitz’s picture

Issue summary: View changes
jrockowitz’s picture

Drupal AJAX integration is not fully documented and there are some more immediate issues that need to be addressed. So I am going to hold off implementing this feature.

jrockowitz’s picture

Status: Active » Postponed
fenstrat’s picture

Title: Allow YAML forms to be submitted using AJAX » Allow Webforms to be submitted using AJAX
Project: YAML Form » Webform
Version: 8.x-1.x-dev » 8.x-5.x-dev
jrockowitz’s picture

This feature could be handled by the port of the Webform AJAX module to D8

jrockowitz’s picture

Issue summary: View changes
AndyF’s picture

Status: Postponed » Active

Hey @jrockowitz, I'm starting the ball rolling on this. AFAICT there hasn't been a start on webform_ajax 8. Would you prefer me to submit a patch here or there? (:

jrockowitz’s picture

@AndyF Similar to the Webform View Integration module, I think it is perfectly okay to start with submitting a patch. First off, you will most likely get more followers and reviewer within the Webform issue queue.

Personally, I think AJAX integration should be part of Webform core, similar to how Views includes AJAX integration... all you need to do is check the 'Use AJAX' box to ajaxify a View.

Still, I must emphasize that this feature requires test coverage, which is now possible using Drupal's JavaScript testing framework.

I recommended looking at the WebformDialogTrait, which handles how admin and element setting forms are ajaxified within modal dialogs. In theory, you might be able to write a trait that enables Ajax support for ANY entity form. At the very least, the trait could support AJAX validation.

Finally, you should see if any of your work could be applicable to the Examples module.

Lastly... thanks.

AndyF’s picture

Status: Active » Needs review
FileSize
14.45 KB

So I feel a bit silly - having asked what you'd prefer, I found it easier to develop a standalone module while I'm still trying to get an idea of Webform's overall architecture (and because I shamelessly stole from contact_ajax which is also standalone). I've created a sandbox for it but am very happy for you to fold it into webform.

Hopefully I can trigger testbot by attaching as a patch...

jrockowitz’s picture

@AndyF I am getting a little swamped with issues and requests. I think a dedicated Webform Ajax sandbox module is the way to go.

I will do some initial review of your sandbox module and then add it the Webform Add-Ons list.

Status: Needs review » Needs work

The last submitted patch, 10: webform-ajax_submission-2757491-10-D8.patch, failed testing.

AndyF’s picture

@jrockowitz thanks, I appreciate any time you can spare, I'm still far from feeling confident with Drupal 8!

Glad to see testbot worked (there's one known bug - Drupal messages stack up on multiple submissions - which is what's causing the failure).

jrockowitz’s picture

@AndyF I just tested your sandbox, your code works, and make complete sense to me. Could you experiment with getting AJAX callbacks to work within multiple step forms with preview and save draft enabled.

If you can get the ajax callbacks working and it is really just few lines of extra code. I can find the time to get the UX right.

jrockowitz’s picture

Also, checkout code in the D7 Webform Ajax module, I think the trick with multiple step AJAX forms is storing the unique but persistent 'webform_ajax_wrapper_id' in a hidden field or $form_state (which I would prefer).

jrockowitz’s picture

AndyF’s picture

ah my bad, I did check both webform and webform_ajax but somehow missed that :/

I'll take a look now...

jrockowitz’s picture

I think the Webform Ajax integration is definitely going to be a collaborative effort, which would be great since we should be able to resolve any outstanding issues/challenges and then add this to the Webform core module.

AndyF’s picture

K a quick look at the existing port and I think our efforts could complement each other's. It looks like @vanvlaanderen has multipage and working back links but no tests, so hopefully not too much duplicated effort!

AndyF’s picture

I got a chance to work on this yesterday:

  • I hadn't realised that the confirmation message is normally shown as a status message if confirmation type is set to 'message'. I've updated the code to match that behaviour.
  • Form resets on submission if it's redisplayed.
  • Incorporated @vanvlaanderen's 'Back to form' link handling and added test
  • Got multipage working (again many thanks to @vanvlaanderen!) and minimally tested
  • Clear old messages before displaying new ones
AndyF’s picture

Status: Needs review » Needs work

The last submitted patch, 20: interdiff-2757491-10-20.patch, failed testing.

AndyF’s picture

Status: Needs work » Needs review
FileSize
26.05 KB
24.25 KB

Silly me, forgot the 'Back to form' callback route...

dawehner’s picture

  1. +++ b/modules/webform_ajax/tests/src/FunctionalJavascript/WebformAjaxTest.php
    @@ -0,0 +1,232 @@
    + * @todo This lumps a number of different tests together. AJAX is quite a cross-
    + *   cutting concern and I'd welcome input on how best to slice up the tests.
    + */
    

    I'm wondering whether we could minimize the tests a bit by merging them together a bit aka. always add the required text field, but then in one test method:

    1. Forget to enter this required text field
    2. Posting it, ensure the message is shown
    3. Do another post to ensure that a new message is shown

    On the other hand having explicit named bits on the ajax webform is really nice for itself.

  2. +++ b/modules/webform_ajax/tests/src/FunctionalJavascript/WebformAjaxTest.php
    @@ -0,0 +1,232 @@
    +    $path = $webform->toUrl()->getInternalPath();
    ...
    +    $this->drupalPostForm($path, ['textfield' => ''], t('Submit'));
    ...
    +    $path = $webform->toUrl()->getInternalPath();
    ...
    +    $this->drupalPostForm($path, ['textfield' => 'test value'], t('Submit'));
    

    Is it possible to pass in an URL object here, so just use

    $this->drupalPostForm($webform->toUrl(),
     )
  3. +++ b/modules/webform_ajax/tests/src/FunctionalJavascript/WebformAjaxTest.php
    @@ -0,0 +1,232 @@
    +  public function testMessages() {
    

    This one feels a bit redundant with the other tests

  4. +++ b/modules/webform_ajax/tests/src/FunctionalJavascript/WebformAjaxTest.php
    @@ -0,0 +1,232 @@
    +    $assert->assertWaitOnAjaxRequest();
    +    $element = $assert->waitForElement('css', '.webform-confirmation');
    

    Oh that is tricky. There is indeed no new element popping up, right?

  5. +++ b/modules/webform_ajax/tests/src/FunctionalJavascript/WebformAjaxTest.php
    @@ -0,0 +1,232 @@
    + * @todo: Move this to a sensible namespace (but where?)
    

    Can't you put it into Drupal\Tests\webformweb / webform/tests/src

  6. +++ b/modules/webform_ajax/tests/src/FunctionalJavascript/WebformAjaxTest.php
    @@ -0,0 +1,232 @@
    + * @todo: Add some helper methods for adding elements to a webform. I think it
    + *   will be neater than FAPI arrays into ::createWebform().
    

    That indeed looks quite ugly :)

  7. +++ b/modules/webform_ajax/tests/src/FunctionalJavascript/WebformAjaxTest.php
    @@ -0,0 +1,232 @@
    +class WebformProvider {
    ...
    +class WebformAjaxProvider extends WebformProvider {
    ...
    +  public function createWebform($values = []) {
    

    At least for me there seems to be nothing in those helper classes which requires them to be classes. Couldn't you just use methods which call each other, like on a trait?

  8. +++ b/modules/webform_ajax/webform_ajax.module
    @@ -0,0 +1,134 @@
    + * @todo Scroll up on submission.
    

    Its a bit sad ... views already provides an ajax command which does exactly that: \Drupal\views\Ajax\ScrollTopCommand. I think you should be able to copy exactly from it.

  9. +++ b/modules/webform_ajax/webform_ajax.module
    @@ -0,0 +1,134 @@
    +  if (!in_array($confirmation_type, ['message', 'inline'])) {
    

    For in_array we should pass TRUE ($strict) as third parameter, otherwise you might run into the problem shown in https://3v4l.org/jX5Ze

  10. +++ b/modules/webform_ajax/webform_ajax.module
    @@ -0,0 +1,134 @@
    +    'effect' => 'fade',
    

    I assume there aren't many usecases to make that configurable?

  11. +++ b/modules/webform_ajax/webform_ajax.module
    @@ -0,0 +1,134 @@
    +/**
    + * Ajax callback for webform.
    + */
    ...
    +  // Get the results of drupal_set_message() calls.
    +  $messages = StatusMessages::renderMessages(NULL);
    ...
    +  $output[] = $messages;
    

    Note: \Drupal\Core\Render\MainContent\AjaxRenderer::renderResponse does a slightly simpler approach aka. just use ['#type' => 'status_messages']

  12. +++ b/modules/webform_ajax/webform_ajax.module
    @@ -0,0 +1,134 @@
    +    /** @var \Drupal\webform\WebformMessageManagerInterface $message_manager */
    

    There is no variable $message_manager hanging around :)

  13. +++ b/modules/webform_ajax/webform_ajax.module
    @@ -0,0 +1,134 @@
    +    if ($webform->getSetting('confirmation_type') == 'message') {
    

    Let's use ===

  14. +++ b/modules/webform_ajax/webform_ajax.module
    diff --git a/webform.info.yml b/webform.info.yml
    index 705be15..8f2b93c 100644
    
    index 705be15..8f2b93c 100644
    --- a/webform.info.yml
    
    --- a/webform.info.yml
    +++ b/webform.info.yml
    
    +++ b/webform.info.yml
    +++ b/webform.info.yml
    @@ -8,3 +8,5 @@ dependencies:
    
    @@ -8,3 +8,5 @@ dependencies:
       - 'drupal:field'
       - 'drupal:system (>= 8.2)'
       - 'drupal:user'
    +test_dependencies:
    +  - webform_ajax
    

    Is that really needed? Do the tests in subdirectories don't run automatically?

AndyF’s picture

Thanks for the review @dawehner!

1. I've been trying to make the setup explicit and at the start of the test, rather than hidden in either setUp() or createWebform(). I realise it does lead to slightly top-heavy test cases and repetition. Happy to go with whatever people prefer (:

2. Yes! Thank you!

3. It's a specific regression test, so perhaps would be good to keep it separate. It detected the problem with implementing point 11.

4. Yeah :/ Will investigate using the throbber.

5. Apparently not, the autoloader isn't happy with that (or it's me being silly).

6. :p Not at the top of the priority list just yet

7. At the moment they don't need to be classes because they have no state, but I thought they might have some at some point. I realise the more normal thing to do would just be to add methods on a base class, but I've seen the object creator pattern before and found it quite neat personally.

8. Thanks! Will use that

9. Thanks, added.

10. I wanted to focus on core behaviour first, and add things like animation customisation second. (I'm sure someone _will_ want to configure it at some point!)

11. Hmmn, looks like my #prefix and #suffix will get overridden if I go that approach.

12. How'd that crafy little beggar get there?! Thanks!

13. Done

14. Dunno, will give it a go and find out...

dawehner’s picture

Some additional feedback ...


This description is super pointless ... Let's just drop it, the label of the checkbox explains enough already.

  1. +++ b/modules/webform_ajax/js/webform_ajax.js
    @@ -0,0 +1,23 @@
    +        .once('webform_ajax')
    
    +++ b/modules/webform_ajax/webform_ajax.libraries.yml
    @@ -0,0 +1,7 @@
    +  js:
    +    js/webform_ajax.js: {}
    +  dependencies:
    +    - core/jquery
    +    - core/drupalSettings
    

    It'd be nice to have an explicit dependency on - core/jquery.once.

  2. +++ b/modules/webform_ajax/src/Controller/WebformAjaxController.php
    @@ -0,0 +1,43 @@
    +   */
    +  public function return_webform(WebformInterface $webform, $wrapper_id) {
    

    Nitpick: Missing @return statement.

  3. +++ b/modules/webform_ajax/src/Controller/WebformAjaxController.php
    @@ -0,0 +1,43 @@
    +    $response->addCommand(new RemoveCommand("#$wrapper_id-status-messages"));
    

    Ideally you could set a data- attribute on the element and then use it here. By this we don't have classes/ids while still making it possible to access it, see core/modules/big_pipe/js/big_pipe.js:68 as random example

  4. +++ b/modules/webform_ajax/src/Controller/WebformAjaxController.php
    @@ -0,0 +1,43 @@
    +      $output = \Drupal::entityManager()->getViewBuilder('webform')->view($form);
    

    Note: You can use ::getEntityManager().

  5. +++ b/modules/webform_ajax/src/Controller/WebformAjaxController.php
    @@ -0,0 +1,43 @@
    +      $output = array (
    +        '#type' => 'markup',
    +        '#markup' => t('The webform cannot be displayed.')
    +      );
    

    Do you mind using short array syntax here?

  6. +++ b/modules/webform_ajax/tests/src/FunctionalJavascript/WebformAjaxTest.php
    @@ -0,0 +1,329 @@
    +class WebformAjaxTest extends JavascriptTestBase {
    

    Note: I would have ordered the test methods in a different order. For example I would start with testAjaxSubmissionStorage then continue with testRequiredTextField or something like that. I know this is super nitpicky, but I like a nice readable test. This also makes it easier for reviewers, to be honest.

  7. +++ b/modules/webform_ajax/tests/src/FunctionalJavascript/WebformAjaxTest.php
    @@ -0,0 +1,329 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function setUp() {
    +    parent::setUp();
    +    $this->provider = new WebformAjaxProvider();
    ...
    +
    +  /**
    +   * @var \Drupal\Tests\webform_ajax\FunctionalJavascript\WebformProvider $provider
    +   */
    +  protected $provider;
    +
    

    Let's please use the convention used in Drupal usually, aka. move the constructor/setup method to the top as well as any property we use ...

  8. +++ b/modules/webform_ajax/webform_ajax.module
    @@ -0,0 +1,267 @@
    + * Questions:
    + * - Is there any point in allowing AJAX submissions of forms with a redirect
    + *   confirmation?
    

    In an ideal world you would render the confirmation inline, right?

  9. +++ b/modules/webform_ajax/webform_ajax.module
    @@ -0,0 +1,267 @@
    + * @todo Add more tests for server-side validation.
    + * @todo Check the implementation against the reference on #2757491 and
    + *   https://api.drupal.org/api/drupal/core%21core.api.php/group/ajax/8.3.x
    + * @todo Remove inappropriate confirmation options.
    + * @todo Expand test coverage:
    + *   - multiple step forms with preview and save draft enabled
    + * @todo I think I saw that webform automatically loads from MODULE.webform.inc.
    + * @todo Scroll up on submission.
    

    It doesn't sound like any of these@todos needs to be addressed now.

jrockowitz’s picture

+++ b/modules/webform_ajax/webform_ajax.module
@@ -0,0 +1,271 @@
+function webform_ajax_form_webform_third_party_settings_form_alter(&$form, FormStateInterface $form_state) {

I am about to commit #2875841: Hide third party tab (at least optionally). You will need to change 'webform_ajax_form_webform_third_party_settings_form_alter' to 'webform_ajax_webform_third_party_settings_form_alter'

basil_snowman’s picture

ziomizar’s picture

Hi basil_snowman,

Thanks for your patch.

When you write a patch its good practice upload also an interdiff (https://www.drupal.org/documentation/git/interdiff) in this way its easier see what differ between patches.

AndyF’s picture

@jrockowitz Thanks for the update, that would've had me scratching my head a bit! I've updated the code in the sandbox, but haven't posted a patch (that's the only change). (I still intend to look at @dawehner's feedback.)

jrockowitz’s picture

@AndyF I have just committed #2878193: Allow actions (aka submit buttons) to be placed anywhere on a webform which should not have any impact on your code because your #ajax callbacks should still be attached to the $form['actions'].

AndyF’s picture

I've responded to some more of @dawehner's comments:

1. Done
2. Added.
3. On the todo list (:
4. I'm using \Drupal\Core\Controller\ControllerBase::entityTypeManager(), I couldn't find an appropriate ::getEntityManager().
5. Done.
6. I've reordered the tests, not 100% sure if the order's perfect...
7. Sorry, wasn't done intentionally! I've moved them up top.
8. No, because the confirmation is a redirect. However before the redirect can happen the form must validate - and it's nice to do that with AJAX, so there is a good reason to have AJAX redirects I think!

Here's the latest patch which includes clean-up based on the feedback above, and also supports page and URL confirmation types.

AndyF’s picture

@jrockowitz I think it's getting to the point where we should decide if webform_ajax should remain as a standalone module, or if it gets folded into webform proper (there are some ugly bits in the code where I suspect the ideal solution will change based on this).

Status: Needs review » Needs work

The last submitted patch, 32: webform-ajax_submission-2757491-32-D8.patch, failed testing.

AndyF’s picture

It looks like I can't assume <front> maps to / with testbot. (The tests pass for me locally.)

dawehner’s picture

It looks like I can't assume maps to / with testbot. (The tests pass for me locally.)

Right, Drupal is installed in a directory on the testbot. $this->getAbsoluteUrl() might help you.

jrockowitz’s picture

@AndyF I would like to include Ajax support in the core Webform module as an out-of-the-box feature.

ABaier’s picture

Nice to hear, @jrockowitz! Especially for the Multistep Forms Ajax would be a great plus, which would allow them to be used inside blocks or paragraphs easily. Would love to see that coming!

hanoii’s picture

For what it's worth, I am starting to try this patch. On a simple form it seems to be working fine, one thing I noticed is that on forms that the submissions are disabled the thank you message is not displayed.

I think this belongs more to webform than this patch. Reported it on #2883894: A webform's state is always STATE_UNSAVED if submissions are disabled.

Also, there's a lot of work put into this patch. What can it be done to be included in webform. Is there anything I can do to help?

hanoii’s picture

AndyF’s picture

Thanks @hanoii, off the top of my head

  1. there's a broken test (due to the testbot running them in a subdir)
  2. Check support for and add tests for draft and preview
  3. Moving from a submodule to part of webform
  4. There might be some other bits and pieces in @todos.
jrockowitz’s picture

Status: Needs work » Needs review
FileSize
32.93 KB
914 bytes

Attached patch fixes broken tests.

  • jrockowitz committed c7042b8 on 2757491-ajax
    Issue #2757491 by AndyF, jrockowitz, basil_snowman, dawehner: Allow...

  • jrockowitz committed 9aefaac on 2757491-ajax
    Issue #2757491: Allow Webforms to be submitted using AJAX. Add setting.
    

  • jrockowitz committed a46869a on 2757491-ajax
    Issue #2757491: Allow Webforms to be submitted using AJAX. Initial setup...

  • jrockowitz committed 9a5b9c7 on 2757491-ajax
    Issue #2757491: Allow Webforms to be submitted using AJAX. Add warning...

  • jrockowitz committed 9ff7a5c on 2757491-ajax
    Issue #2757491: Allow Webforms to be submitted using AJAX. Restore...

  • jrockowitz committed ebe1aae on 2757491-ajax
    Issue #2757491: Allow Webforms to be submitted using AJAX. Write tests.
    

  • jrockowitz committed 9ef68b0 on 2757491-ajax
    Issue #2757491: Allow Webforms to be submitted using AJAX. Remove...
jrockowitz’s picture

@AndyF Your patch was so unbelievably helpful. It immediately highlighted my mistakes with how inline confirmations and confirmation messages were being handled. Your code also provided the needed helper functions and perfect work-arounds. I had thought it would take a few days to get this integrated and it only took a few hours.

Notes

  • I refactored how the inline confirmation and confirmation message were setup so that I could setup a generic WebformAjaxFormTrait which isolates all the Ajax help functions.
  • I renamed a few method in WebformDialogFormTrait to correspond with the new WebformAjaxFormTrait.
  • At some point, I would like to merge WebformDialogFormTrait and WebformAjaxFormTrait to remove some code duplication and cleanup redirect handling.
  • If all goes well with this ticket, we should be able to also support opening a Webform in a Ajax dialog/modal.
  • WebformAjaxJavaScriptTest are the first JS tests included in the Webform module, for now I am trying to keep them as simple as simple possible.
  • View all 'Test: Ajax' form by enabling the webform_test module as going to /admin/structure/webform?category=Test%3A%20Ajax

Status: Needs review » Needs work

The last submitted patch, 50: allow_webforms_to_be-2757491-50.patch, failed testing. View results

jrockowitz’s picture

Status: Needs work » Needs review

Something is up with the testbot.

All the tests are passing locally, so feel free to download and review the patch.

hanoii’s picture

I'll review, but do you think this one could still be necessary: #2883894: A webform's state is always STATE_UNSAVED if submissions are disabled?

  • jrockowitz committed 36774e6 on 2757491-ajax
    Issue #2757491: Allow Webforms to be submitted using AJAX. Refactor...
jrockowitz’s picture

Attached patch, refactors the WebformDialogFormTrait to use the WebformAjaxFormTrait and removes most of the duplicate code.

dotist’s picture

This is great! #55 is working for me.
A note to those using a custom template for your webform: you must ensure that you've got the right #id on your form for the ajax response to work. I passed it to my template in hook_preprocess_webform() with $variables['element']['#form_id']

jrockowitz’s picture

I think adding adding a $form['#form_wrapper_id'] property should help with customized form templates.

  • jrockowitz committed 1b428ff on 2757491-ajax
    Issue #2757491: Allow Webforms to be submitted using AJAX. Add #...
mikeohara’s picture

Is this in the -dev branch now?

  • jrockowitz committed b47dae1 on 8.x-5.x
    Issue #2757491 by AndyF, jrockowitz, basil_snowman, dawehner: Allow...
jrockowitz’s picture

Status: Needs review » Fixed

I committed the committed the patch. Please download the latest dev release to review.

AndyF’s picture

@jrockowitz that's awesome, thanks!

5n00py’s picture

Wow! Great job. Works well for me.
I used

Message (reloads the current page/form and displays the confirmation message at the top of the page.)

And message was shown on top of form.

Thanks!

Lennard Westerveld’s picture

Oops my bad :) the fault was in a other patch i applied.