fix(editor): keep word delete and move on whole characters - #2727
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while investigating #2717 (PR #2724).
Option+Delete, Option+Forward Delete and Option+Left/Right in the SQL and JSON editors could split a character in two. Deleting the word
éx(e+ U+0301) left a lone combining accent. Deleting after'%😀abcremovedabcplus half of the emoji, which left an unpaired surrogate that draws as a replacement box. Control+Delete on💯deleted one half of the pair. Each of these leaves exactly the kind of invisible or broken character #2717 was about.Root cause
TextSelectionManager.extendSelectionWordwalks the text withNSString.enumerateSubstrings(options: .byCaretPositions), which hands back one grapheme cluster per step, and grew anNSRangebysubstring.counton each step.NSRangecounts UTF-16 code units, andString.countcounts grapheme clusters. So every character wider than one UTF-16 unit (a combining sequence, anything outside the BMP, an emoji with a modifier or ZWJ, a flag) made the range one or more units short. Backward, the start landed inside the first character of the word. Forward, the end landed inside the last one. The same range drives word delete, word move and word select, so all six commands were off.extendSelectionCharacterwithdecomposeCharacters(Control+Delete,deleteBackwardByDecomposingPreviousCharacter:) returned a single UTF-16 unit. That separates a combining mark correctly, but it also cut a surrogate pair in half. AppKit'sNSTextViewremoves one Unicode scalar here: all of😀, only the skin tone modifier of👍🏽, only U+0301 ofé(measured with a probe againstNSTextView.doCommand(by:)).What changed
All in
LocalPackages/CodeEditTextView,SelectionManipulation+Horizontal.swift:substringRange), which are already in UTF-16 units. No counting.rangeOfComposedCharacterSequence(at:)directly.findBeginningOfLineTextstepped the same way (one unit per caret position). It now moves to the end of the enumerator's range instead. I found no input where the old stepping gave a wrong answer, since every horizontal whitespace character is a single unit, but it was the same unit mix.TextSelectionManagerTestshad two expectations that encoded the bug: on"Loren Ipsum 💯", a decomposing step over💯expected(13, 1)and(12, 1), which is half of the surrogate pair. They now expect(12, 2), the whole scalar, matchingNSTextView.Verification
swift test --package-path LocalPackages/CodeEditTextView(the CI gate), on the committed HEAD: XCTest 52 tests, 0 failures; Swift Testing 269 tests in 26 suites, all passed. Log:scratchpad/word-delete-surrogates-package-tests-publish.log.WordNavigationUnicodeTests: 12 tests, 84 cases. Eight parameterized tests run 10 strings each (combining marks,naïve, Deseret and math-italic letters outside the BMP, emoji with a skin tone, a ZWJ family, emoji before and after an identifier) through the realTextViewresponder methods:deleteWordBackward(_:),deleteWordForward(_:),moveWordLeft(_:),moveWordRight(_:)and bothAndModifySelectionvariants, plus the raw word range. Four cases cover the decomposing character step. All 84 pass.SelectionManipulation+Horizontal.swiftfail with 83 issues across 11 of the 12 tests. The one that passes on both isdecomposingBackwardSeparatesACombiningMark, which guards that the scalar fix still splitséthe way AppKit does.TextSelectionManagerTestsfails 3 assertions on the old code ({13, 1}and{12, 1}). Log:scratchpad/word-delete-surrogates-baseline-publish.log.verify.sh buildsucceeded (.analysis/fix-word-delete-surrogate-pairs/logs/build-TablePro-154459.log).verify.sh testover the editor suitesEditorContextMenuRoutingTestsandStatementNavigationCommandTests: 18 cases passed (.analysis/fix-word-delete-surrogate-pairs/logs/test-EditorContextMenuRoutingTests-144530.log).swiftlint lint --strict: clean on the changed source file and the new test file.TextSelectionManagerTests.swiftreports 17xct_specific_matcherviolations, the same 17 as onmain, none on the edited lines.TextView, so a UI test would only add keyboard driving of astral-plane input.Notes
Codex review was unavailable (usage limit), so an adversarial reviewer agent read the diff and probed the old and new word logic side by side. What it raised and what I did with it:
😀abc, Option+Delete now removes😀abc, whileNSTextViewremoves onlyabc. The editor's word rule treats an emoji as part of a word because it is neither punctuation nor whitespace. That rule predates this change and is a policy question, not the defect: the old code removedabcand half of the emoji. The tests accept either range, so a later change to the word rule does not have to rewrite them.🇻🇳abcused to loseabcplus the trailing surrogate of the second regional indicator. It now goes with the word as a whole, by the same rule as above.eand U+0301, a forward step starts mid-cluster and still separates them. The editor's own movement commands never leave the caret there, so this is not reachable from the keyboard and is left alone.\r\nis one caret position of two units; the reviewer confirmed the old and new ranges are identical for word moves across it.❤️(U+2764 U+FE0F) selects U+2764 alone and leaves the variation selector outside the selection. That isfindWordBoundary, the double-click word selection, which this PR does not touch. Worth a follow-up since deleting that selection leaves an invisible U+FE0F behind.https://claude.ai/code/session_011THKc9TRHE8xjcxXidDHXP