Skip to content

fix(editor): Close Find popup when Replace is triggered (Fixes #3428) #3431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

LalitNarayanYadav
Copy link

Description

This PR resolves issue #3428 where the Replace popup would fail to open when the Find popup was active. The fix ensures smooth switching between Find and Replace dialogs.

Changes

  • Modified showReplace() to close active Find dialog before opening Replace
  • Updated keyboard shortcut handlers in extraKeys configuration
  • Ensured consistent behavior for both UI buttons and keyboard shortcuts

Testing

Verified Scenarios

✅ Find (Ctrl+F) → Replace (Ctrl+H)
✅ Click Find → Click Replace
✅ Mixed usage (shortcuts + UI buttons)
✅ Persistent search highlights during switches
✅ No regression in core search functionality

Checklist

…rtcut

Close active Find dialog before opening Replace popup for both UI and keyboard triggers.
Copy link

welcome bot commented Apr 5, 2025

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already.

@LalitNarayanYadav
Copy link
Author

Hi @therewasaguy @limzykenneth, could you please review this fix for #3428 when you have time?
This resolves the Find/Replace popup conflict. Thank you!

@raclim raclim added Area:Editor For CodeMirror-related features Status: On Hold labels Apr 10, 2025
@raclim
Copy link
Collaborator

raclim commented Apr 10, 2025

Thanks for taking a look into this!

We're currently in the process of updating CodeMirror to v6, which does a major refactor of the Editor component. Since the changes to it are quite large, I think it might be best to hold on this issue until that work has been complete!

@Jatin24062005
Copy link
Contributor

Jatin24062005 commented May 3, 2025

Hey @LalitNarayanYadav,
I reproduced your pull request to verify the fixes, but unfortunately, it doesn't resolve the issue and is now also causing the following error
image
.

This might be due to using an older version of CodeMirror — perhaps the closeSearch method is only available in a newer version, as you mentioned. However, I kindly request you to test your pull request thoroughly before submitting, as it helps ensure the fix works as expected and avoids introducing new issues.

As @raclim mentioned, the p5-web editor team is currently working on migrating to CodeMirror v6, so aligning your implementation with that direction could be more helpful moving forward.

Thanks!

@LalitNarayanYadav
Copy link
Author

Hi @Jatin24062005, @raclim, and team,

Thank you for reproducing the PR and pointing out the issue. I appreciate your feedback and understand that the current implementation isn't working as intended due to the closeSearch() method likely being unavailable in the older CodeMirror version used right now.

Given that the team is migrating to CodeMirror v6, I’ll hold off on this patch for now and instead look into aligning the fix with the updated version once the migration is complete. I’ll also re-test thoroughly in the updated environment to ensure better compatibility and stability.

Please feel free to close this PR if that’s cleaner for your workflow, and I’ll revisit this once CodeMirror v6 is fully integrated.

Thanks again!

— Lalit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area:Editor For CodeMirror-related features Status: On Hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants