Fix backspace over a flag emoji leaving a dangling regional indicator - #334808
Open
Aditya Jagtap (AdityaJagtap18) wants to merge 2 commits into
Open
Conversation
getLeftDeleteOffset()/getOffsetBeforeLastEmojiComponent() already treat an emoji modifier sequence (e.g. a skin-tone-modified emoji) as a single unit for backspace, per the fix for microsoft#99629. Flag emoji were not covered by that special-casing: a flag is two Regional Indicator Symbol code points (e.g. US = U+1F1FA U+1F1F8), and backspacing over one deleted only the trailing code point, leaving a dangling, meaningless lone Regional Indicator behind (fixable only with a second backspace) — the exact class of bug microsoft#99629 was meant to prevent, just for a different emoji construct. Per Unicode UAX microsoft#29 rules GB12/GB13, Regional Indicators combine greedily in pairs from the start of a maximal run of consecutive indicators, so a run's length parity determines whether its last indicator is paired. Added isRegionalIndicator()/isRegionalIndicatorPaired() and used them so a complete flag pair is deleted together, while a genuinely unpaired trailing indicator (odd run length) is still deleted alone. Fixes microsoft#334807. Verified by importing the real, unmodified strings.ts directly (via tsx, no transpilation of the algorithm by hand) and simulating repeated backspace over several emoji constructs before and after the fix: - Skin-tone modifier sequence: unaffected, still one unit (regression check). - ZWJ family emoji: unaffected, still peeled component-by-component to match Firefox's behavior per the discussion in microsoft#99629 (regression check). - Single flag: now one backspace instead of two, no dangling indicator. - Two adjacent flags: now one flag per backspace instead of leaving a dangling indicator after each of the first three presses. - A flag followed by a lone unpaired indicator: the lone indicator is still deleted alone, then the completed flag as a unit (no dangling step). Also ran the existing src/vs/base/test/common/strings.test.ts file (54 tests, all passing) via a minimal suite/test shim under tsx, since a full `npm install` + Electron test run was impractical in this environment — this exercises the real, unmodified test file end-to-end, just outside the standard mocha/Electron runner, so a maintainer re-running it via `./scripts/test.sh --run src/vs/base/test/common/strings.test.js` is the recommended final check before merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Copilot started reviewing on behalf of
Aditya Jagtap (AdityaJagtap18)
September 6, 2026 13:09
View session
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The remaining comments are non-blocking documentation nits.
Pull request overview
Fixes Backspace handling so paired Regional Indicators forming flag emoji are deleted atomically.
Changes:
- Adds parity-based Regional Indicator pairing logic.
- Adds regression tests for flags and existing emoji behavior.
File summaries
| File | Description |
|---|---|
src/vs/base/common/strings.ts |
Handles paired Regional Indicators during left deletion; minor documentation nits remain. |
src/vs/base/test/common/strings.test.ts |
Tests flags, modifiers, ZWJ sequences, and plain text. |
Review details
Suppressed comments (2)
src/vs/base/common/strings.ts:1245
- This JSDoc restates the implementation across multiple sentences, exceeding the repository's 1–2 short-sentence limit for function documentation. A single sentence preserves the useful contract without documenting the algorithm twice.
/**
* Determines whether the regional indicator immediately preceding `offset`
* (already accounted for by the caller) is paired with the one before it.
src/vs/base/common/strings.ts:1245
- The offset passed by the caller is the start of the regional indicator already counted in
runLength, not the position after it. As written, this contract describes the preceding indicator and can mislead future callers.
* Determines whether the regional indicator immediately preceding `offset`
* (already accounted for by the caller) is paired with the one before it.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1209
to
+1213
| // Regional indicators combine in pairs to form a single flag emoji | ||
| // (Unicode UAX #29 GB12/GB13: do not break within an emoji flag | ||
| // sequence). If this regional indicator completes such a pair, | ||
| // delete both code points together; otherwise it is a lone, | ||
| // unpaired indicator and only it should be deleted. |
The previous wording described offset as pointing to "the regional indicator immediately preceding offset", but offset is actually the boundary of the regional indicator the caller already counted, not a separate one before it. Also trimmed to match the repo's 1-2 sentence convention for function docs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author
|
@microsoft-github-policy-service agree |
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.
Fixes #334807.
getLeftDeleteOffset()(viagetOffsetBeforeLastEmojiComponent()) already treats an emoji modifier sequence — e.g. a skin-tone-modified emoji like 👶🏾 — as a single unit for Backspace, per the fix for #99629. Flag emoji weren't covered: a flag is two Regional Indicator Symbol code points (e.g. 🇺🇸 = U+1F1FA U+1F1F8), and backspacing over one deleted only the trailing code point, leaving a dangling, meaningless lone Regional Indicator behind — the exact class of bug #99629 was meant to prevent, just for a different emoji construct.What changed
Per Unicode UAX #29 rules GB12/GB13, Regional Indicators combine greedily in pairs from the start of a maximal run of consecutive indicators, so the run's length parity determines whether the last indicator in it is paired. Added
isRegionalIndicator()/isRegionalIndicatorPaired()instrings.tsand used them ingetOffsetBeforeLastEmojiComponent()so:Before / after
Testing
I added
suite('getLeftDeleteOffset', ...)tosrc/vs/base/test/common/strings.test.ts, covering all five rows above plus a plain-text sanity check.Verification method (please re-run before merge): a full
npm install+ Electron test run (./scripts/test.sh --run ...) wasn't practical in the environment I used, so I verified two ways instead:strings.tsdirectly withtsx(no hand-transpilation of the algorithm) and simulated repeated Backspace over the emoji constructs in the table above, before and after the fix, confirming the "before" column reproduces the bug and the "after" column matches the fix.src/vs/base/test/common/strings.test.ts(54 tests, including the new ones) end-to-end via a minimalsuite/test/setup/teardownshim undertsx, standing in for mocha — all 54 pass.Both give me high confidence in the change, but since this bypassed the project's actual mocha/Electron test runner, I'd appreciate a maintainer (or CI) re-running it via
npm run test-node/./scripts/test.sh --run src/vs/base/test/common/strings.test.jsas the authoritative check before merge.I used Claude Code to help locate this bug, derive the Unicode-spec-based fix, and draft this PR and description; I reviewed the diff and the reasoning by hand before submitting.
🤖 Generated with Claude Code