fix: prevent surrogate pairs from being split by the selected range#1226
fix: prevent surrogate pairs from being split by the selected range#1226Thomaash wants to merge 2 commits intobasecamp:mainfrom
Conversation
|
Hi @jorgemanrubia, could I ask for a code review? |
|
Hi @jorgemanrubia or @flavorjones, is there any chance for this to be reviewed and merged/rejected? |
This is already handled in functions like `expandSelectionInDirection` which won't select a half of emoji etc. but always the whole surrogate pair. The solution here is the same, if the cursor is placed in the middle of a surrogate pair, it will be moved to the end of the pair. Similarly a selection will either select both or neither.
|
I've rebased this onto current @Thomaash can you say a bit more about how this problem occurs in an application -- how is the cursor ending up in the middle of a multi-byte code point? |
|
@Thomaash bump! 🙏 |
|
This happens when you have text with surrogate pairs and programmatically move the cursor. Some functions will move over the pair as a single unit ( |
There was a problem hiding this comment.
Pull request overview
This PR hardens Composition#setSelectedRange so selection boundaries won’t land inside a UTF-16 surrogate pair (e.g., half an emoji), aligning setSelectedRange behavior with existing cursor/selection expansion logic.
Changes:
- Normalize
setSelectedRangeboundaries via UTF-16-aware position translation to avoid splitting surrogate pairs. - Add system tests covering collapsed and expanded selections around surrogate pairs in multiple text positions.
- Register the new system test file in the system test entrypoint.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/trix/models/composition.js | Adjusts setSelectedRange to coerce selection endpoints onto surrogate-pair-safe boundaries. |
| src/test/system/set_selected_range_test.js | Adds coverage for selection normalization behavior around surrogate pairs. |
| src/test/system.js | Includes the new system test module in the test suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Make sure that surrogate pairs are always selected both or neither. | ||
| const correctedSelectedRange = [ | ||
| this.translateUTF16PositionFromOffset(start, 0), | ||
| this.translateUTF16PositionFromOffset(end, 0), |
| assert.selectedRange([ 2, 2 ]) | ||
| }) | ||
|
|
||
| test("collapssed selection in the middle of the surrogate pair", () => { |
| assert.selectedRange([ 5, 5 ]) | ||
| }) | ||
|
|
||
| test("collapssed selection in the middle of the surrogate pair", () => { |
| assert.selectedRange([ 5, 5 ]) | ||
| }) | ||
|
|
||
| test("collapssed selection in the middle of the surrogate pair", () => { |
This is already handled in functions like
expandSelectionInDirectionwhich won't select a half of emoji etc. but always the whole surrogate pair. The solution here is the same, if the cursor is placed in the middle of a surrogate pair, it will be moved to the end of the pair. Similarly a selection will either select both or neither.