Skip to content

Keep the Gravity Forms recorder working after an AJAX page change - #2217

Merged
subodhr258 merged 4 commits into
developfrom
fix/gf-recorder-ajax-page-change
Oct 20, 2026
Merged

subodhr258 merged 4 commits into
developfrom
fix/gf-recorder-ajax-page-change

Conversation

@subodhr258

@subodhr258 subodhr258 commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #2222

Review packet

In one line: On a Gravity Forms form with AJAX turned on, the GoDAM Record field now keeps working after the visitor changes page or a submit fails, so Start Recording opens the recorder again. Fixes #2222. Related to #2159 (same form flow, found while retesting it).

1. Criteria and proof

Acceptance criterion Proof
On a 5-page AJAX form, after page 1 then Next, Start Recording on each of the 3 Record fields on page 2 opens the recorder and a clip shows its preview (before: it opened nothing in every run) After video V2 at 0:06 to 0:20; before video V1 at 0:04
Next, then Previous: page 2 shows all 3 recordings again and Start Recording still opens V2 at 0:25
Removing one recording and recording it again changes only that field V2 at 0:30; the stored entry holds the new clip for that field and the first clips for the other two
A failed submit on the page that holds the recorders keeps the 3 recordings, Start Recording opens, replacing one clip works, and the form then submits Scenario ajax validate, 10 checks, on Gravity Forms 3.1.2 and on 2.7.7.1 (the older iframe AJAX). Log only, not in the video
Every entry holds exactly one file per Record field, nothing sent twice, no "Number of files (2) exceeds limit (1)" Entries read back after every run: 6 entries on 3.1.2 and 2 on 2.7.7.1, 1 file in each of the 3 fields
The non-AJAX twin of the same form still works as in 2.3.1 Right pane of V2, plus scenarios postback plain and postback validate, all checks pass
The recorders of WPForms, Fluent Forms, Everest Forms and SureForms are unaffected Each page: opens, records 2 s, preview shows, file input holds 1 file, Remove clears both, Start Recording opens again, no JS error

2. Before and after

Left pane: a form with AJAX on (the setup that failed). Right pane: the same form with AJAX off, as a control. Both panes get the same visitor actions. Each step has a check, so a red step is a real failure.

Before (develop, 2.4.0): on the AJAX form, Start Recording opens nothing after Next; the control opens and records

V1 before: Start Recording opens nothing on the AJAX form

After (this branch): the full flow on both forms, 8 steps, all checked

V2 after: record 3 clips, Next, Previous, replace one, submit

3. Tests

  • Added: assets/src/js/godam-recorder/index.test.js, 11 tests. They replace a page's markup the way Gravity Forms does, fire gform_post_render, and assert: the new button opens the dashboard exactly once (also with a different button id, with the event fired three times, and across several page changes); the held recording is shown again; it is not sent again when Gravity Forms holds a copy and is put on the new file input when it does not; removing it after the page change drops Gravity Forms' saved copy; nothing happens while the markup is still on the page; a field that is not on the page yet is picked up when it appears. The Uppy stand-in binds its dashboard trigger once at install, as the real one does.
  • Changed: none.
  • Revert check: with develop's index.js put back, 8 of the 11 fail, for example "opens the dashboard from the new button after a page change" with Expected number of calls: 1, Received number of calls: 0. With the fix all 11 pass.

4. Risk flags

No risk flags fired on #2217. Nothing in the diff touched the watched areas.

5. Independent review

A fresh reviewer saw only the task and the diff. It found nothing it could confirm. Two low-confidence points:

Finding Grade What I did
Gravity Forms prints its hidden gform_uploaded_files input only for multi-file upload fields, so on a form whose only upload fields are Record fields an AJAX page change might throw inside Gravity Forms before gform_post_render fires Invalid Tested exactly that: a 5-page AJAX form on Gravity Forms 3.1.2 with only Record fields, recorded on page 2, Next to page 3. No JS error; the saved copy was listed and the next page showed. Nothing changed
Two embeds of the same form on one page share container ids, so a detached uploader could pick the other embed's container By design The constructor already finds the input by id the same way, so this adds no new limit. Not changed

6. Not verified

  • Ninja Forms: not installed on the test site, so its recorder was not run. It shares the constructor and its markup follows the same container, input and button ids, which this PR reads exactly as before.
  • Gravity Forms versions between 2.7.7.1 and 3.1.2 (for example the first 2.9 releases) were not run. 3.1.2 uses true AJAX and 2.7.7.1 the older iframe AJAX; both pass.
  • A Record field inside a GoDAM video layer on an AJAX form was not run.
  • Only Chrome (headless, fake camera and screen) was used; Firefox and Safari were not.
  • Not tested on the live site of the customer behind Gravity Forms: GoDAM Record field fails with "Number of files (2) exceeds limit (1)" on Gravity Forms 2.9.18+ #2159.

7. Scope

The fix is in assets/src/js/godam-recorder/index.js with its new test. languages/godam.pot changes by three source line references (the recorder's lines moved), regenerated with the repo's own build and make-pot, because the POT check requires it. No PHP, no changelog line, no version bump.

What was wrong

When Gravity Forms changes page or fails a submit over AJAX it replaces that page's markup and fires gform_post_render. The recorder kept pointing at the old container, file input and button, and Uppy had bound its dashboard trigger once, to the old button. The old handler also returned unless the form had a validation error, so on a page change nothing was re-bound and Start Recording did nothing, without any error. The server prints the trigger button's id with wp_unique_id(), so it can differ on every render.

What changed

On every gform_post_render, an uploader whose container left the page finds the new container by its id (stable between renders), reads the file input and button from it, adds one click listener to the new button, and shows the recording it already holds again. Nothing changes while the markup is still on the page (first render, a non-AJAX submit, a page the response did not replace). The non-AJAX path and the other form plugins go through the same lookup as before.

Gravity Forms replaces a page's markup on every AJAX page change or failed
submit. The recorder kept pointing at the replaced container, file input and
button, and Uppy had bound its dashboard trigger once to the old button, so
Start Recording opened nothing on the new page.

On every gform_post_render, an uploader whose markup left the page now finds
the new container by its id, reads the input and button from it, binds the
new button once and shows the recording it holds again.
@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

🔍 WordPress Plugin Check Report

⚠️ Status: Passed with warnings

📊 Report

🎯 Total Issues ❌ Errors ⚠️ Warnings
15 0 15

⚠️ Warnings (15)

📁 inc/classes/class-video-sitemap.php (1 warning)
📍 Line 🔖 Check 💬 Message
620 WordPress.DB.SlowDBQuery.slow_db_query_meta_key Detected usage of meta_key, possible slow query.
📁 readme.txt (2 warnings)
📍 Line 🔖 Check 💬 Message
0 mismatched_plugin_name Plugin name "GoDAM - Organize WordPress Media Library & File Manager with Unlimited Folders for Images, Videos & more" is different from the name declared in plugin header "GoDAM".
0 trademarked_term The plugin name includes a restricted term. Your chosen plugin name - "GoDAM - Organize WordPress Media Library & File Manager with Unlimited Folders for Images, Videos & more" - contains the restricted term "wordpress" which cannot be used at all in your plugin name.
📁 inc/classes/class-video-sitemap-core-provider.php (1 warning)
📍 Line 🔖 Check 💬 Message
73 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "wp_sitemaps_index_entry".
📁 assets/build/css/main.css (1 warning)
📍 Line 🔖 Check 💬 Message
0 EnqueuedStylesScope This style is being loaded in all contexts.
📁 assets/src/libs/analytics.min.js (5 warnings)
📍 Line 🔖 Check 💬 Message
0 EnqueuedScriptsScope This script is being loaded in all frontend contexts.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880 (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/2026/10/20/demo-post-post/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-page-post/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-attachment-post/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
📁 assets/build/js/main.min.js (5 warnings)
📍 Line 🔖 Check 💬 Message
0 EnqueuedScriptsScope This script is being loaded in all frontend contexts.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880 (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/2026/10/20/demo-post-post/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-page-post/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-attachment-post/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.

🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check

@KMchaudhary

Copy link
Copy Markdown
Collaborator

Code review

Verdict: LGTM — solid, well-tested fix with a clean refactor. The root cause (stale container/fileInput/uploadButton references and a one-time Uppy trigger binding after Gravity Forms replaces the page markup) is addressed at the right level: extracting resolveRecorderMarkup() + useMarkup() and re-reading the button id (which the server builds with wp_unique_id(), so it changes every render) from the container each time. Using this.container.isConnected as the "my markup was replaced" signal is a much more robust trigger than the old gform_validation_error / submission-method heuristics, and it correctly no-ops on first render, postbacks, and unrelated forms' renders. The 11 tests with the revert check (8 fail on develop) give good confidence.

A couple of points, one worth a look before merge:

1. Fields that first appear via AJAX get no instance (robustness) — index.js bootstrap

Instances are created only once, at DOMContentLoaded, by iterating .uppy-video-upload containers then present. The fix then moves existing instances onto replaced markup (found by the stable containerId). This works for the multi-page case you tested because Gravity Forms renders all pages' HTML up front (hidden), so page-2 containers already exist at load and get an instance.

The gap: a Record field that is absent at load and introduced later via AJAX — a field revealed by GF conditional logic, or a Record field inside a GoDAM video layer loaded on demand (which your "Not verified" list calls out) — never gets an UppyVideoUploader, so gform_post_render has nothing to re-bind and Start Recording does nothing. A global gform_post_render pass that instantiates any uninstantiated .uppy-video-upload container would close this, and would also subsume the "move onto new markup" case. Not a blocker for the verified scenarios, but worth confirming the video-layer path before relying on it.

2. Two embeds of the same form share a container id (acknowledged, not a regression) — index.js

handleUppyUIOnAjaxRequest() locates the new container with document.getElementById(this.containerId), which returns the first match. If the same form is embedded twice on one page, after an AJAX page change both instances resolve to the first embed and the second embed's recorder breaks. You've flagged this as by-design/pre-existing (the constructor already resolved by id the same way), so just noting it stays a known limitation.

Minor

  • setupGravityFormsAjaxHandler() no longer reads formId, so every uploader instance's handler runs on every form's gform_post_render. It's correctly guarded by isConnected, so this is safe — just O(instances) cheap checks per render event. Fine to leave as-is.

Nice work on the test harness that replays gform_post_render the way GF does, and on documenting the exact "not verified" surface.

🤖 Automated review via Claude Code (/code-review, high effort).

@subodhr258
subodhr258 merged commit fdd53f8 into develop Oct 20, 2026
4 checks passed
@subodhr258
subodhr258 deleted the fix/gf-recorder-ajax-page-change branch October 20, 2026 10:29
@subodhr258

Copy link
Copy Markdown
Collaborator Author

@KMchaudhary on your comment:

Thanks Kuldip.

  1. Record fields that appear later: partly. In the player, a Gravity Forms layer is rendered into the page at load (inc/templates/godam-player.php:919), and Gravity Forms prints conditional-logic fields hidden, so both get an uploader at DOMContentLoaded. The on-demand gform endpoint is only used by the video editor preview (pages/video-editor/redux/api/gravity-forms.js:32). It can still happen when another plugin loads the form later, for example in a popup, so creating an uploader for any new container on gform_post_render goes on the follow-up list.
  2. Same form embedded twice: agreed, a known limitation from before this PR.

Minor: agreed, isConnected keeps it safe, so it's left as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants