Fix #247141: Transform actions fail on text selected in F&R widget#307748
Fix #247141: Transform actions fail on text selected in F&R widget#307748vascoreino wants to merge 1 commit intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Fixes #247141 where “Transform to …” case actions invoked from the Command Palette do not apply to text selected in the Find/Replace widget input, by routing transform actions to the last-focused Find/Replace input when appropriate.
Changes:
- Add a small
IFindInputTransformerhook on the find controller so other editor actions can transform the find widget’s selected text. - Track the last-focused input element in
FindWidgetand provide a method to transform its current selection and sync state. - Update
AbstractCaseActionto attempt find-widget selection transforms before falling back to editor buffer transforms; add unit tests for the helper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/editor/contrib/linesOperations/browser/linesOperations.ts | Case transform actions now delegate to the find controller when a find-widget selection should be transformed. |
| src/vs/editor/contrib/find/browser/findWidget.ts | Tracks last-focused input and adds selection-transform helper + state synchronization. |
| src/vs/editor/contrib/find/browser/findController.ts | Exposes a transformFocusedInput capability from the controller to the editor action. |
| src/vs/editor/contrib/find/browser/findCommon.ts | New shared constant/interface for finding the controller contribution and invoking transforms. |
| src/vs/editor/contrib/find/test/browser/findController.test.ts | Adds tests for the selection-transform helper and controller behavior when the widget is not created. |
| public transformFocusedInputSelection(transform: (text: string) => string): boolean { | ||
| const el = this._lastFocusedInput; | ||
| if (!el) { | ||
| return false; | ||
| } | ||
| const transformed = transformInputSelection(el, transform); | ||
| if (transformed) { | ||
| if (el === this._findInput.inputBox.inputElement) { | ||
| this._state.change({ searchString: el.value }, true); | ||
| } else { | ||
| this._state.change({ replaceString: el.value }, false); | ||
| } | ||
| } | ||
| return transformed; | ||
| } |
There was a problem hiding this comment.
transformFocusedInputSelection can still transform the Find/Replace input even when the widget is hidden (or after the editor regains focus), because _lastFocusedInput is retained indefinitely and there’s no _isVisible/state guard. This can cause Transform Case commands to unexpectedly operate on the (hidden) find widget instead of the editor. Consider returning false when the widget isn’t revealed, and clearing _lastFocusedInput when the widget hides and/or when the editor widget receives focus so editor transforms aren’t hijacked by a stale input selection.
| const start = el.selectionStart ?? 0; | ||
| const end = el.selectionEnd ?? 0; | ||
| if (start === end) { |
There was a problem hiding this comment.
transformInputSelection uses ?? 0 for selectionStart/selectionEnd. For input types that don’t support selection those properties can be null, and defaulting to 0 risks treating a partially-null selection as a real range. Prefer an explicit null check (return false if either is null) before comparing start === end.
| const start = el.selectionStart ?? 0; | |
| const end = el.selectionEnd ?? 0; | |
| if (start === end) { | |
| const start = el.selectionStart; | |
| const end = el.selectionEnd; | |
| if (start === null || end === null || start === end) { |
| input.value = 'hello world'; | ||
|
|
||
| const result = transformInputSelection(input, s => s.toLocaleUpperCase()); | ||
|
|
||
| assert.strictEqual(result, false); | ||
| assert.strictEqual(input.value, 'hello world'); |
There was a problem hiding this comment.
The test labeled “selection is null” doesn’t actually set up a null selection (a default text <input> typically reports selectionStart/selectionEnd as 0, not null). To validate the intended branch, consider forcing an input type that doesn’t support selection (where these properties are null) and asserting the precondition before calling transformInputSelection.
| input.value = 'hello world'; | |
| const result = transformInputSelection(input, s => s.toLocaleUpperCase()); | |
| assert.strictEqual(result, false); | |
| assert.strictEqual(input.value, 'hello world'); | |
| input.type = 'color'; | |
| input.value = '#000000'; | |
| assert.deepStrictEqual({ selectionStart: input.selectionStart, selectionEnd: input.selectionEnd }, { selectionStart: null, selectionEnd: null }); | |
| const result = transformInputSelection(input, s => s.toLocaleUpperCase()); | |
| assert.strictEqual(result, false); | |
| assert.strictEqual(input.value, '#000000'); |
| const findController = editor.getContribution(FIND_CONTROLLER_ID) as IFindInputTransformer | null; | ||
| const findWordSeparators = editor.getOption(EditorOption.wordSeparators); | ||
| if (findController?.transformFocusedInput(text => this._modifyText(text, findWordSeparators))) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
AbstractCaseAction.run now has a new early-return path that delegates to the find controller (transformFocusedInput). There are existing unit tests for case transform actions in linesOperations.test.ts, but none appear to exercise this new branch. Adding a test with a stub contribution registered under editor.contrib.findController that returns true (and verifies the editor model is unchanged) would help prevent regressions.
…widget Transform to Uppercase/Lowercase/etc. had no effect when invoked from the command palette with text selected inside the Find & Replace widget inputs. The commands only operated on the editor buffer, unaware of the find/replace DOM inputs. Fixed by adding IFindInputTransformer in findCommon.ts. AbstractCaseAction now checks if a find/replace input was last focused and delegates the transform there, falling back to the editor buffer otherwise.
vascoreino
force-pushed
the
fix/issue-247141/find-widget-transform-actions
branch
from
26787ba to
f412b84
Compare
Fixes #247141
When text is selected inside the Find & Replace widget and a transform action is run from the command palette, nothing happens.
This happens because opening the command palette moves focus away from the find input, while the browser still preserves the selection. As a result, AbstractCaseAction operates on the editor instead of the find input.
This change tracks the last focused input in FindWidget. AbstractCaseAction now checks this state and applies the transform to the find input when appropriate.
How to test