-
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
[Android] Fix edge-cases when text leaf nodes are deleted #5325
Conversation
🦋 Changeset detectedLatest commit: fc79dd5 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 |
@@ -153,7 +154,7 @@ export function createAndroidInputManager({ | |||
EDITOR_TO_PENDING_DIFFS.get(editor) | |||
) | |||
|
|||
let scheduleSelectionChange = !!EDITOR_TO_PENDING_DIFFS.get(editor)?.length | |||
let scheduleSelectionChange = hasPendingDiffs() |
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.
Unrelated to this PR, but thought I would clean this up while I was here, hasPendingDiffs()
is equivalent to !!EDITOR_TO_PENDING_DIFFS.get(editor)?.length
if (type.startsWith('delete')) { | ||
if (Range.isExpanded(targetRange)) { |
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.
Just re-structuring the code paths here to avoid checking type.startsWith('delete')
multiple times
5a3ddb1
to
fc79dd5
Compare
if (Range.isExpanded(targetRange) && type.startsWith('delete')) { | ||
const [start, end] = Range.edges(targetRange) | ||
const leaf = Node.leaf(editor, start.path) | ||
|
||
if (leaf.text.length === start.offset && end.offset === 0) { | ||
const next = Editor.next(editor, { at: start.path, match: Text.isText }) | ||
if (next && Path.equals(next[1], end.path)) { | ||
targetRange = { anchor: end, focus: end } | ||
} | ||
} | ||
} |
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.
This code block hasn't been deleted, it is just relocated on line 393
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, going to leave it for a day or two in case @BitPhinix wants to take a look.
Description
This PR fixes edge-cases in the Android input manager when text leafs are deleted.
Issue
Fixes:
Context
Previously, text leaf deletions were stored in pending diffs and only reconciled when flushing as a result of a selection change or an action (such as inserting a line break). This could result in a number of different edge cases, such as the browser not knowing where to place the selection after deleting a text node before or after a void node.
In order to fix these edge cases, we detect when an input event would result in a text leaf being deleted, and schedule an action to allow Slate to reconcile its state with the DOM and apply normalization fixes (such as inserting zero-width spacers next to inline void nodes) rather than storing these events as deferred text diffs.
Example
Before
Screen.Recording.2023-02-28.at.4.06.09.PM.mov
After
Screen.Recording.2023-02-28.at.4.05.03.PM.mov
Note
There is a separate issue for the keyboard being closed when the selection moves right before void nodes, this PR does not solve that separate issue:
If we apply the fix described in the PR linked above, we can see what the final desired behaviour would eventually look like once we find a solution to #5183:
Screen.Recording.2023-02-28.at.4.09.53.PM.mov
Checks
yarn test
.yarn lint
. (Fix errors withyarn fix
.)yarn start
.)yarn changeset add
.)