-
Notifications
You must be signed in to change notification settings - Fork 41.7k
Fix typing over RTL selection requiring two keypresses #331077
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -737,11 +737,11 @@ export class TextAreaWrapper extends Disposable implements ICompleteTextAreaWrap | |
| } | ||
|
|
||
| public getSelectionStart(): number { | ||
| return this._actual.selectionDirection === 'backward' ? this._actual.selectionEnd : this._actual.selectionStart; | ||
| return this._actual.selectionStart; | ||
| } | ||
|
|
||
| public getSelectionEnd(): number { | ||
| return this._actual.selectionDirection === 'backward' ? this._actual.selectionStart : this._actual.selectionEnd; | ||
| return this._actual.selectionEnd; | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is the correct fix for this input pipeline. TextAreaState and the input diffing engine (deduceInput) assume standard, normalized selection boundaries (selectionStart <= selectionEnd). Standard browsers also do not support setting a selection range where selectionStart > selectionEnd (doing so collapses the selection). Swapping selectionStart and selectionEnd for backward/RTL selections violates these assumptions, leading to a state mismatch where the native selection is collapsed and replacePrevCharCnt is incorrectly computed as 1 instead of 0 on the first keypress. Since the hidden textarea is only used for IME/input bridging and not direct user selection interaction, returning normalized boundaries here is safe and correctly aligns the native state with TextAreaState expectations, solving the double-keypress bug without breaking caret logic. |
||
| } | ||
|
|
||
| public setSelectionRange(reason: string, selectionStart: number, selectionEnd: number): void { | ||
|
|
||
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 change is the correct fix for this input pipeline.
TextAreaState and the input diffing engine (deduceInput) assume standard, normalized selection boundaries (selectionStart <= selectionEnd). Standard browsers also do not support setting a selection range where selectionStart > selectionEnd (doing so collapses the selection).
Swapping selectionStart and selectionEnd for backward/RTL selections violates these assumptions, leading to a state mismatch where the native selection is collapsed and replacePrevCharCnt is incorrectly computed as 1 instead of 0 on the first keypress.
Since the hidden textarea is only used for IME/input bridging and not direct user selection interaction, returning normalized boundaries here is safe and correctly aligns the native state with TextAreaState expectations, solving the double-keypress bug without breaking caret logic.