Fix delete behaviour when selection ends in void element #3990

songeunyou · 2020-11-20T00:40:23Z

Is this adding or improving a feature or fixing a bug?

Fixing a bug

What's the new behavior?

Using Cmd+A -> Delete properly deletes the selection when the last element in the editor is a void element or a void element followed by a newline.

Intuitive cursor position after delete (Addressing [#3868])

This fix allows void nodes to be deleted correctly and places the cursor on a newline after deletion.

How does this change work?

Previously, when using Cmd+A -> Delete, selections ending with a void element had their endpoint at offset: 0 (before the void element), causing Editor.unhangRange to trigger and omit the last elements when executing delete.

Now, Transforms.delete checks that the last element is not a void element before calling Editor.unhangRange. If it is a void element, Editor.unhangRange is skipped, allowing the selection to be deleted properly.

Additionally, voids is passed as true to Editor.nodes within Editor.unhangRange so that void elements will be iterated through while unhanging. This resolves the issue of buggy deletion on void elements followed by a newline and does not affect selections without void elements.

Two new tests were added to address these changes. The output of the test delete-void-and-newline-at-end has an extra <block/> than would be expected; this is another small bug that occurs when deleting selections ending in a newline. However, that is an issue that should be addressed in a separate PR.

Have you checked that...?

The new code matches the existing patterns and styles.
The tests pass with yarn test.
The linter passes with yarn lint. (Fix errors with yarn fix.)
The relevant examples still work. (Run examples with yarn start.)

Does this fix any issues or need any specific reviewers?

Fixes two sub-issues mentioned in: [#3456]
Fixes: [#4005]
Fixes adjacent issue: [#3868]

- Include void node in range - Add delete-void-at-end test - Add delete-void-and-newline-at-end test

BrentFarese · 2020-12-11T03:46:12Z

packages/slate/src/interfaces/editor.ts

@@ -1265,6 +1265,13 @@ export const Editor = {
      at: end,
      match: n => Editor.isBlock(editor, n),
    })
+
+    // If last element is a void node, unhang range by including void node in range
+    if (Editor.isVoid(editor, endBlock[0])) {


I am not sure this the best solution. Should we just simply not unhang the range in Transforms.delete if the selection end is inside a void node? I think maybe we should handle this in Transforms.delete vs. changing the default behavior of unhangRange? Let's pair on it.

I thought there were some path issues before, but I didn't realize that they're handled further down in Transforms.delete. I've moved the end void node handling to Transforms.delete so that it now skips unhangRange. If you think there are still changes that could be made I'm happy to pair. Thanks for the feedback!

- remove end void node handling from Editor.unhangRange

BrentFarese · 2020-12-11T15:46:27Z

packages/slate/src/interfaces/editor.ts

@@ -1274,7 +1275,7 @@ export const Editor = {
      at: before,
      match: Text.isText,
      reverse: true,
-      voids,
+      voids: true,


I think we might want to leave this as a variable and perhaps change how Transforms.delete is called in slate-react instead.

BrentFarese · 2020-12-11T16:52:21Z

packages/slate/src/transforms/text.ts

+        const [, end] = Range.edges(at)
+        const endBlock = Editor.above(editor, {
+          at: end,
+          match: n => Editor.isBlock(editor, n),
+        })
+
+        if (endBlock && !Editor.isVoid(editor, endBlock[0])) {
+          at = Editor.unhangRange(editor, at, { voids })
+        }


I think this might result in another set of bugs that we don't really intend to introduce. What if the endBlock is a void but it's not at the end of the document? Then, the void node would be deleted because the range is not "unuhung". Are there test cases for hanging ranges with voids in the middle of a document? If not, we should likely add a test case(s). And, maybe we consider doing something like the below:

Suggested change

const [, end] = Range.edges(at)

const endBlock = Editor.above(editor, {

at: end,

match: n => Editor.isBlock(editor, n),

})

if (endBlock && !Editor.isVoid(editor, endBlock[0])) {

at = Editor.unhangRange(editor, at, { voids })

}

const [, end] = Range.edges(at)

const endDoc = Editor.end(editor, []);

if (!Point.equals(end, endDoc)) {

at = Editor.unhangRange(editor, at, { voids })

}

Need to think about this behavior a bit more. The above might not be exactly what we want. We should pair.

lukesmurray · 2021-01-30T00:07:34Z

Potentially related to this. In next, if the last element is a void element, then you click away, and refocus, the void element gets mouse focus and you can't edit or do anything until you use the arrow keys to navigate away from the void element.

songeunyou · 2021-01-31T09:56:10Z

@lukesmurray what is "next" in reference to? Also, will take a look into this + finish up this fix this weekend, thanks for the comment.

lukesmurray · 2021-01-31T21:59:51Z

slate@next published on npm. Version 0.60.2.

songeunyou · 2021-02-09T22:53:05Z

@BrentFarese, I looked into the default voids behavior in Transforms.delete that was raised during our pairing session, and the current implementation is correct (voids: false denotes that text within void elements can be deleted). Changing it introduces further issues.

The current changes in this PR still fixes all of the issues, so I would appreciate a final review + merge!

ulion · 2021-03-03T15:10:42Z

I encountered similar but a little different issues. here is the case:
<text>|abc</text><inline void/><text>|</text>
If select bc+inline void, it can be deleted, but if select all (abc+inline void), the inline void is not deleted due to the unhangRange method skipped the last text, then skipped the inline void since it does not match Text.isText, finally after unhang it got only 'abc'. That is definitely incorrect unhang result. despite start.offset check in unhangRange is no reason. the inline void should not be removed from selection, nor the inline void be counted as anything hanging.
So, what's proper fix for this?

ianstormtaylor · 2021-03-31T19:37:34Z

/rebase

There's a slight change in behavior, but we accept it. Also there was a regression in how slate handles void nodes in 0.63, but then it was fixed somewhere between 0.63 and 0.66. See ianstormtaylor/slate#3990

songeunyou force-pushed the master branch from 1842554 to 91bdcb2 Compare December 9, 2020 23:22

UnhangRange considers void nodes

855e25f

- Include void node in range - Add delete-void-at-end test - Add delete-void-and-newline-at-end test

songeunyou force-pushed the master branch from 67f77d3 to 855e25f Compare December 10, 2020 18:40

BrentFarese reviewed Dec 11, 2020

View reviewed changes

Move end void node handling to Transforms.delete

c46fd91

- remove end void node handling from Editor.unhangRange

BrentFarese reviewed Dec 11, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/main'

0c82e90

clauderic mentioned this pull request Mar 16, 2021

Fix deletion of hanging ranges with trailing void block #4125

Merged

4 tasks

ianstormtaylor merged commit 17b0bc5 into ianstormtaylor:main Mar 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix delete behaviour when selection ends in void element #3990

Fix delete behaviour when selection ends in void element #3990

songeunyou commented Nov 20, 2020 •

edited

Loading

BrentFarese Dec 11, 2020

songeunyou Dec 11, 2020

BrentFarese Dec 11, 2020

BrentFarese Dec 11, 2020

lukesmurray commented Jan 30, 2021

songeunyou commented Jan 31, 2021

lukesmurray commented Jan 31, 2021

songeunyou commented Feb 9, 2021

ulion commented Mar 3, 2021 •

edited

Loading

ianstormtaylor commented Mar 31, 2021

Fix delete behaviour when selection ends in void element #3990

Fix delete behaviour when selection ends in void element #3990

Conversation

songeunyou commented Nov 20, 2020 • edited Loading

Is this adding or improving a feature or fixing a bug?

What's the new behavior?

How does this change work?

Have you checked that...?

Does this fix any issues or need any specific reviewers?

BrentFarese Dec 11, 2020

Choose a reason for hiding this comment

songeunyou Dec 11, 2020

Choose a reason for hiding this comment

BrentFarese Dec 11, 2020

Choose a reason for hiding this comment

BrentFarese Dec 11, 2020

Choose a reason for hiding this comment

lukesmurray commented Jan 30, 2021

songeunyou commented Jan 31, 2021

lukesmurray commented Jan 31, 2021

songeunyou commented Feb 9, 2021

ulion commented Mar 3, 2021 • edited Loading

ianstormtaylor commented Mar 31, 2021

songeunyou commented Nov 20, 2020 •

edited

Loading

ulion commented Mar 3, 2021 •

edited

Loading