Forms: Add form fill duration to feedback - #45786
Conversation
enejb
added
Enhancement
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
Bot
added
[Feature] Forms
[Plugin] Jetpack
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds form fill duration tracking to Jetpack forms, measuring the time from a user's first interaction with a form to submission. This feature provides valuable analytics data for form optimization.
- Tracks form fill duration in seconds from first user interaction to submission
- Persists duration data in feedback entries and exposes it via REST API
- Adds frontend JavaScript tracking and backend PHP support
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/plugins/jetpack/changelog/add-forms-submit-timer | Changelog entry for Jetpack plugin |
| projects/packages/forms/changelog/add-forms-submit-timer | Changelog entry for forms package |
| projects/packages/forms/src/contact-form/class-feedback.php | Adds form_fill_duration property and getter method |
| projects/packages/forms/src/contact-form/class-contact-form.php | Adds hidden input field and focusin event handler |
| projects/packages/forms/src/contact-form/class-contact-form-endpoint.php | Adds form_fill_duration to REST API schema and response |
| projects/packages/forms/src/modules/form/view.js | Implements JavaScript tracking logic for form interaction timing |
| projects/packages/forms/tests/php/contact-form/class-utility.php | Updates test utility to support form_fill_duration in test requests |
| projects/packages/forms/tests/php/contact-form/Feedback_Test.php | Adds test for form_fill_duration persistence |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Coverage SummaryCoverage changed in 5 files.
Full summary · PHP report · JS report Coverage check overridden by
Covered by non-unit tests
|
|
This PR has been marked as stale. This happened because:
If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation. If the PR is not updated (or at least commented on) in another month, it will be automatically closed. |
| const duration = Math.round( ( Date.now() - context.formFirstInteractionTime ) / 1000 ); // Duration in seconds | ||
| const form = document.getElementById( 'jp-form-' + context.formHash ); | ||
| if ( form ) { | ||
| const durationField = form.querySelector( 'input[name="form_fill_duration"]' ); |
There was a problem hiding this comment.
This storing/calculating in context, then putting the value into input, then submitting the form made me think that we could have these "in memory" type things submitted directly from context as part of submitForm() function, without needing to inject the values first into the DOM.
...but this works now so it's fine :-)
There was a problem hiding this comment.
I looked at doing exactly this, but it turns out the DOM round-trip is load-bearing: non-AJAX forms never reach submitForm() at all — they fall through the handler and the browser submits natively from the DOM. So passing the value through submitForm() would only cover the AJAX path, and would reintroduce the bug Copilot flagged just above (the duration silently staying at its default for non-AJAX submissions).
Keeping the hidden input as the single channel means both paths work the same way. I did move the write out of the useAjax branch so it happens for both, and it now reuses the existing getForm() helper from shared.ts instead of re-inlining the lookup.
If we ever drop non-AJAX submissions entirely, your version is definitely the nicer one.
There was a problem hiding this comment.
Oh yah, I would think in 2026 non-AJAX doesn't really matter anymore. :-)
There was a problem hiding this comment.
I went and checked, and non-AJAX is still reachable through a current product feature rather than just legacy fallback — useAjax is is_response_without_reload_enabled && ! $has_custom_redirect, and has_custom_redirect() is true whenever the form's confirmation type is "redirect" with a customThankyouRedirect URL set. That's the "Redirect to another webpage" option in the form block's confirmation settings (blocks/contact-form/edit.tsx), so any form using it submits natively today.
There's also the plain no-JS case, where the view script never runs at all.
I verified all three paths on a JN site with the current branch:
| Path | Fill time | Recorded |
|---|---|---|
| AJAX (default) | 7s | 7 |
| Non-AJAX (native POST) | 12s | 12 |
| JavaScript disabled | – | null |
The last row is why the hidden input now defaults to empty instead of '0': when the duration is genuinely unknown we store null, so it stays distinguishable from a form that really was filled out in under a second. REST returns {"form_fill_duration": null} for that case, matching the declared array( 'integer', 'null' ) type.
0d5249f to
158412e
Compare
|
This PR has been marked as stale. This happened because:
If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation. If the PR is not updated (or at least commented on) in another month, it will be automatically closed. |
|
This PR has been automatically closed as it has not been updated in some time. If you want to resume work on the PR, feel free to restore the branch and reopen the PR. |
158412e to
21155af
Compare
c31aae4 to
4ee4681
Compare
enejb
added
the
Covered by non-unit tests
kraftbj
left a comment
kraftbj
left a comment
There was a problem hiding this comment.
Two things I'd fix before merge:
- The timer is never cleared. A second submission in the same page load reports the time since the first fill started. AJAX is on by default and the success panel ships a "Back" link by default, so this is the path most sites get.
absint()is doing validation it can't do.'abc'stores0,'-1'stores1, and'-9223372036854775808'comes back as a float, contradicting thearray( 'integer', 'null' )type this PR adds.0deliberately means "filled out in under a second," so junk values land as meaningful.
Three follow-ups I wouldn't block on:
- Nothing reads the field yet. It's missing from the CSV export (
class-contact-form-plugin.php:3007-3009, which does includebrowser), there's no dashboard consumer, andFormResponseinsrc/types/index.tswasn't extended. Until something surfaces it, every failure mode above is invisible. Worth confirming the UI work is tracked.docs/feedback-class.mdis missing the new getter too. - Drag-and-drop file upload never starts the timer. The dropzone runs in its own
jetpack/field-filenamespace and a drop fires nofocusin. Calling it out because file upload is in your testing instructions. - The POST key isn't namespaced. A site owner can set a field's Advanced name/ID to
form_fill_durationand quietly win the collision.
The suite passes on the branch (910 tests, 3120 assertions), but all three new tests build $post_data by hand. Delete the hidden input at class-contact-form.php:1615 and every one still goes green. There's also no JS coverage of the timer, though Jest is configured for the package and tests/js/modules/field-phone/view.test.js shows the pattern for testing this kind of store logic. The non-AJAX write-path fix is why the last commit exists, and it's only been verified by hand on a JN site.
| const context = getContext(); | ||
| // Store the first interaction time when user focuses on any form field | ||
| if ( ! context.formFirstInteractionTime ) { | ||
| context.formFirstInteractionTime = Date.now(); |
There was a problem hiding this comment.
Before merge. Set once, never cleared, so it survives a reset.
goBack (line 754) calls form.reset(), which fires reset into onFormReset (line 611). That clears fields, showErrors, and currentStep, but leaves formFirstInteractionTime alone. form.reset() also restores the hidden input to value='', so the next submit re-enters the write block at line 670 and stores Date.now() - <first focus of the first fill>.
Repro: submit with AJAX on, click "Back" on the success panel, fill it in again, submit. The second entry's duration swallows the first fill, the time spent reading the success message, and the "Back" click.
The reload-after-success path starts even earlier. scrollToWrapper calls successWrapper?.focus() while submissionSuccess is true, which trips focusin and starts the clock before anyone touches the form.
One line in onFormReset covers all of it, since goBack already routes through the native reset event:
context.formFirstInteractionTime = null;There was a problem hiding this comment.
Confirmed the repro. onFormReset was clearing fields, showErrors and currentStep but not the timer, and goBack routes through data-wp-on--reset="actions.onFormReset", so it was reachable exactly as described.
Verified live on a JN site — fill for 9s, submit, linger 5s on the success panel, go back, refill for 3s:
| Entry | Fill | Recorded |
|---|---|---|
| First | 9s | 9 |
| Second | 3s | 3 |
One thing the one-liner didn't cover, which showed up while testing it: form.reset() left the hidden input holding the previous submission's value (I read 10 off it after going back). The write on submit is conditional, so a submit with no interaction recorded would have resent that stale number as if it were a fresh measurement. onFormReset now clears the input as well as the timer, and the same scenario re-run reads "" before the second submit.
| return null; | ||
| } | ||
|
|
||
| return absint( $post_data['form_fill_duration'] ); |
There was a problem hiding this comment.
Before merge. absint() can't validate this. It's abs( (int) $value ) against a hidden input any visitor controls, and the === '' guard above only catches the empty string. What actually gets stored, running each value through on PHP 8.5:
| POST value | stored |
|---|---|
abc |
0 |
-1 |
1 |
' ' |
0 |
form_fill_duration[]=1 |
1 |
1e10 |
10000000000 |
-9223372036854775808 |
float(9.22e18) |
Two separate problems. The design treats 0 as a genuine sub-second fill rather than unknown, so malformed values land as meaningful. And the last row JSON-encodes as 9.223372036854776e+18, which contradicts the array( 'integer', 'null' ) type this PR adds to the endpoint.
The negative case doesn't need an attacker either. Date.now() is wall clock, so a clock step backward mid-fill produces a negative duration in view.js that the server then flips positive.
Validate first, then cast:
$raw = $post_data['form_fill_duration'];
if ( ! is_scalar( $raw ) || ! ctype_digit( (string) $raw ) ) {
return null;
}
return min( (int) $raw, DAY_IN_SECONDS );is_scalar has to come first so an array-shaped POST doesn't fatal on the cast. The clamp is optional, but it keeps an abandoned tab from skewing aggregates.
There was a problem hiding this comment.
Fixed in bdad437. I reproduced your table locally before changing anything — every row behaves as you described, including -9223372036854775808 coming back as a float, which is the one that would have broken the advertised type.
Took the validate-then-cast approach:
$raw = $post_data[ self::FORM_FILL_DURATION_FIELD ];
// is_scalar() has to come first so an array-shaped POST does not blow up on the cast.
if ( ! is_scalar( $raw ) || ! ctype_digit( (string) $raw ) ) {
return null;
}
// Clamp so an abandoned tab left open for days cannot skew aggregates.
return min( (int) $raw, DAY_IN_SECONDS );Kept the clamp — the abandoned-tab case seemed worth guarding given the whole point is aggregates.
The data provider now covers non-numeric, whitespace, negative, scientific notation, decimal and array-shaped input, with separate tests for the overflow and the clamp. A genuine 0 is still preserved as 0, since that is a known duration rather than an unknown one.
| registerField( fieldId, fieldType, fieldLabel, fieldValue, fieldIsRequired, fieldExtra ); | ||
| }, | ||
|
|
||
| trackFirstInteraction() { |
There was a problem hiding this comment.
Optional. focusin catches essentially everything focusable, with one gap: drag-and-drop file upload. fileDropped (src/modules/file-field/view.js:305) runs in the separate jetpack/field-file namespace via data-wp-on--drop, and a drop fires no focus event. Drag a file in, wait, submit, and you get null or a near-zero duration once the submit button takes focus.
File upload is in the testing instructions, so probably worth setting the interaction time in fileDropped the same way.
Lower stakes, separate point: this is the only data-wp-on--* binding for a real DOM event in the package pointing at callbacks.*. Elsewhere callbacks.* is reserved for data-wp-init and data-wp-watch effects. It resolves fine, it just reads oddly next to its neighbors.
There was a problem hiding this comment.
Both taken, in bdad437.
trackFirstInteraction moved from callbacks to actions, and the directive is now data-wp-on--focusin="actions.trackFirstInteraction", which puts it in line with its neighbours. That also gave jetpack/field-file a natural way to call it, so fileDropped now starts the timer directly — same jetpackFormStore.actions.* route the file field already uses for updateFieldValue.
Partly verified. I confirmed on JN that the drop path executes and my call raises nothing — dropping a file renders the preview with no console or page errors. I could not complete the end-to-end check, because file uploads fail on that site (File upload failed, try again.), so the form never gets past validation to write a duration. Worth a look on an environment with working uploads.
For what it is worth, the failure mode is safe: if the cross-namespace getContext() were to resolve the field's context rather than the form's, the write lands somewhere harmless and the behaviour is what it is today — the clock starting at the submit button — rather than anything breaking.
| // Left empty on purpose: the view script fills this in on submit. An empty | ||
| // value is stored as null so "never interacted with" stays distinguishable | ||
| // from "filled out in under a second". | ||
| $r .= "<input type='hidden' name='form_fill_duration' value='' />\n"; |
There was a problem hiding this comment.
Optional. This name shares a flat POST namespace with author-defined fields.
reservedAttributes in src/blocks/shared/components/jetpack-field-id-control.jsx:7-18 is ten HTML attribute names, so nothing stops a site owner from setting a field's Advanced name/ID to form_fill_duration. Manual IDs are kept verbatim, their field renders after this one, PHP's last-wins hands their value to the metric, and it fails silently.
Unlikely, and only the site owner can trip it. But jetpack_form_fill_duration sidesteps it and matches the jetpack_contact_form_jwt convention on the line above. Would need matching updates at view.js:673 and tests/php/contact-form/class-utility.php:178.
There was a problem hiding this comment.
Renamed in bdad437. The field is now jetpack_form_fill_duration, matching jetpack_contact_form_jwt on the line above.
Put the name in Feedback::FORM_FILL_DURATION_FIELD and referenced it from the renderer, the parser and the test utility, so the PHP side can't drift. view.js has a matching constant with a comment pointing back at the PHP one, since that pair is the part a rename could still silently break.
The REST property and the serialized storage key stay form_fill_duration — only the POST field name shares a namespace with author-defined fields, so only that one needed prefixing.
Confirmed on JN that the rendered markup carries the new name.
| /** | ||
| * Test that form_fill_duration is included in serialized response and persists after save/load. | ||
| */ | ||
| public function test_form_fill_duration_persists_after_save() { |
There was a problem hiding this comment.
Optional. These pass, and null-vs-zero is the right thing to pin down.
The gap is that all three build $post_data by hand, so nothing covers the path from rendered markup to stored value. Delete the hidden input at class-contact-form.php:1615 and all three stay green. Contact_Form_Test.php already has the markup-assertion pattern for this.
Two smaller ones, neither blocking:
load_from_post()(class-feedback.php:442) is the "entries predating this feature" path the null design exists for, and nothing exercises it.Utility::create_legacy_feedback()is already there for it.Contact_Form_Endpoint_Test::test_item_schema()assertsassertArrayHasKeyfor every sibling property but wasn't extended for this one.
There was a problem hiding this comment.
All three added in bdad437.
- Markup coverage.
Contact_Form_Test::test_rendered_form_carries_form_fill_duration_markuprenders the form viado_shortcodeand asserts the hidden input, its empty default, and the focusin binding. I checked it actually bites by deleting the input line and re-running — it fails, which was the point. - Legacy entries.
test_form_fill_duration_is_null_for_legacy_entriesgoes throughUtility::create_legacy_feedback()and asserts null, so theload_from_post()path the whole null design exists for is now exercised. - Schema.
test_item_schema()now assertsform_fill_durationis present, and that its type allows bothintegerandnull.
The package suite is at 920 tests. The one red is Form_Webhooks_Test::test_send_webhooks_blocks_localhost_hostname, which fails on clean trunk locally too — unrelated to this branch.
02be43b to
5c10311
Compare
This PR add a form fill duration to each of the form responses.
We start the timer when the user interacts with the for for the first time.
This is done so that we can track ow long the form take to fill out.
Proposed changes:
Other information:
Jetpack product discussion
See p1762390969490259-slack-C086RGTJT1D
Does this pull request change what data or activity we track or use?
Yes, we track how long it takes the user to submit the form.
Testing instructions:
Create a form.
Fill it out. Test this across browser and different devices. Also test out form fields such as file upload.
Notice that we have the data recorded as expected.