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 Cursor blot restoration and selection preservation #2354

Merged
merged 3 commits into from
Oct 15, 2018

Conversation

dgreensp
Copy link
Contributor

@dgreensp dgreensp commented Oct 12, 2018

This PR refixes issue #1019 and de0d564.

Repros for bugs addressed:

  1. The repro in Undo issue #1019
  2. Type aa, click between the two A's, then do command-B, command-B, then type b. You should get a plain aba with the cursor after the b. Added a functional test for this.
  3. Type aa, click between the two A's, then do command-B, type b, command-B, type c. You should get abca with a bold b. Added a functional test for a case similar to this.
  4. Type a string of text (e.g. asdfasdf), click in the middle of it, then do command-B, command-B, and then click somewhere else in the text, before or after the first position. The text cursor should stay where you planted it (instead of e.g. jumping to the end of the line). This is equivalent to the unit test that was failing.

Basically, to be correct, cursor.restore has to take a number of things into account:

  • It returns a new native selection range to apply, but before this range is applied, TextBlot optimization happens, so the returned range should not reference any text nodes that will be removed by this optimization.
  • The cause of the cursor.restore may be a selection change from the user clicking in a different text node, such as that of an adjacent TextBlot, which may be optimized away. Therefore, selection preservation must operate even if the selection isn't in the Cursor blot's own textNode. This is the subject of repro 4 above. In this case, I also had to make sure that the return value of cursor.restore is always used by the caller!
  • The fix to Undo issue #1019 partly accounted for the optimization hack in editor.update, but for full correctness there mustn't be any case of cursor.restore that uses the Cursor blot's own textNode, mutated by the user, as part of the document (outside of the Cursor blot which is removed), because then the "hack path" in editor.update will think it has a green light and try to generate a delta based on the current value of the text node, which is not going to come out properly.

The solution is for cursor.restore to optimize the surrounding text nodes on the spot and calculate an appropriate selection range if the old selection is anywhere in those text nodes.

@dgreensp dgreensp requested a review from jhchen October 12, 2018 07:10
@dgreensp dgreensp merged commit 9d4b827 into develop Oct 15, 2018
@dgreensp dgreensp deleted the dg-cursor-span-bug branch October 15, 2018 15:23
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.

2 participants