Repository navigation
Route macOS editor navigation through sequenced input commands - #6027
shai-almog wants to merge 2 commits into
Conversation
Cloudflare Preview
|
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
|
Compared 206 screenshots: 206 matched. |
|
Compared 170 screenshots: 170 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
|
Compared 185 screenshots: 185 matched. Benchmark ResultsDetailed Performance Metrics
ParparVM vs HotSpot (JDK 25): Windows x64Runner CPU: AMD64 Family 25 Model 17 Stepping 1, AuthenticAMD (baseline Ratios are ParparVM / JDK 25: below 1.00x ParparVM is faster (time) or smaller (RAM). Median of 5 interleaved, paired rounds; every run's output was verified. A ratio more than 15% (time) / 15% (RAM) away from its baseline in
Result: no regression |
|
Compared 185 screenshots: 185 matched. Benchmark ResultsDetailed Performance Metrics
ParparVM vs HotSpot (JDK 25): Windows arm64Runner CPU: ARMv8 (64-bit) Family 8 Model D49 Revision 0, MICROSOFT CORPORATION (baseline Ratios are ParparVM / JDK 25: below 1.00x ParparVM is faster (time) or smaller (RAM). Median of 5 interleaved, paired rounds; every run's output was verified. A ratio more than 15% (time) / 15% (RAM) away from its baseline in
Result: no regression |
|
Compared 185 screenshots: 185 matched. ParparVM vs HotSpot (JDK 25): Linux x64Runner CPU: AMD EPYC 9V45 96-Core Processor (baseline Ratios are ParparVM / JDK 25: below 1.00x ParparVM is faster (time) or smaller (RAM). Median of 5 interleaved, paired rounds; every run's output was verified. A ratio more than 15% (time) / 15% (RAM) away from its baseline in
Result: no regression |
|
Compared 185 screenshots: 185 matched. ParparVM vs HotSpot (JDK 25): Linux arm64Runner CPU: Neoverse-N2 (baseline Ratios are ParparVM / JDK 25: below 1.00x ParparVM is faster (time) or smaller (RAM). Median of 5 interleaved, paired rounds; every run's output was verified. A ratio more than 15% (time) / 15% (RAM) away from its baseline in
Result: no regression |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73d5950071
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Override | ||
| public TextInputState getEditingState() { | ||
| return new TextInputState(doc.getText(), getSelectionStart(), getSelectionEnd(), composingStart, composingEnd); | ||
| return new TextInputState(doc.getText(), anchor < 0 ? caret : anchor, caret, composingStart, composingEnd); |
There was a problem hiding this comment.
When a user creates a backward selection, this now publishes anchor/focus values such as (13, 0) to every platform, but only the macOS bridge was updated for that ordering. The JavaScript bridge writes these directly through setSelectionStart() and setSelectionEnd() (HTML5Implementation.java:4307-4310), which collapses a reversed DOM selection, while Android's deleteSurroundingText() forces the end to be at least the start (CN1TextInputConnection.java:136-140), so soft-keyboard deletion targets the wrong range or does nothing. Preserve the previous ordered state for those ports or update all consumers to normalize while retaining direction before globally emitting anchor/focus.
Useful? React with 👍 / 👎.
| } else if ([name isEqualToString:@"pageUp:"] || [name isEqualToString:@"scrollPageUp:"]) { | ||
| command = 7; | ||
| } else if ([name isEqualToString:@"pageDown:"] || [name isEqualToString:@"scrollPageDown:"]) { | ||
| command = 8; |
There was a problem hiding this comment.
When AppKit emits scrollPageUp: or scrollPageDown:, these branches translate it into KEY_PAGE_UP/DOWN, whose handler calls moveCaret() (EditorView.java:2021-2025) and therefore changes the insertion point and selection. AppKit defines the scrollPage* selectors as scrolling the content rather than editing the selection (Apple NSStandardKeyBindingResponding), so a plain Page Up/Down binding can unexpectedly relocate the caret and subsequent typing instead of only scrolling the viewport. Route these selectors to a scroll-only operation rather than grouping them with caret page commands.
Useful? React with 👍 / 👎.
| if (word && !hasSelection() && editable) { | ||
| replaceRange(wordLeft(), caret, "", true); |
There was a problem hiding this comment.
When Option-Backspace/Delete is used on non-ASCII prose, these new branches call wordLeft()/wordRight(), but isWordChar() at EditorView.java:1748-1749 recognizes only ASCII letters, digits, and underscore. For text containing only Greek, Arabic, or CJK words, whitespace and every letter are all classified identically as non-word characters, so one word-deletion command can consume everything to the document boundary instead of one word. Use Unicode-aware word boundaries before exposing these helpers through AppKit's deleteWord* selectors.
Useful? React with 👍 / 👎.
| if (word && !hasSelection() && editable) { | ||
| replaceRange(wordLeft(), caret, "", true); |
There was a problem hiding this comment.
When Option-Backspace is pressed at offset 0 (or Option-Delete at the document end), wordLeft()/wordRight() returns the current caret and this branch still calls replaceRange() with an empty range. That method unconditionally records an undo entry, clears the redo stack, invokes editorChanged(), and pushes input state even though no text changed, so a boundary keypress can discard valid redo history and leave the next Undo appearing to do nothing. Guard against equal range endpoints before invoking replaceRange().
Useful? React with 👍 / 👎.
1 participant
AppKit word/page navigation was swallowed for
TextInputClienteditors, and line commands used newline boundaries instead of the editor's visual layout. Reversed Java selections also collapsed when copied into AppKit, so later native character movement or typing could target the wrong range.Route word movement/deletion, visual line/paragraph/document boundaries, and page/scroll-page selectors through the existing sequenced editor-command dispatcher. Preserve anchor/focus direction alongside AppKit's ordered range, including transitions between semantic word movement and native character movement. Handle AppKit paragraph selectors and the forward/backward steps in its Option-Up/Down key bindings. Update
EditorViewparagraph/document boundaries and word-deletion handling and document the contract in the developer guide. Legacy TextArea command handling remains on its existing path.Validation:
These checks do not certify every input method, bidi/grapheme behavior, or all application workflows.