-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
fix: sync state on undo when editor is unfocused #5737
fix: sync state on undo when editor is unfocused #5737
Conversation
🦋 Changeset detectedLatest commit: e00d08e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable, it is triggering a failing test/example so fixing that would be cool.
@dylans Sorry, Actions didn't give me any feedback last time, so I thought all the tests were successful. But now it seems like there's an issue with the test:integration environment. Comparing the old test results with the new ones, I found that the previous test results for Chromium were successful. Old: https://github.com/ianstormtaylor/slate/actions/runs/11258958959 Regarding the issue with the test environment failure, maybe I can handle it :) |
I merged the fix #5742 for the test:integration environment into my own test branch , and the tests ran smoothly. https://github.com/WindRunnerMax/slate/actions/runs/11364143647 |
* fix: fix firefox test integration env * chore: test ubuntu apt source * chore: IMMUTABLE_INSTALLS ? * fix: ubuntu version
@WindRunnerMax it looks like we're on a very old version of Node (12) which is then being kicked to the oldest available (16) which is likely too old. If you have time to upgrade the ci config for Slate to use Node 20, awesome, otherwise I'll get to that this week. |
@dylans Sure, I have time to finish this task this week. Also, I rebase the current main branch, triggered the CI tests, and the results were good. |
Description
Undo when the editor is not in focus will change editor content, but won't update editor state.
In my tests, this issue only appears on Chrome/Chromium. It doesn't occur on Firefox, Safari, or Edge. However, the #5544 report indicates that it also happens on Edge and Safari, so I didn't limit the patch to Chrome.
Issue
Fixes: #5544
Example
After entering some content, click elsewhere to make the editor lose focus. Then use Ctrl/Cmd+Z to undo. When you start typing again, you'll notice the previously undone content reappears.
After making adjustments.
Context
I tried to get to the root of the problem. If you completely preventDefault the beforeinput event, nothing will be placed in the browser's undo stack, so input events like historyUndo won't be triggered.
As mentioned in the comments, if you completely block the event, we won't be able to handle undo events in beforeinput.
slate/packages/slate-react/src/components/editable.tsx
Lines 1498 to 1501 in 8c7f7ea
However, because there's a conditional preventDefault in the beforeinput event, some content will still enter the browser's undo stack. For example, typing
123
won't cause the issue, but typingabc
will trigger the undo problem when losing focus.slate/packages/slate-react/src/components/editable.tsx
Lines 656 to 658 in 8c7f7ea
I came up with four solutions to address this issue.
In the end, I chose option 3. I found that triggering this behavior only fires the oninput event of that node, while the onbeforeinput and onkeydown events don't get triggered. It's quite fascinating.
Also, I tried to prevent the undo action in this input event, but it didn't work. event.preventDefault didn't do the trick here. So, I decided to check whether we should be in edit mode to determine if we should manually call the editor's undo/redo methods. Interestingly, when this action is triggered, we can determine the state by checking the focus.
In this case, I ran a lot of tests to avoid any unexpected undo/redo actions.
Checks
yarn test
.yarn lint
. (Fix errors withyarn fix
.)yarn start
.)yarn changeset add
.)