[suggestions] Fix multiple onExit calls when using escape to close#7592
Open
oBusk wants to merge 5 commits intoueberdosis:mainfrom
Open
[suggestions] Fix multiple onExit calls when using escape to close#7592oBusk wants to merge 5 commits intoueberdosis:mainfrom
oBusk wants to merge 5 commits intoueberdosis:mainfrom
Conversation
The escape code called onExit directly, then called dispatchExit which in turn called onExit directly, and finally once the exit signal is received in view.update, is called again. This meant that the onExit callback was being called up to 3 times when escape was used to close it. *Affecting change*: Previously `onExit` would receive the `decorationNode` right before it was removed when hitting escape. This was not the case if the suggestion was exited any other way (Moving cursor out from trigger, calling `exitSuggestion()`, etc.), so the current behaviour is more consistent across all exit methods.
The suggestion plugin now correctly handles Escape via exitSuggestion, which triggers the view.update lifecycle and calls onExit once. The demos were previously handling Escape in onKeyDown and returning true, which bypassed the plugin's exit mechanism and left it in a stale active state. Now Escape falls through to the plugin naturally.
🦋 Changeset detectedLatest commit: e52986f The changes in this PR will be included in the next version bump. This PR includes changesets to release 72 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes @tiptap/suggestion so pressing Escape to close an active suggestion triggers renderer.onExit() exactly once (instead of multiple times), and aligns Escape handling with the standard exit path driven by the plugin view lifecycle.
Changes:
- Removed direct/manual
renderer.onExit()calls from the Escape key path and rely on the plugin state transition (exitSuggestion→view.update) to fireonExitonce. - Added an integration test asserting
onExitis called exactly once on Escape. - Updated multiple demos to stop manually handling Escape (so they don’t bypass the plugin exit logic).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/suggestion/src/suggestion.ts | Removes the extra Escape-path onExit calls and routes Escape through exitSuggestion so onExit is emitted via the plugin view lifecycle. |
| packages/suggestion/src/tests/suggestion.test.ts | Adds a regression test ensuring Escape triggers onExit exactly once. |
| demos/src/Nodes/Mention/Vue/suggestion.js | Removes demo-level Escape handling and delegates to the suggestion plugin lifecycle. |
| demos/src/Nodes/Mention/React/suggestion.js | Removes demo-level Escape handling and delegates to the suggestion plugin lifecycle. |
| demos/src/Nodes/Emoji/Vue/suggestion.js | Removes demo-level Escape handling and delegates to the suggestion plugin lifecycle. |
| demos/src/Nodes/Emoji/React/suggestion.js | Removes demo-level Escape handling and delegates to the suggestion plugin lifecycle. |
| demos/src/Experiments/Commands/Vue/suggestion.js | Removes demo-level Escape handling and delegates to the suggestion plugin lifecycle. |
| demos/src/Examples/MultiMention/Vue/suggestions.js | Removes demo-level Escape handling and delegates to the suggestion plugin lifecycle. |
| demos/src/Examples/MultiMention/React/suggestions.js | Removes demo-level Escape handling and delegates to the suggestion plugin lifecycle. |
| demos/src/Examples/Community/Vue/suggestion.js | Removes demo-level Escape handling and delegates to the suggestion plugin lifecycle. |
| demos/src/Examples/Community/React/suggestion.js | Removes demo-level Escape handling and delegates to the suggestion plugin lifecycle. |
| .changeset/new-carrots-hide.md | Documents the user-facing bugfix and behavioral note for onExit on Escape. |
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
262
to
267
| return () => { | ||
| const state = pluginKey.getState(editor.state) | ||
| const decorationId = state?.decorationId | ||
| const currentDecorationNode = view.dom.querySelector(`[data-decoration-id="${decorationId}"]`) | ||
|
|
||
| return currentDecorationNode?.getBoundingClientRect() || null |
Author
There was a problem hiding this comment.
Again, onExit() typically does not send a decorationNode, and there is already code to use clientRect from the anchor when decorationNode is missing, so it should be fine.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes Overview
Hitting escape when suggestion is open no longer triggers multiple calls to
renderer.onExit(), and behaves more in line with otheronExit()calls.Implementation Approach
Looking at the code for handling escape presses, it initially called
renderer.onExit()directly, and then continued to calldispatchExit()which in turn also calledrenderer.onExit()directly, before finally dispatching the meta-only transaction which reachesview.updateand triggers the baselineonExit()logic.I added a test which simply triggers escape and counts the number of times
onExit()is called, and initially it was triggered 3 times.This change removes the extra calls to
renderer.onExit()and letting the "default" path of handling it inview.updatego through, triggeringonExit()exactly once, regardless of how the suggestions are closed.Note: the initial
onExit()call would include thedecorationNodeelement to theonExit()listener, but I'm making the assumption that this is not strictly necessary since the element itself will be removed within the same execution once the transaction goes through, so you can't really use the element for a lot. You will still getclientRectif you need to position something relating to the canceled suggestion. Also no other scenario of closing suggestions will giveonExit()the decorationNode. If this slight change in behaviour is something we want to avoid, I can look at trying to pass it before exiting.I also updated all exampels to no longer use their own escape handling. I assume they had this before the general escape handler was built into suggestion. The fact that they all handled escape on their own meant that they
If we want the exampels to handle escape on their own, they're probably better of using
exitSuggestion()utility instead.Testing Done
A test was added to make sure that triggering escape triggers onExit a single time.
All the touched exampels was manually tested
Verification Steps
Verify that exiting suggestions using escape, or
exitSuggestion()works as expected.Consider if it's likely that consumers rely on
decorationNodeinonExit()specifically for escape presses.Additional Notes
Related to: #6833, #6908, #6910
Checklist
Related Issues