Postback: Prevent form validation double-submits

[EricDunsworth]

The form validation plugin's pass/fail logic was interfering with in-progress form submissions.

Repeatedly submitting postback forms on slower internet connections could trick the aforementioned logic into disengaging forms that had already been submitted (but had yet to receive an HTTP response code). That made it possible to initiate multiple parallel submissions.

This fixes it as follows:

• Replaces the form element's data-wb-engaged attribute with purpose-specific ones:
  • data-wb-blocked: Indicates the form validation plugin failed (to prevent invalid forms from getting submitted).
  • data-wb-sending: Indicates the form is currently being sent (to prevent duplicate submissions... like repeatedly submitting before the first submission finishes)
• Simplifies how pressed submit buttons are detected:
  • Uses the submit() event's submitter property to detect pressed submit buttons (instead of juggling with data-wb-engaged to figure out what was pressed)
  • Note: Changes the plugin to match default browser behaviour:
    • Old behaviour: Submitting a form in JS mode without pressing a button didn't send any submit button variables
    • New behaviour: Same scenario now causes the first submit button's variable to be included in the form submission (since submitter maps to the first submit button if none were directly pressed)

Successor of sorts to #8924 and #9197.

CC @GormFrank @polmih

[EricDunsworth]

FYI I did most of my before/after testing in Firefox with its dev tools' console or network panel open.

Also tried a bit of quick testing in Edge, but was unable to replicate the bug in that browser. Maybe it's too fast to give me a chance to double-click :P. But it's probably still theoretically possible to trigger the issue in Chromium browsers.

[duboisp]

Pre-approved upon successful testing and after we are able to reproduce in FF.

[EricDunsworth]

Btw this'll probably need to be rebased after #9534 gets merged (former's diff shows an edit on the same else line this PR changes).

[duboisp]

Issue reproduction
I was able to reproduce by running the "click()" function 2-3 time consecutively in browser JS console.
That situation is only applicable for postback plugin that is combined with the form validation plugin.

$("main form:eq(2) button").get( 0 ).click(); $("main form:eq(2) button").get( 0 ).click();

As expected, I was able to observe the XHR request which was sent twice or as many consecutive repeated click instruction.

---

Test result

I confirm the solution do fix the issue but only if there was no error triggered by the form validation before the post back.

To reproduce the failed test:

1. Load the page
2. Ensure the required field of the form (3rd form in the working example) are empty.
3. Trigger the form submission: ex: $("main form:eq(2) button").get( 0 ).click(); $("main form:eq(2) button").get( 0 ).click();
4. Go in the erroneous field and type something to fulfill the requirement and remove the form validation error
5. Trigger the form submission: ex: $("main form:eq(2) button").get( 0 ).click(); $("main form:eq(2) button").get( 0 ).click();
6. Expected: The form are submitted only once
7. Fail result: The form are not submitted, like if the button state are still in "engaged" mode.

[EricDunsworth]

FYI I've just rebased and force-pushed to account for #9534 having been merged.

@duboisp Good catch on the failed test. I missed it in my own testing. I'll try to sort it out at some point this week (already missed today's release window :P).

[EricDunsworth]

I think I've figured it all out and have come up with a better fix (completely different from what was initially pitched). But it'll have to hold off until next week :P.

I'll force-push an update once I'm done polishing it up and do more testing from my end.

[duboisp]

Pre-approve for this week if the change are submitted by the end of day and upon testing/review completed before 9am tomorrow.

[EricDunsworth]

Here's my follow-up :)!

The underlying cause of the double-submit bug is that data-wb-engaged is being used for too many purposes at once:

• "Locking" invalid forms
• "Locking" forms that are in the process of being sent

If form validation fails, the form gets locked. If the user subsequently fixes their errors and resubmits the form, the form validation plugin's if/else condition "unlocks" it to allow submission to proceed.

The cause of the bug is that the form validation recovery logic always "unlocks" forms that pass - with no consideration for if a form submission is already in-progress. End result is that if a form is submitted multiple times before the first submission finishes, the validation recovery logic "undoes" the in-progress submissions' locks and allows extra submissions to be sent in parallel. The fun stops when postback's success/fail message appears.

IMO the likelihood of a user being able to exploit the bug in practice depends on their internet connection speed. A lightning-fast submission to localhost is unlikely to give anyone enough time to submit more than once. But a slow mobile connection could make it possible to initiate several submits before the first one finishes.

My first attempt at fixing the bug was a dud. I didn't fully understand what was going on behind-the-scenes and didn't test it thoroughly-enough. Plus the if else condition I pitched was silly (basically checks for the same thing it'll do if it gets processed :D lol).

My next attempt will scrap the data-wb-engaged attribute and replace it with blocked/sending attributes. That should fix the bug once and for all.

[EricDunsworth]

Just updated the OP and force-pushed a reworked commit :)!

My only remaining concern is that using the submitter property might count as an API change because it behaves differently vs what was there before. But the old behaviour probably shouldn't of existed in the first place because it's inconsistent with default browser behaviour.

When pressing Enter to submit a form while focus is on a normal form field (like a text field), browsers treat it as if the first submit button was pressed. So they add the first submit button's variable (name/value attributes) as a form parameter. WET's noscript and basic HTML modes do that too.

But in WET's JS mode... the postback plugin previously excluded submit button variables from form params unless a submit button was literally pressed. So pressing Enter via a keyboard didn't bring any submit button variables along for the ride.

The new behaviour will cause WET's JS mode to match default browser behaviour, but it's probably going to mess up the page feedback tool (PFT) (#2098) since that uses multiple submit buttons with variables.

@duboisp Thoughts?

[duboisp]

I reviewed the code and tested locally, I confirm it works as expected and following the default browser behaviour is the right way to follow.

Regarding the type of change, this remain a patch because it's impact can't be measure in feature addition neither in feature removal according to our versioning API. The special data attribute we had was only managed by the code for the support of the form submission logic.

Regarding the impact on the feedback component in GCWeb, we will simply update our implementation to consider this update which is now aligned with the HTML5 form submission process.

Nice work on this @EricDunsworth