Try to render captcha a second time on a timeout#2058
Conversation
WalkthroughThe changes in this pull request focus on enhancing the captcha rendering logic within the Changes
Possibly related PRs
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js/formidable.js (1)
1864-1878: Approve changes with a minor suggestion.The changes to the
frmCaptchafunction look good. They address the issue of captcha not rendering correctly when the form is initially hidden. The added timeout mechanism is a smart solution to handle forms that fade into view.A minor suggestion for improvement:
Consider extracting the timeout logic into a separate function for better readability and potential reuse. For example:
function renderCaptchaWithDelay(captcha, captchaSelector, closestForm) { setTimeout( function() { if (closestForm && closestForm.offsetParent !== null) { frmFrontForm.renderCaptcha(captcha, captchaSelector); } }, 1000 ); }Then you can call this function instead of having the setTimeout directly in the loop.
|
Zip file from that change. @Crabcyborg just to confirm, we are sure that one second is enough time, correct? |
|
@engahmeds3ed I'm not totally confident it's perfect, no. It might be nice to actually use an interval so we can solve the modal issue as well. |
|
@engahmeds3ed I just updated this to try an interval instead. I shortened it to 400ms now as well so it should be more instant when the visibility change happens. It should solve the modal issue as well. Could you help test it out? Thank you! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js/formidable.js (1)
1864-1880: Improved captcha rendering logic for hidden formsThe changes to the
frmCaptchafunction introduce a more robust handling of captcha rendering, especially for forms that may not be immediately visible. This is a good improvement that addresses potential issues with forms in modals or those that fade in on page load.Key improvements:
- Checks if the closest form is visible before rendering the captcha.
- If the form is not visible, it sets up an interval to check visibility every 400ms.
- Once the form becomes visible, it renders the captcha and clears the interval.
These changes should help prevent issues where captchas fail to render properly in dynamically displayed forms.
Consider adding a maximum number of retry attempts or a timeout for the interval to prevent potential infinite loops in edge cases where the form never becomes visible.
|
I tested this change for that case and it works perfectly. Let me validate with the modal issue. |
|
It works perfectly also with the modal issue, u r awesome @Crabcyborg The only risk here is that the interval will keep working forever till the modal trigger button is clicked, once it's clicked and the form appears, we will clear that interval. |
|
Thanks @engahmeds3ed! Yeah, I think it should be fine. It only happens if it's hidden as well and helps to fix the whole class of new tickets that have been coming up because of this visibility check. |
Related Slack conversation https://strategy11.slack.com/archives/C799A2R61/p1729107464507059
Now if the form immediately becomes visible (within the next second), the captcha will still render.