gw-rich-text-html-fields.php: Added improvements for Rich Text HTML Fields Snippet.
#1132
Conversation
… Fields Snippet.
WalkthroughAdds a persistent gwRichTextMode, editor readiness waiting, mode-aware initialization and switching for TinyMCE/HTML editors, content synchronization via a new window.SetFieldContentProperty API, and tweaks TinyMCE config (forced_root_block:false, height:250). Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Field Settings UI
participant GFJS as gw-rich-text JS
participant TinyMCE as TinyMCE Editor
participant GFAPI as Gravity Forms Field API
UI->>GFJS: load field (includes field.gwRichTextMode)
GFJS->>GFJS: set gwRichTextMode (default 'tmce')
GFJS->>TinyMCE: waitForEditorToBeReady()
alt Editor becomes ready
GFJS->>TinyMCE: initialize or switch editor mode (tmce/html)
TinyMCE-->>GFJS: ready/initialized
end
TinyMCE->>GFJS: on change/paste events
GFJS->>GFJS: call SetFieldContentProperty()
GFJS->>GFAPI: SetFieldProperty(content)
UI->>GFJS: user toggles mode
GFJS->>GFJS: persist gwRichTextMode via SetFieldProperty
GFJS->>TinyMCE: remove or re-init as needed (wp.editor.remove / init)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
gravity-forms/gw-rich-text-html-fields.php (3)
63-66: Consider using a more scoped approach to avoid global variable pollution.The global variable
gwRichTextModecould potentially conflict with other scripts. Consider using a namespace or scoping it within an IIFE to avoid global pollution.- var gwRichTextMode; + var GWRichText = GWRichText || {}; jQuery( document ).on( 'gform_load_field_settings', function( event, field ) { - gwRichTextMode = field.gwRichTextMode || 'tmce'; + GWRichText.mode = field.gwRichTextMode || 'tmce';
115-130: Improve timeout handling in the polling mechanism.The polling mechanism is well-implemented, but the timeout case doesn't provide any fallback behavior or error handling. Consider adding a callback or logging when the timeout occurs.
const waitForEditorToBeReady = (callback, timeout = 5000) => { const start = Date.now(); const interval = setInterval(() => { const editor = typeof tinymce !== 'undefined' && tinymce.get(editorId); if (editor) { clearInterval(interval); callback(); } else if (Date.now() - start > timeout) { clearInterval(interval); + console.warn('TinyMCE editor initialization timed out'); } }, 100); };
131-148: Add error handling and consider scoping the global function.The function logic is correct, but it lacks error handling for the TinyMCE editor instance and uses global scope.
// Set the content when save. - window.SetFieldContentProperty = function () { + window.SetFieldContentProperty = function () { var mode = jQuery('#wp-' + editorId + '-wrap').hasClass('html-active') ? 'html' : 'tmce'; var content = ''; if (mode === 'html') { content = jQuery('#' + editorId).val(); - } else if (tinymce.get(editorId)) { + } else { + var editor = tinymce.get(editorId); + if (editor) { - content = tinymce.get(editorId).getContent(); + content = editor.getContent(); + } } SetFieldProperty('content', content); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gravity-forms/gw-rich-text-html-fields.php(3 hunks)
🔇 Additional comments (2)
gravity-forms/gw-rich-text-html-fields.php (2)
150-158: Good improvement for mode state persistence.The addition of saving the current mode to the field property ensures state persistence across form editor sessions. The implementation correctly identifies the clicked button and updates the field property accordingly.
129-129: Ensure consistent renaming ofgwRichTextModewhen adding a namespaceIf you introduce a namespaced global (e.g.
window.MyPlugin.richTextMode), be sure to update every reference ingravity-forms/gw-rich-text-html-fields.php:
- Declaration:
var gwRichTextMode;- Initialization from field settings:
gwRichTextMode = field.gwRichTextMode || 'tmce';- Mode switch call:
waitForEditorToBeReady(() => window.switchEditors.go(editorId, gwRichTextMode === 'html' ? 'html' : 'tmce' ) );- Saving back to the field property:
SetFieldProperty('gwRichTextMode', mode)Verify that each instance is updated to use your chosen namespace.
|
@saifsultanc A few issues:
|
… Fields Snippet.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gravity-forms/gw-rich-text-html-fields.php(1 hunks)
🔇 Additional comments (2)
gravity-forms/gw-rich-text-html-fields.php (2)
66-73: LGTM on editor cleanup logic.The cleanup sequence correctly removes the TinyMCE instance first (if it exists), then calls
wp.editor.remove()for complete cleanup before setting the new content value.
78-85: Verifyforced_root_block: falseis sufficient to prevent tag wrapping.The PR comments report "code is being wrapped in tags automatically on save" despite
forced_root_block: false. This setting should prevent<p>wrapping, but TinyMCE may still apply formatting through other mechanisms.Consider adding these additional settings to prevent unwanted formatting:
wp.editor.initialize( id, { tinymce: { forced_root_block: false, + force_br_newlines: true, + remove_linebreaks: false, + convert_newlines_to_brs: false, setup: function( editor ) {Additionally, verify if the tag wrapping occurs on initial load, during editing, or specifically when saving the field settings. The Loom video in the PR comments may provide more context.
| setTimeout( function() { | ||
| wp.editor.initialize( id, { | ||
| tinymce: { | ||
| forced_root_block: false, | ||
| setup: function( editor ) { | ||
| editor.on( 'Paste Change input Undo Redo', function () { | ||
| SetFieldProperty( 'content', editor.getContent() ); | ||
| } ); | ||
| } | ||
| }, | ||
| quicktags: true | ||
| } ); | ||
| }, 100 ); |
There was a problem hiding this comment.
Missing editor_height likely causes height accumulation on each save.
The original editor initialization (line 38) specifies editor_height: 250, but this reinitialization omits it. This matches the reported issue where "code editor height increases each time the field settings are saved."
setTimeout( function() {
wp.editor.initialize( id, {
tinymce: {
forced_root_block: false,
+ height: 250,
setup: function( editor ) {
editor.on( 'Paste Change input Undo Redo', function () {
SetFieldProperty( 'content', editor.getContent() );
} );
}
},
quicktags: true
} );
}, 100 );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setTimeout( function() { | |
| wp.editor.initialize( id, { | |
| tinymce: { | |
| forced_root_block: false, | |
| setup: function( editor ) { | |
| editor.on( 'Paste Change input Undo Redo', function () { | |
| SetFieldProperty( 'content', editor.getContent() ); | |
| } ); | |
| } | |
| }, | |
| quicktags: true | |
| } ); | |
| }, 100 ); | |
| setTimeout( function() { | |
| wp.editor.initialize( id, { | |
| tinymce: { | |
| forced_root_block: false, | |
| height: 250, | |
| setup: function( editor ) { | |
| editor.on( 'Paste Change input Undo Redo', function () { | |
| SetFieldProperty( 'content', editor.getContent() ); | |
| } ); | |
| } | |
| }, | |
| quicktags: true | |
| } ); | |
| }, 100 ); |
🤖 Prompt for AI Agents
In gravity-forms/gw-rich-text-html-fields.php around lines 76 to 88, the
wp.editor.initialize call reinitializes the editor without the editor_height
option which causes the editor height to grow on each save; add editor_height:
250 (matching the original initialization) into the options object passed to
wp.editor.initialize so the height is fixed, e.g. include "editor_height: 250"
alongside tinymce and quicktags, ensuring the value is numeric and consistent
with the original initialization.
|
@saifsultanc Seeing some wonkiness here that's preventing me from testing the original issues: https://www.loom.com/share/2a344afec9604cfa90548f1db527da09 |
… Fields Snippet.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gravity-forms/gw-rich-text-html-fields.php (1)
70-81:heightshould beeditor_heightat the top level of the options object.The
height: 250is placed inside thetinymceconfig, butwp.editor.initialize()expectseditor_heightas a top-level option (sibling totinymceandquicktags). This likely explains why the reported height accumulation issue persists.wp.editor.initialize( id, { tinymce: { forced_root_block: false, - height: 250, setup: function( editor ) { editor.on( 'Paste Change input Undo Redo', function () { SetFieldProperty( 'content', editor.getContent() ); } ); } }, - quicktags: true + quicktags: true, + editor_height: 250 } );
🧹 Nitpick comments (1)
gravity-forms/gw-rich-text-html-fields.php (1)
117-131: Timeout silently fails without fallback.If the editor doesn't initialize within 5 seconds, the callback never executes and the mode switch silently fails. Consider logging a warning or falling back to a default behavior.
const waitForEditorToBeReady = (callback, timeout = 5000) => { const start = Date.now(); const interval = setInterval(() => { const editor = typeof tinymce !== 'undefined' && tinymce.get(editorId); if (editor) { clearInterval(interval); callback(); } else if (Date.now() - start > timeout) { clearInterval(interval); + console.warn('TinyMCE editor initialization timed out for', editorId); } }, 100); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gravity-forms/gw-rich-text-html-fields.php(3 hunks)
🔇 Additional comments (1)
gravity-forms/gw-rich-text-html-fields.php (1)
63-66: Mode persistence initialization looks correct.The global variable properly captures the saved mode preference with a sensible default to visual mode ('tmce').
| // Update the content. | ||
| jQuery(document).on('change', `#${editorId}`, function () { | ||
| window.SetFieldContentProperty(); | ||
| }); |
There was a problem hiding this comment.
Use input event instead of change for real-time sync; prevent duplicate handlers.
The change event only fires when the textarea loses focus, so keystrokes won't sync until the user clicks away. This could explain the reported line break issues. Additionally, this delegated handler isn't cleaned up on editor reinitialization, leading to duplicate handlers.
+ // Namespace the event for proper cleanup
+ jQuery(document).off('input.gwRichText', `#${editorId}`);
- jQuery(document).on('change', `#${editorId}`, function () {
+ jQuery(document).on('input.gwRichText', `#${editorId}`, function () {
window.SetFieldContentProperty();
});🤖 Prompt for AI Agents
In gravity-forms/gw-rich-text-html-fields.php around lines 147 to 150, the
delegated handler uses the 'change' event (which only fires on blur) and can be
bound repeatedly on reinitialization; switch to the 'input' event for real-time
keystroke syncing and prevent duplicate handlers by removing any existing
handler before binding (use a namespaced event or call off(...) with the same
selector/event signature prior to on(...)). Ensure the new binding targets the
correct element and that cleanup happens on editor teardown or before
reinitialization to avoid multiple listeners.
| // Switch to visual/text mode. | ||
| jQuery(`#wp-${editorId}-wrap .switch-tmce, #wp-${editorId}-wrap .switch-html`).on('click', function() { | ||
| var mode = jQuery(this).hasClass('switch-tmce') ? 'tmce' : 'html'; | ||
|
|
||
| window.switchEditors.go(editorId, mode); | ||
|
|
||
| // Save the current mode to field property. | ||
| SetFieldProperty('gwRichTextMode', mode) | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Sync content before switching modes to prevent data loss.
When switching from HTML to visual mode (or vice versa), the current content should be saved first. Otherwise, unsaved changes in the active mode could be lost during the switch.
jQuery(`#wp-${editorId}-wrap .switch-tmce, #wp-${editorId}-wrap .switch-html`).on('click', function() {
var mode = jQuery(this).hasClass('switch-tmce') ? 'tmce' : 'html';
+ // Sync content before switching to preserve unsaved changes
+ window.SetFieldContentProperty();
+
window.switchEditors.go(editorId, mode);
// Save the current mode to field property.
- SetFieldProperty('gwRichTextMode', mode)
+ SetFieldProperty('gwRichTextMode', mode);
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In gravity-forms/gw-rich-text-html-fields.php around lines 152-160, the click
handler switches editor modes without syncing the current editor content,
risking data loss; before calling window.switchEditors.go(editorId, mode) detect
the active mode and capture the current content (from tinyMCE when visual/tmce
is active, or from the textarea when HTML is active), write that content back
into the underlying textarea and/or the field value used by Gravity Forms, then
persist any mode property (SetFieldProperty('gwRichTextMode', mode)) and only
after syncing call switchEditors.go so the editor switch preserves unsaved
changes.
… Fields Snippet. Disable HTML click on load and re-enable after page load.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
gravity-forms/gw-rich-text-html-fields.php (2)
161-164: Still usingchangeevent instead ofinput; handlers may accumulate.This was flagged in a previous review: the
changeevent only fires on blur, not during typing, which may cause the reported line break sync issues. Additionally, this delegated handler isn't cleaned up between field loads, leading to duplicate handlers.+ // Clean up previous handlers + jQuery(document).off('input.gwRichText', `#${editorId}`); - jQuery(document).on('change', `#${editorId}`, function () { + jQuery(document).on('input.gwRichText', `#${editorId}`, function () { window.SetFieldContentProperty(); });
166-174: Content not synced before mode switch; handlers may accumulate.As flagged in a previous review: switching modes without first saving content can lose unsaved changes. Also, this click handler binds on every
tinymce-editor-setupevent without cleanup.+ // Clean up previous handlers + jQuery(`#wp-${editorId}-wrap .switch-tmce, #wp-${editorId}-wrap .switch-html`).off('click.gwRichText'); - jQuery(`#wp-${editorId}-wrap .switch-tmce, #wp-${editorId}-wrap .switch-html`).on('click', function() { + jQuery(`#wp-${editorId}-wrap .switch-tmce, #wp-${editorId}-wrap .switch-html`).on('click.gwRichText', function() { var mode = jQuery(this).hasClass('switch-tmce') ? 'tmce' : 'html'; + // Sync content before switching to preserve unsaved changes + window.SetFieldContentProperty(); + window.switchEditors.go(editorId, mode); // Save the current mode to field property. - SetFieldProperty('gwRichTextMode', mode) + SetFieldProperty('gwRichTextMode', mode); });
🧹 Nitpick comments (3)
gravity-forms/gw-rich-text-html-fields.php (3)
63-75: Magic timeout and inline style manipulation are fragile.The 800ms delay is arbitrary and may be insufficient on slower connections or excessive on fast ones. Consider toggling a CSS class instead of manipulating inline styles, and tying the re-enable to actual editor readiness rather than a fixed timeout.
+ <style> + .gf-html-container.gw-editor-loading { + pointer-events: none; + opacity: 0.6; + } + </style> + <script> - // Immediately disable HTML click. - jQuery( '.gf-html-container' ) - .css( 'pointer-events', 'none' ) - .css( 'opacity', '0.6' ); - - // Re-enable only after full load of page. - jQuery( window ).on( 'load', function() { - setTimeout( function() { - jQuery( '.gf-html-container' ) - .css( 'pointer-events', 'auto' ) - .css( 'opacity', '1' ); - }, 800 ); - }); + // Disable interaction until editor is ready. + jQuery( '.gf-html-container' ).addClass( 'gw-editor-loading' );Then remove the class when the editor is actually ready (inside
waitForEditorToBeReadycallback) rather than using a fixed timeout.
77-79: Global variablegwRichTextModerisks conflicts.Consider namespacing this variable (e.g.,
window.gwRichText = { mode: ... }) to avoid potential conflicts with other scripts in the Gravity Forms admin environment.
131-145: Silent timeout failure may mask initialization issues.The 5-second timeout silently aborts without logging or fallback behavior. Consider adding a console warning on timeout to aid debugging if the editor fails to initialize.
} else if (Date.now() - start > timeout) { clearInterval(interval); + console.warn('GW Rich Text: Editor initialization timed out for', editorId); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gravity-forms/gw-rich-text-html-fields.php(3 hunks)
🔇 Additional comments (1)
gravity-forms/gw-rich-text-html-fields.php (1)
84-95: LGTM!The
height: 250addition addresses the previously reported height accumulation issue, andforced_root_block: falsecorrectly prevents automatic<p>tag wrapping.
| // Set the content when save. | ||
| window.SetFieldContentProperty = function () { | ||
| var mode = jQuery('#wp-' + editorId + '-wrap').hasClass('html-active') ? 'html' : 'tmce'; | ||
| var content = ''; | ||
|
|
||
| if (mode === 'html') { | ||
| content = jQuery('#' + editorId).val(); | ||
| } else if (tinymce.get(editorId)) { | ||
| content = tinymce.get(editorId).getContent(); | ||
| } | ||
|
|
||
| SetFieldProperty('content', content); | ||
| }; |
There was a problem hiding this comment.
Potential content loss if TinyMCE not ready in visual mode.
When mode === 'tmce' but tinymce.get(editorId) returns falsy, content remains an empty string and SetFieldProperty is still called, potentially wiping existing content. Consider falling back to the textarea value:
if (mode === 'html') {
content = jQuery('#' + editorId).val();
} else if (tinymce.get(editorId)) {
content = tinymce.get(editorId).getContent();
+ } else {
+ // Fallback to textarea if TinyMCE not ready
+ content = jQuery('#' + editorId).val();
}🤖 Prompt for AI Agents
In gravity-forms/gw-rich-text-html-fields.php around lines 147 to 159, the
SetFieldContentProperty function can set content to an empty string when mode
=== 'tmce' but tinymce.get(editorId) is falsy, risking data loss; change the
logic so that when tinymce is not available you fall back to the textarea value
(jQuery('#' + editorId).val()) instead of leaving content empty, then call
SetFieldProperty('content', content) with that fallback value.
Context
⛑️ Ticket(s): https://secure.helpscout.net/conversation/2987432785/86002
Summary
Primarily inspired from #1071, with some improvements. Takes into account the recent updates to the snippet here 55c2438
https://www.loom.com/share/fbb020ebea884e76a1ed2fe0eae1e9fb