Allow Webforms to be submitted using AJAX

Moving to Webform queue, see #2827845: [roadmap] YAML Form 8.x-1.x to Webform 8.x-5.x.

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? (:

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

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

I'll take a look now...

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!

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

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

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?

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

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.

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.

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

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.

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

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

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.

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!

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?

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.

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?

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

Is this in the -dev branch now?

@jrockowitz that's awesome, thanks!

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!

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

Update -- made a comment here and then found the bug (which was not related to this module) almost immediately afterward.

Is there any way to alter te position of the message to the bottom of the form and add an extra class to it? or wrap it?