Avoid selecting text when multiselecting fields #2211
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (4)
js/formidable_admin.js (4)
Line range hint
1156-1180: Potential memory leak in event handlerThe event handler for field group hover is not properly cleaned up. The mousemove event listener should be removed when no longer needed.
function maybeRemoveHoverTargetOnMouseMove(event) { const elementFromPoint = document.elementFromPoint(event.clientX, event.clientY); if (null !== elementFromPoint && null !== elementFromPoint.closest('#frm-show-fields')) { return; } maybeRemoveGroupHoverTarget(); + jQuery('#wpbody-content').off('mousemove', maybeRemoveHoverTargetOnMouseMove); }
Line range hint
2178-2201: Security vulnerability in HTML sanitizationThe purifyHtml function needs to be more robust to prevent XSS attacks. Consider using a dedicated HTML sanitization library.
function purifyHtml(html) { + // Use DOMPurify or similar library + return DOMPurify.sanitize(html, { + ALLOWED_TAGS: ['b', 'i', 'em', 'strong'], + ALLOWED_ATTR: [] + }); - // Current implementation may allow unsafe HTML - return clean; }
Line range hint
3012-3040: Improve error handling in AJAX requestsThe AJAX error handling in postAjax could be more robust. Add proper error handling and user feedback.
function postAjax(data, success) { const xmlHttp = new XMLHttpRequest(); + xmlHttp.onerror = function() { + // Handle network errors + console.error('Network error occurred'); + showErrorMessage('Network error occurred. Please try again.'); + }; + xmlHttp.ontimeout = function() { + // Handle timeouts + console.error('Request timed out'); + showErrorMessage('Request timed out. Please try again.'); + }; }
Line range hint
3452-3475: Add accessibility improvementsThe modal dialog implementation needs better accessibility support.
function initModal(id, width) { const dialogArgs = { modal: true, + role: 'dialog', + aria: { + labelledby: id + '-title', + describedby: id + '-description' + }, + // Add keyboard navigation support + onKeyDown: function(e) { + if (e.key === 'Escape') { + $info.dialog('close'); + } + } }; }
🧹 Nitpick comments (1)
js/formidable_admin.js (1)
Line range hint
1-50: Code organization needs improvementThe code would benefit from better modularization and separation of concerns. Consider:
- Breaking down the large frmAdminBuildJS function into smaller, focused modules
- Using ES modules for better code organization
- Adding JSDoc documentation for the main function and public methods
// Break into modules like: import { formBuilder } from './modules/form-builder'; import { settings } from './modules/settings'; import { exports } from './modules/exports'; const frmAdminBuild = () => { // Core functionality };
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2211 +/- ##
============================================
+ Coverage 26.94% 26.97% +0.02%
- Complexity 8247 8258 +11
============================================
Files 127 127
Lines 27305 27316 +11
============================================
+ Hits 7357 7368 +11
Misses 19948 19948 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @AbdiTolesa.
It looks like this update is working in Chrome but now the text is getting selected in Firefox and not getting unselected (since the other code was also removed).
Could you test it in FIrefox as well?
|
Actually, I thought this was working, but I'm still seeing the same issue sometimes. 🫤 |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (5)
js/formidable_admin.js (5)
Line range hint
1-100: Add error handling for AJAX requestsThe AJAX requests throughout the code should include error handling to gracefully handle failures. Add catch blocks and error notifications.
Example improvement for one of the AJAX calls:
jQuery.ajax({ type: 'POST', url: ajaxurl, data: data, success: function( response ) { // Success handling + }, + error: function( xhr, status, error ) { + // Show error notification + console.error( 'Request failed:', error ); + infoModal( frm_admin_js.ajax_error ); } });
Line range hint
400-500: Add input validationThe form field validation could be improved by adding more robust input validation before processing.
function validateInput(value, type) { switch(type) { case 'email': return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(value); case 'number': return !isNaN(value) && value !== ''; // Add more validation types } return true; }
Line range hint
800-900: Add debouncing for resource-intensive operationsSome operations like field updates and AJAX calls should be debounced to prevent excessive server requests.
const debouncedFieldUpdate = debounce(function(fieldId, value) { updateField(fieldId, value); }, 250);
Line range hint
1400-1500: Implement proper cleanup in event handlersEvent handlers should be properly cleaned up to prevent memory leaks, especially for dynamically added/removed elements.
function addEventHandlers() { const handlers = new Map(); return { add: function(element, event, handler) { element.addEventListener(event, handler); handlers.set(element, {event, handler}); }, cleanup: function() { handlers.forEach((value, element) => { element.removeEventListener(value.event, value.handler); }); handlers.clear(); } }; }
Line range hint
1600-1700: Add comprehensive error loggingImplement better error logging and monitoring throughout the application.
const logger = { error: function(error, context) { console.error('Error:', error); // Send to error monitoring service if (typeof errorMonitoring !== 'undefined') { errorMonitoring.captureException(error, {extra: context}); } } };
🧹 Nitpick comments (5)
js/formidable_admin.js (5)
10706-10710: Remove unnecessary event handlerThe mousedown event handler that only prevents default when shift key is pressed appears to be redundant and doesn't serve a clear purpose. Consider removing it or documenting its necessity.
- document.querySelector( '.frm_form_builder' ).addEventListener( 'mousedown', event => { - if ( event.shiftKey ) { - event.preventDefault(); - } - });
Line range hint
200-300: Improve event delegationMultiple event handlers are attached directly to elements. Use event delegation for better performance, especially for dynamically added elements.
- jQuery('.frm_form_builder form').on('submit', function() { + jQuery(document).on('submit', '.frm_form_builder form', function() {
Line range hint
600-700: Implement caching for frequently accessed DOM elementsCache frequently accessed DOM elements to improve performance instead of repeatedly querying them.
const cache = { $formBuilder: null, $fieldOptions: null, init: function() { this.$formBuilder = jQuery('#frm_form_builder'); this.$fieldOptions = jQuery('#frm_field_options'); } };
Line range hint
1000-1100: Improve code organization with modulesThe code would benefit from being split into modules using ES6 modules for better organization and maintainability.
// field-manager.js export class FieldManager { constructor() { // Initialize field management } // Field related methods } // settings-manager.js export class SettingsManager { // Settings related methods }
Line range hint
1200-1300: Add TypeScript type definitionsConsider adding TypeScript type definitions to improve code maintainability and catch potential errors during development.
interface FieldOptions { id: number; type: string; label: string; // Add more field properties }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable_admin.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cypress
I tried hooking into |
Fix https://github.com/Strategy11/formidable-pro/issues/5224
This is just an implementation improvement than a feature to test. We have been unselecting selected text after fields are multiselected but we are now preventing text selection at the first place.