Skip to content
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 erratic cursor jumps to last line #1193

Merged
merged 16 commits into from
Feb 8, 2019
Merged

Fix erratic cursor jumps to last line #1193

merged 16 commits into from
Feb 8, 2019

Conversation

mirka
Copy link
Member

@mirka mirka commented Feb 6, 2019

Closes #1185
Based on @qualitymanifest's work in #1192

Some background on the issue is detailed in #1192 (comment).

TODO

  • Fix "New Note"
  • Fix revision selection
  • Test as hotfix on 1.4.0 (Electron 2)

In relation to #36

Before this PR, the cursor just moved to the end when a remote change came in. After this PR, the cursor will be restored to the same paragraph/character indices. So definitely a UX improvement. However, we aren't doing any calculation based on the Simperium patch, which means it doesn't do anything fancy like advance the cursor six characters if six characters were inserted before it.

Technical notes

It seemed like @qualitymanifest's approach might be sufficient (as long as the undo stack was preserved 23d10c4), but it turned out that it would cause flickers and data loss if CJK typing was involved 😬

To protect CJK typing, we absolutely need a way to distinguish between content updates that come from local changes vs. those coming remotely from the server. I did this by adding a property to the Redux state b8c71f2.

Combined with a new noteId prop on the component, we can now distinguish between the three kinds of content updates:

a. Another note was selected
b. The content was changed locally
c. A remote change came in from the server

They will be handled as such: (a) will recreate an editorState from scratch, (b) will be ignored (so the internal state in DraftJS will stay in control), and (c) will be sent through @qualitymanifest's special focus handling.

Testing

Some things to try:

  • Try to trigger the original bug as detailed in Cursor jump to last line #1185 (comment)
  • Open the same note on a different Simplenote client, and see how remote changes are applied.
  • Is Markdown preview working as expected?
  • Are checklists working as expected?

@roundhill
Copy link
Contributor

I tested this out and am not seeing the cursor jump any longer 👍

But, I've found that I can get a crash. It's probably not a normal use case though :) It also may be unrelated to this PR. If you type really fast on a line and keep selecting the text you just typed with the mouse, it'll eventually crash.

https://cloudup.com/co8U9-Abcgn

Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
    at removeChild (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:1891:413)
    at unmountHostComponents (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:3142:91)
    at commitDeletion (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:3151:1)
    at commitAllHostEffects (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:3272:177)
    at HTMLUnknownElement.callCallback (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:66:102)
    at Object.invokeGuardedCallbackDev (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:86:45)
    at invokeGuardedCallback (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:102:126)
    at commitRoot (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:3294:118)
    at completeRoot (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:3624:60)
    at performWorkOnRoot (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:3605:1)
removeChild @ react-dom.development.js?f8c1:8703
unmountHostComponents @ react-dom.development.js?f8c1:16470
commitDeletion @ react-dom.development.js?f8c1:16519
commitAllHostEffects @ react-dom.development.js?f8c1:17202
callCallback @ react-dom.development.js?f8c1:145
invokeGuardedCallbackDev @ react-dom.development.js?f8c1:198
invokeGuardedCallback @ react-dom.development.js?f8c1:253
commitRoot @ react-dom.development.js?f8c1:17417
completeRoot @ react-dom.development.js?f8c1:18909
performWorkOnRoot @ react-dom.development.js?f8c1:18841
performWork @ react-dom.development.js?f8c1:18729
performSyncWork @ react-dom.development.js?f8c1:18712
interactiveUpdates$1 @ react-dom.development.js?f8c1:18979
interactiveUpdates @ react-dom.development.js?f8c1:2162
dispatchInteractiveEvent @ react-dom.development.js?f8c1:4872
5react-dom.development.js?f8c1:15749 The above error occurred in the <div> component:
    in div (created by DraftEditorContents)
    in div (created by DraftEditorContents)
    in DraftEditorContents (created by DraftEditor)
    in div (created by DraftEditor)
    in div (created by DraftEditor)
    in div (created by DraftEditor)
    in DraftEditor (created by NoteContentEditor)
    in div (created by NoteContentEditor)
    in NoteContentEditor (created by NoteDetail)
    in div (created by NoteDetail)
    in div (created by NoteDetail)
    in div (created by NoteDetail)
    in NoteDetail (created by Connect(NoteDetail))
    in Connect(NoteDetail) (created by NoteEditor)
    in div (created by NoteEditor)
    in NoteEditor (created by Connect(NoteEditor))
    in Connect(NoteEditor) (created by AppLayout)
    in div (created by AppLayout)
    in Suspense (created by AppLayout)
    in div (created by AppLayout)
    in AppLayout (created by App)
    in div (created by App)
    in div (created by App)
    in App (created by Connect(App))
    in Connect(App) (created by BrowserShell)
    in BrowserShell
    in Provider

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://fb.me/react-error-boundaries to learn more about error boundaries.
logCapturedError @ react-dom.development.js?f8c1:15749
logError @ react-dom.development.js?f8c1:15749
update.callback @ react-dom.development.js?f8c1:16645
callCallback @ react-dom.development.js?f8c1:11154
commitUpdateEffects @ react-dom.development.js?f8c1:11191
commitUpdateQueue @ react-dom.development.js?f8c1:11191
commitLifeCycles @ react-dom.development.js?f8c1:15991
commitAllLifeCycles @ react-dom.development.js?f8c1:17202
callCallback @ react-dom.development.js?f8c1:145
invokeGuardedCallbackDev @ react-dom.development.js?f8c1:198
invokeGuardedCallback @ react-dom.development.js?f8c1:253
commitRoot @ react-dom.development.js?f8c1:17452
completeRoot @ react-dom.development.js?f8c1:18909
performWorkOnRoot @ react-dom.development.js?f8c1:18841
performWork @ react-dom.development.js?f8c1:18729
performSyncWork @ react-dom.development.js?f8c1:18712
interactiveUpdates$1 @ react-dom.development.js?f8c1:18979
interactiveUpdates @ react-dom.development.js?f8c1:2162
dispatchInteractiveEvent @ react-dom.development.js?f8c1:4872

@qualitymanifest
Copy link
Contributor

qualitymanifest commented Feb 7, 2019

@roundhill I can confirm that that is unrelated to these updates. I've made that happen before when I was first testing trying to identify the cursor jumping issue. I just double-checked on the production version of 1.4.0 that I have installed on my system and you can cause that error there too. You can even make it happen on the editor on draftjs.org (their editor doesn't go blank when it happens but you get the error in the console).

@mirka mirka changed the title [WIP] Fix erratic cursor jumps to last line Fix erratic cursor jumps to last line Feb 7, 2019
@mirka mirka requested a review from roundhill February 7, 2019 22:48
Copy link
Contributor

@roundhill roundhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested well for me! I'm afraid I'm out of time for the day so didn't have much time to review the code.

@mirka
Copy link
Member Author

mirka commented Feb 8, 2019

Thank you @roundhill and @qualitymanifest for testing and confirming 🙏

@mirka mirka merged this pull request into master Feb 8, 2019
@mirka mirka deleted the fix/focus-jumping branch February 8, 2019 10:56
mirka added a commit that referenced this pull request Feb 8, 2019
* Fix cursor position jump when setting new EditorState

* Use const for newSelectionState

* Create new editor state preserving undo stack

* Remove use of componentWillReceiveProps()

Deprecated

* Move and rename focus calculating function

* Return the new, merged selection state

* Add hasRemoteUpdate hint

* Rename `noteUpdated` action for clarity

* Handle content updates differently

Three kinds of content updates:
1. Another note was selected
2. The content was changed locally
3. A remote change came in from the server

* Add tests and adjust for new upper bounds

* Fix revision selection

* Fix New Note

* Remove unneeded loadNotes on newNote

* Fix state bug when adding new note after trashing

* Fix bug when clicking checkbox

* Fix bug when making new note when editor has focus
mirka added a commit that referenced this pull request Feb 8, 2019
* Fix cursor position jump when setting new EditorState

* Use const for newSelectionState

* Create new editor state preserving undo stack

* Remove use of componentWillReceiveProps()

Deprecated

* Move and rename focus calculating function

* Return the new, merged selection state

* Add hasRemoteUpdate hint

* Rename `noteUpdated` action for clarity

* Handle content updates differently

Three kinds of content updates:
1. Another note was selected
2. The content was changed locally
3. A remote change came in from the server

* Add tests and adjust for new upper bounds

* Fix revision selection

* Fix New Note

* Remove unneeded loadNotes on newNote

* Fix state bug when adding new note after trashing

* Fix bug when clicking checkbox

* Fix bug when making new note when editor has focus

Co-authored-by: cdr <qualitymanifest@gmail.com>
@codebykat codebykat added this to the 1.4.1 milestone Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor jump to last line
4 participants