cg rich text editor from ground up#557
Merged
softmarshmallow merged 15 commits intomainfrom Mar 7, 2026
Merged
Conversation
- Introduced the `AttributedText` module to support a run-based rich text data model, allowing for mixed-style text editing. - Enhanced the `TextEditor` to manage rich text snapshots for undo/redo functionality, capturing both text and style states. - Updated the Skia layout engine to handle per-run styles and variable font features, improving rendering capabilities for rich text. - Added rich text shortcuts and documentation for the new features, enhancing user experience and editor functionality. This update aims to provide a robust foundation for rich text editing in the grida-text-edit project.
- Updated the `TextEditor` to support rich text editing with snapshots that capture both text and style states for undo/redo functionality. - Introduced `GenericEditHistory` to manage a more flexible history system, allowing for style-only changes and improved state management. - Enhanced clipboard operations to preserve formatting during copy/paste actions, improving user experience. - Removed the deprecated `AttributedText` module, streamlining the codebase for better maintainability. This update aims to provide a more robust and user-friendly rich text editing experience in the grida-text-edit project.
- Introduced `html.rs` to handle serialization of styled text to HTML and deserialization from HTML back to styled text. - Implemented support for inline styles and basic HTML tags such as `<b>`, `<i>`, `<u>`, and `<s>`, enhancing rich text editing capabilities. - Updated the `AttributedText` module to facilitate rich text operations with HTML, improving clipboard functionality and user experience. This update aims to provide a seamless integration of HTML with the rich text editing features in the grida-text-edit project.
- Introduced Inconsolata and Lora as variable fonts, including their respective TTF files for both regular and italic styles. - Added comprehensive README and OFL license files for both fonts, detailing usage instructions and licensing terms. - Updated the fonts table in the README.md to include new entries for Inconsolata and Lora, enhancing the available font options for users. This update aims to expand the font offerings in the project, providing more flexibility and options for typography in web applications.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
WalkthroughAdds a full attributed rich-text model, HTML import/export, snapshot-based undo/redo, per-block Skia layout with per-run styling and variable-font support, editor integrations (formatting, paste/drag-drop), and performance/layout docs and fixtures. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Editor as TextEditor
participant History as GenericEditHistory
participant Snapshot as RichTextSnapshot
participant Content as AttributedText
participant Layout as SkiaLayoutEngine
participant Canvas as Renderer
User->>Editor: apply(EditingCommand)
Editor->>Snapshot: snapshot() -- capture state + content
Editor->>History: push(snapshot_before, kind)
Editor->>Editor: apply command to plain state (produces EditDelta)
Editor->>Content: sync_content_with_delta(delta)
Content->>Content: modify runs (insert/delete/apply_style)
Editor->>Layout: ensure_layout_attributed(content)
Layout->>Layout: rebuild affected ParaBlock(s)
Layout->>Canvas: paint_paragraph_at(...) -- per-run styling rendered
User->>Editor: undo()
Editor->>History: undo(current_snapshot)
History->>Editor: return previous Snapshot
Editor->>Editor: restore(snapshot)
Editor->>Layout: invalidate/rebuild
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Added a new document `impl-performance.md` detailing the performance model for text editing, focusing on viewport-based rendering and efficient text buffer representation. - Updated `attributed-text.md` to include a reference to the new performance model, enhancing the documentation's comprehensiveness. - The performance model outlines strategies for maintaining responsiveness in text editing applications, addressing critical paths such as layout, rendering, and offset conversion. This update aims to provide a clear framework for optimizing text editing performance in the grida-text-edit project.
- Implemented function key presets for quick text color and font family changes in the text editor. - Added functionality for F1, F2, and F3 to set text color to black, red, and blue respectively. - Introduced F5, F6, and F7 to switch between the Inter, Lora, and Inconsolata font families. - Enhanced the demo text to include instructions for using these new features, improving the user experience for developers. This update aims to facilitate rapid styling adjustments during development, enhancing the text editing capabilities in the grida-text-edit project.
- Introduced a new `ParaBlock` struct to manage individual paragraph layouts, improving text rendering efficiency. - Updated the `SkiaLayoutEngine` to support a per-block architecture, allowing for selective rebuilding of affected text blocks on edits. - Enhanced the `ensure_layout` and `rebuild_blocks` methods to handle text splitting and layout caching, optimizing performance during text updates. This update aims to enhance the text layout capabilities in the grida-text-edit project, providing a more efficient and responsive editing experience.
- Added vertical scrolling functionality to the text editor, allowing users to navigate through content that exceeds the viewport height. - Introduced a `scroll_y` property to manage the vertical scroll offset and implemented methods for scrolling and ensuring the cursor remains visible. - Updated the rendering logic to account for the scroll offset, ensuring proper display of text within the visible area. - Enhanced documentation to outline the new scrolling behavior and its integration with the text editing engine. This update aims to improve the user experience by providing a seamless scrolling mechanism in the grida-text-edit project.
- Introduced a caching mechanism for the caret rectangle in the `TextEditor`, reducing redundant computations during rendering. - Updated the `apply_command` function to invalidate the cache when text or cursor changes occur, ensuring accurate caret positioning. - Enhanced the `SkiaLayoutEngine` to support incremental layout updates, allowing for efficient re-layout of only affected text blocks on edits. - Improved the `line_index_for_offset` function to utilize binary search for better performance in locating line indices. This update aims to enhance the responsiveness and efficiency of the text editing experience in the grida-text-edit project.
- Updated the text editor to utilize per-block layout for rich text, enhancing rendering efficiency. - Refined selection handling by implementing `selection_rects_for_range`, simplifying the selection rectangle calculations. - Adjusted cursor positioning logic to reset the cursor to the start after content updates. - Enhanced the Skia layout engine with incremental UTF-16 to UTF-8 offset conversion, optimizing performance during text rendering. This update aims to streamline text selection and layout processes, improving overall performance and user experience in the grida-text-edit project.
…ation - Introduced `EditDelta` struct to encapsulate text mutation details, optimizing the `apply_command_mut` function for better performance and clarity. - Updated the `TextEditor` to utilize the new delta structure for efficient content synchronization after edits, replacing the previous text diffing approach. - Enhanced the `GenericEditHistory` with merge detection to avoid unnecessary snapshot creation, improving history management efficiency. - Refined layout handling in the `SkiaLayoutEngine` to ensure accurate updates for attributed text, preventing layout inconsistencies during rich text editing. This update aims to streamline command processing and layout synchronization, enhancing the overall performance and user experience in the grida-text-edit project.
- Introduced new text fixture files: `ascii.txt`, `bidi.txt`, and `cjk.txt` for testing layout and rendering. - `ascii.txt` contains printable ASCII characters and punctuation. - `bidi.txt` includes bidirectional text examples in Arabic and Hebrew, along with embedded LTR/RTL content. - `cjk.txt` features Chinese, Japanese, and Korean text samples, addressing layout and rendering requirements for CJK scripts. - Updated README.md to document the new text fixtures and their purposes. This update enhances the testing capabilities for diverse text layouts in the grida-text-edit project.
- Introduced a new `left` field in `LineMetrics` to represent the X offset of the line's left edge in layout-local space, improving text alignment handling. - Updated the `SimpleLayoutEngine` to initialize the `left` value to `0.0`. - Enhanced the `SkiaLayoutEngine` to calculate the `left` offset based on text alignment for empty lines, ensuring accurate layout metrics during rendering. - Added a new method `empty_line_left` in the `TextAlign` enum to determine the left offset for empty lines based on the layout width. This update aims to improve text alignment and layout accuracy in the grida-text-edit project.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| self.caret_style_override = None; | ||
| self.layout.invalidate(); | ||
| self.reset_blink(); | ||
| } |
There was a problem hiding this comment.
paste_attributed missing layout rebuild and auto-scroll
Medium Severity
paste_attributed calls self.layout.invalidate() and self.reset_blink() but, unlike apply and apply_with_kind, omits the calls to self.layout.ensure_layout_attributed(&self.content) and self.ensure_cursor_visible(). This means after pasting HTML content via Cmd+V, the layout isn't rebuilt before the next frame and the view won't auto-scroll to keep the cursor visible. The doc comment on ensure_cursor_visible explicitly warns that ensure_layout_attributed must be called first.
Additional Locations (1)
![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: 0652bb4935
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/grida-text-edit/examples/wd_text_editor.rs (1)
821-849:⚠️ Potential issue | 🟠 MajorClear
caret_style_overrideon direct mouse cursor moves.Lines 824-826 and 838-847 mutate
state.cursordirectly, so they bypass theapply()path that clears the typing-style override on cursor movement. After toggling bold/italic with no selection, a click elsewhere will keep the old override alive and the next insertion gets the wrong style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/examples/wd_text_editor.rs` around lines 821 - 849, The mouse handlers on_mouse_down and on_mouse_move directly set self.state.cursor (and self.state.anchor) which bypasses the normal apply() path that clears the typing-style override; update both handlers to clear the caret_style_override (e.g., set self.state.caret_style_override = None or call the same helper used by apply()) whenever the cursor moves due to a direct mouse action so toggled styles (bold/italic) are not preserved for the next insertion; ensure you clear the override in all branches where self.state.cursor is changed (including when drag_anchor_utf8 is set) so behavior matches apply().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-text-edit/examples/wd_text_editor.rs`:
- Around line 1639-1646: The paste path calls html_to_attributed_text on
untrusted clipboard HTML (triggered in the PhysicalKey::Code(KeyCode::KeyV)
branch after clipboard.get().html()) which can produce invalid runs that cause
AttributedText::from_parts to panic; modify the code to call
html_to_attributed_text defensively (e.g., return Result from
html_to_attributed_text or validate its output) and handle failures by falling
back to plain text from clipboard.get_text() or by ignoring the HTML paste, then
call inner.editor.paste_attributed only on a successfully validated
AttributedText; apply the same defensive validation to the dropped-file HTML
handling path so malformed HTML cannot panic the app.
- Around line 451-489: The history is being recorded before we know if the edit
actually changed the document; move the history push/merge to after calling
apply_command_mut and only record an undo entry when the edit produced a
mutation (i.e. delta.is_some()). Concretely, in both apply and apply_with_kind
call apply_command_mut(&mut self.state, cmd, &mut self.layout) first, capture
the returned delta, then if let Some(d) = delta { if
!self.history.would_merge(kind) { self.history.push(&self.snapshot(), kind); }
else { self.history.push_merge(kind); } self.sync_content_with_delta(&d,
old_cursor); } (and keep the existing caret/cache/blink/layout code) so
history.push/push_merge and sync_content_with_delta only run when a real
mutation occurred; use the same change for both apply (determine kind from
cmd.edit_kind()) and apply_with_kind.
- Around line 1818-1853: When replacing the whole document in the drop handlers
for the .txt and .html branches, also reset the editor undo/redo history so a
subsequent Undo doesn't resurrect the previous document; locate the places
updating inner.editor.content and inner.editor.state (inside the Some("txt")
Ok(content) block and the Some("html" | "htm") Ok(html) block) and clear or
reinitialize inner.editor.history (or call the editor's history reset/clear
method) immediately after setting the new content/state and before requesting a
redraw.
In `@crates/grida-text-edit/src/attributed_text/html.rs`:
- Around line 36-37: The code diffs runs against at.default_style() but never
outputs that base style, so the fragment isn't self-contained; modify the
function that builds the html String (the block initializing let default =
at.default_style(); and let mut html = ...) to emit a wrapping base <span> (or
equivalent container) with the serialized default style attributes before
emitting run-level spans, and close that wrapper at the end; reuse the existing
style serialization logic used for per-run diffs (and apply the same fix in the
other similar block around lines 53-64) so consumers receive the base
font-family/size/color even when runs match the default.
- Around line 247-282: The parser can hang when read_ident() returns an empty
string for comments/directives/namespaced tags (e.g. <!--...-->, <!doctype ...>,
<o:p>) because pos never advances; to fix, after reading tag_name in the
tag-parsing path check for an empty or special-starting name
(tag_name.is_empty() or tag_name.starts_with('!') or tag_name.starts_with('?')
or tag_name.contains(':')) and call skip_to_tag_end() and return so the parser
advances; likewise inside the attribute parsing loop, if read_ident() yields an
empty attr_name then call skip_to_tag_end() and break the loop (or advance pos
to '>') to avoid the infinite loop — update the code paths around read_ident(),
skip_to_tag_end(), and the attribute loop (where attr_name, read_attr_value(),
pos and input are used) accordingly.
In `@crates/grida-text-edit/src/attributed_text/mod.rs`:
- Around line 816-840: The code calls split_run_at with raw byte offsets
(lo32/hi32) which may not be UTF-8 char boundaries, allowing invalid StyledRun
boundaries; before calling split_run_at in apply_style (and similarly in
set_style via this path) validate the offsets using self.text.is_char_boundary
or adjust them to the nearest valid boundary (or bail out) so you only pass
valid char-boundary byte indices to split_run_at; ensure you check the adjusted
lo/hi after clamping to self.text.len() and return early if they become
empty/invalid, then proceed to split_run_at and apply the style.
In `@crates/grida-text-edit/src/skia_layout.rs`:
- Around line 524-560: The last_affected calculation misses the case where
deleting a newline should merge the following block (e.g., delete between
"A\nB"); after computing last_affected via position(|b| b.byte_end >=
old_edit_end), detect when newlines_removed > 0 and there is a block whose
byte_start == old_edit_end (in the old coordinates) and set last_affected to
that block's index instead so the following paragraph is included in the
rebuild; update the logic around last_affected (and any uses of old_rebuild_end)
to use this adjusted index so merged paragraphs are re-parsed into blocks.
- Around line 1362-1371: The code treats lines with a single UTF-16 code unit as
empty because the condition lm.end_index.saturating_sub(lm.start_index) <= 1
matches length 1; change that test to only treat zero-length lines as empty
(i.e., compare for == 0 or use .is_empty() equivalent) so single-code-unit lines
(e.g., "A") will fall through and use Skia's hit-test result; update the branch
that returns block.byte_start + utf16_to_utf8_offset(slice,
lm.start_index).min(slice.len()) accordingly and keep references to
paragraph.get_line_metrics, lm.start_index/end_index, utf16_to_utf8_offset, and
block.byte_start.
In `@docs/wg/feat-text-editing/attributed-text.md`:
- Line 650: The markdown math uses underscores that trigger emphasis parsing in
"500 _ 4 = 2000" and "5 _ 128 = 640"; replace those underscores with a
multiplication symbol (×) or asterisk (*) in the sentence comparing Figma and
our model so the formulas read "500 × 4 = 2000" and "5 × 128 = 640" (or use "*"
as "500 * 4 = 2000" and "5 * 128 = 640") to prevent markdown emphasis and
satisfy markdownlint.
- Line 96: Several fenced code blocks in attributed-text.md are missing language
tags (plain ```), causing markdownlint warnings; update each triple-backtick
fence at the noted locations (including the fences at lines referenced by the
reviewer) to include the appropriate language identifier (e.g., ```text,
```rust, or ```fbs) so the linter stops flagging them and docs rendering
improves; search for the plain ``` fences in the file and replace them with the
correct labeled fences based on the code snippet content.
In `@docs/wg/feat-text-editing/index.md`:
- Around line 333-340: The docs wrongly list viewport_height as an
engine-provided requirement; update the Engine requirements paragraph to keep
only geometry-providing symbols (line_metrics, caret_rect_at) under engine
responsibilities and remove viewport_height from that list, then add a sentence
indicating that the host must supply viewport_height when computing
PageUp/PageDown or performing scroll adjustments so the host-side contract is
clear.
In `@fixtures/fonts/Inconsolata/README.txt`:
- Around line 4-87: The Inconsolata README lists many static files that
contradict fixtures/fonts/README.md (which documents Inconsolata as
variable-only) and likely references stale static paths like the numerous
static/Inconsolata_*.ttf entries and Inconsolata-VariableFont_wdth,wght.ttf; fix
by either trimming README.txt to only list the actual checked-in artifact(s)
(e.g., keep Inconsolata-VariableFont_wdth,wght.ttf and any real static files) or
explicitly mark the file as "upstream package documentation" and remove or
annotate all stale static/... entries so readers aren’t pointed to non-shipped
paths—update README.txt accordingly to match the truth in
fixtures/fonts/README.md.
In `@fixtures/text/cjk.txt`:
- Around line 12-13: The Korean paragraph contains a fullwidth Chinese comma
(the character after "구성되며，각") which mixes punctuation types; update that string
by replacing the fullwidth comma (U+FF0C) with the appropriate Korean/ASCII
comma (replace "구성되며，각" with "구성되며, 각") so the fixture uses consistent Korean
punctuation and preserves correct line-break/fallback behavior.
---
Outside diff comments:
In `@crates/grida-text-edit/examples/wd_text_editor.rs`:
- Around line 821-849: The mouse handlers on_mouse_down and on_mouse_move
directly set self.state.cursor (and self.state.anchor) which bypasses the normal
apply() path that clears the typing-style override; update both handlers to
clear the caret_style_override (e.g., set self.state.caret_style_override = None
or call the same helper used by apply()) whenever the cursor moves due to a
direct mouse action so toggled styles (bold/italic) are not preserved for the
next insertion; ensure you clear the override in all branches where
self.state.cursor is changed (including when drag_anchor_utf8 is set) so
behavior matches apply().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4b8af4d-b3ab-4852-8ef8-5adc13ad35ff
⛔ Files ignored due to path filters (3)
fixtures/fonts/Inconsolata/Inconsolata-VariableFont_wdth,wght.ttfis excluded by!**/*.ttffixtures/fonts/Lora/Lora-Italic-VariableFont_wght.ttfis excluded by!**/*.ttffixtures/fonts/Lora/Lora-VariableFont_wght.ttfis excluded by!**/*.ttf
📒 Files selected for processing (22)
crates/grida-text-edit/examples/wd_text_editor.rscrates/grida-text-edit/src/attributed_text/html.rscrates/grida-text-edit/src/attributed_text/mod.rscrates/grida-text-edit/src/history.rscrates/grida-text-edit/src/layout.rscrates/grida-text-edit/src/lib.rscrates/grida-text-edit/src/simple_layout.rscrates/grida-text-edit/src/skia_layout.rsdocs/wg/feat-text-editing/attributed-text.mddocs/wg/feat-text-editing/impl-performance.mddocs/wg/feat-text-editing/index.mdfixtures/fonts/Inconsolata/OFL.txtfixtures/fonts/Inconsolata/README.txtfixtures/fonts/Lora/OFL.txtfixtures/fonts/Lora/README.txtfixtures/fonts/README.mdfixtures/text/README.mdfixtures/text/ascii.txtfixtures/text/bidi.txtfixtures/text/cjk.txtfixtures/text/hamlet.txtfixtures/text/lorem.txt
Comment on lines
+36
to
+37
| let default = at.default_style(); | ||
| let mut html = String::with_capacity((hi - lo) * 2); |
There was a problem hiding this comment.
Serialize the base style too, or the fragment is not self-contained.
This function diffs each run against at.default_style(), but it never emits that base style anywhere. Any paste into a destination with a different base style—or any external HTML consumer—will lose source formatting that matched the original default (font-family, font-size, color, etc.). Wrap the whole fragment in a base <span> or diff against a fixed HTML baseline instead.
Also applies to: 53-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-text-edit/src/attributed_text/html.rs` around lines 36 - 37, The
code diffs runs against at.default_style() but never outputs that base style, so
the fragment isn't self-contained; modify the function that builds the html
String (the block initializing let default = at.default_style(); and let mut
html = ...) to emit a wrapping base <span> (or equivalent container) with the
serialized default style attributes before emitting run-level spans, and close
that wrapper at the end; reuse the existing style serialization logic used for
per-run diffs (and apply the same fix in the other similar block around lines
53-64) so consumers receive the base font-family/size/color even when runs match
the default.
Comment on lines
+58
to
+60
| html.push_str("<span style=\""); | ||
| html.push_str(&css); | ||
| html.push_str("\">"); |
There was a problem hiding this comment.
Escape CSS before inserting it into style="...".
font_family is interpolated directly into CSS and the CSS string is written verbatim into the HTML attribute. A family name containing " or ; breaks the markup and can inject extra CSS or attributes into the fragment. HTML-escape the attribute value and CSS-escape quoted family names first.
🔒 Suggested direction
- parts.push(format!("font-family:\"{}\"", style.font_family));
+ parts.push(format!("font-family:\"{}\"", css_escape_string(&style.font_family)));
@@
- html.push_str(&css);
+ html_attr_escape_into(&mut html, &css);Also applies to: 81-83
Comment on lines
+333
to
+340
| ### Engine requirements | ||
|
|
||
| The engine provides the information the host needs to implement scroll: | ||
|
|
||
| - **`line_metrics`**: per-line baselines, ascent, and descent — sufficient to compute total content height and determine which lines are visible for a given scroll offset. | ||
| - **`caret_rect_at`**: the caret's position in layout-local space — the host uses this to decide whether to auto-scroll. | ||
| - **`viewport_height`**: used by PageUp/PageDown to compute the jump distance in lines. | ||
|
|
There was a problem hiding this comment.
Keep viewport_height on the host side.
This section says scroll is entirely host-owned, but Line 339 lists viewport_height as an engine requirement. That inverts the contract and makes the API boundary inconsistent in the same section. The engine should expose geometry (line_metrics, caret_rect_at); the host should supply viewport_height when computing PageUp/PageDown or scroll adjustments.
Suggested edit
### Engine requirements
The engine provides the information the host needs to implement scroll:
- **`line_metrics`**: per-line baselines, ascent, and descent — sufficient to compute total content height and determine which lines are visible for a given scroll offset.
- **`caret_rect_at`**: the caret's position in layout-local space — the host uses this to decide whether to auto-scroll.
-- **`viewport_height`**: used by PageUp/PageDown to compute the jump distance in lines.
+
+The host provides **`viewport_height`** when applying PageUp/PageDown or deciding how far to scroll after caret movement.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Engine requirements | |
| The engine provides the information the host needs to implement scroll: | |
| - **`line_metrics`**: per-line baselines, ascent, and descent — sufficient to compute total content height and determine which lines are visible for a given scroll offset. | |
| - **`caret_rect_at`**: the caret's position in layout-local space — the host uses this to decide whether to auto-scroll. | |
| - **`viewport_height`**: used by PageUp/PageDown to compute the jump distance in lines. | |
| ### Engine requirements | |
| The engine provides the information the host needs to implement scroll: | |
| - **`line_metrics`**: per-line baselines, ascent, and descent — sufficient to compute total content height and determine which lines are visible for a given scroll offset. | |
| - **`caret_rect_at`**: the caret's position in layout-local space — the host uses this to decide whether to auto-scroll. | |
| The host provides **`viewport_height`** when applying PageUp/PageDown or deciding how far to scroll after caret movement. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/feat-text-editing/index.md` around lines 333 - 340, The docs wrongly
list viewport_height as an engine-provided requirement; update the Engine
requirements paragraph to keep only geometry-providing symbols (line_metrics,
caret_rect_at) under engine responsibilities and remove viewport_height from
that list, then add a sentence indicating that the host must supply
viewport_height when computing PageUp/PageDown or performing scroll adjustments
so the host-side contract is clear.
- Added support for ignoring AI-related markdown files in `.gitignore`. - Refactored the `apply` and `apply_with_kind` methods in `TextEditor` to improve history management during text edits, ensuring accurate snapshot handling. - Enhanced the HTML parsing logic to handle malformed HTML more gracefully and updated tests to reflect these changes. - Introduced a new `generation` field in `AttributedText` to track mutations, optimizing layout updates in the `SkiaLayoutEngine`. These changes aim to improve the overall functionality and robustness of the text editing experience in the grida-text-edit project.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
|
|
||
| self.style_stack.push(new_style); | ||
| } |
There was a problem hiding this comment.
HTML parser void elements misalign the style stack
High Severity
The HTML parser only handles <br> as a void/self-closing element. Other void elements like <img>, <hr>, <meta>, <wbr> push a style onto style_stack but never get a matching closing tag to pop it. This misaligns the stack so that subsequent closing tags pop the wrong entry. For example, in <b>Hello<img>World</b>Normal, the <img> pushes an extra style, so </b> only pops the extra entry — "Normal" remains incorrectly bold. Real clipboard HTML from browsers commonly contains void elements like <meta> in the preamble.
Additional Locations (1)
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Dropped file doesn't clear undo history stack
Low Severity
The DroppedFile handler for both .txt and .html files replaces the entire document content, cursor, and scroll state, but doesn't call inner.editor.history.clear(). The stale undo stack still contains snapshots from the previous document, so pressing Cmd+Z after loading a file restores unrelated content.
| self.state.cursor = pos; | ||
| self.state.anchor = None; | ||
| self.drag_anchor_utf8 = Some(pos); | ||
| self.invalidate_caret_cache(); |
There was a problem hiding this comment.
Mouse click doesn't clear stale caret style override
Medium Severity
on_mouse_down and on_mouse_move directly set self.state.cursor without clearing caret_style_override. The field's doc comment says "Cleared on cursor movement," and all paths through apply/apply_with_kind do clear it, but these mouse handlers bypass apply. If the user presses Cmd+B (setting a bold override), then clicks elsewhere, and types, the typed text inherits the stale bold override instead of the style at the new cursor position.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/grida-text-edit/examples/wd_text_editor.rs (1)
842-870:⚠️ Potential issue | 🟠 MajorClear
caret_style_overrideon mouse-driven caret moves too.Keyboard navigation already clears the override in
apply(), but click/drag updatesstate.cursordirectly and keeps the old override alive. After toggling a caret style and clicking elsewhere, the next insertion can pick up the previous location’s style.Proposed fix
fn on_mouse_down(&mut self, x: f32, y: f32) { self.mouse_down = true; let pos = self.layout.position_at_point(&self.state.text, x, y); self.state.cursor = pos; self.state.anchor = None; + self.caret_style_override = None; self.drag_anchor_utf8 = Some(pos); self.invalidate_caret_cache(); self.reset_blink(); } fn on_mouse_move(&mut self, x: f32, y: f32) { if !self.mouse_down { return; } + let old_cursor = self.state.cursor; let pos = self.layout.position_at_point(&self.state.text, x, y); if let Some(anchor) = self.drag_anchor_utf8 { if pos != anchor { self.state.anchor = Some(anchor); self.state.cursor = pos; @@ } else { self.drag_anchor_utf8 = Some(pos); self.state.cursor = pos; } + if self.state.cursor != old_cursor { + self.caret_style_override = None; + } self.invalidate_caret_cache(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/examples/wd_text_editor.rs` around lines 842 - 870, Mouse-driven caret updates don't clear the lingering caret_style_override, so add a clear of self.caret_style_override (set to None) whenever you update the caret from mouse events: in on_mouse_down after you set self.state.cursor (and before invalidate_caret_cache/reset_blink), and in on_mouse_move in the branches where you set self.state.cursor / self.state.anchor (and where you initialize drag_anchor_utf8). Reference the methods on_mouse_down and on_mouse_move and the fields caret_style_override, state.cursor, state.anchor, and drag_anchor_utf8 when making the change.
♻️ Duplicate comments (4)
crates/grida-text-edit/src/skia_layout.rs (1)
1383-1392:⚠️ Potential issue | 🟠 MajorDon't treat one-code-unit lines as empty.
<= 1also matches a real one-code-unit line like"A", so point hits on that line snap to the line start instead of using Skia's hit-test result.💡 Minimal fix
- if lm.end_index.saturating_sub(lm.start_index) <= 1 { + if lm.end_index == lm.start_index {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/src/skia_layout.rs` around lines 1383 - 1392, The current check treats one-code-unit lines as empty because it uses lm.end_index.saturating_sub(lm.start_index) <= 1; change the condition to only match truly empty lines (zero length). In the block that iterates block.paragraph.get_line_metrics() replace the length check so it only triggers when lm.end_index - lm.start_index == 0 (i.e., zero-length line) before returning the byte offset via utf16_to_utf8_offset(slice, lm.start_index) and adding block.byte_start, ensuring one-code-unit lines (e.g., "A") fall through to Skia's hit-test.crates/grida-text-edit/src/attributed_text/html.rs (2)
275-319:⚠️ Potential issue | 🟠 MajorUnsupported tags still desynchronize the style stack.
read_ident()stops at:, so markup like<o:p>is not actually skipped as a namespaced tag, and unsupported/void opening tags still push a frame ontostyle_stack. That means a later</span>/</b>can pop the wrong style and restyle the remaining pasted text.Also applies to: 333-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/src/attributed_text/html.rs` around lines 275 - 319, The parser currently treats namespaced tags (like <o:p>) and unsupported/void opening tags as normal tags, which desynchronizes self.style_stack; to fix, detect namespaced tags by checking the raw input char after read_ident (if the next char is ':' then call skip_to_tag_end() and return) and treat void/unsupported opening tags (e.g., "br", "img", "input", any tag where is_block_tag/is_void_tag applies) as non-style-affecting so do not push a new frame onto style_stack; update the logic around tag_name handling (after read_ident() and before pushing to style_stack) to early-return for namespaced tags and to skip pushing a style frame for known void/unsupported tags (use the existing is_block_tag/is_closing checks and the style_stack vector) so closing tags won't pop the wrong frame.
36-64:⚠️ Potential issue | 🟠 MajorEmit the base style so copied HTML stays self-contained.
runs_to_html()diffs every run againstat.default_style(), but the fragment never serializes that base anywhere. Any source formatting that happens to match the default disappears on paste as soon as the destination uses a different base font/color.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/src/attributed_text/html.rs` around lines 36 - 64, The code diffs each run against at.default_style() but never serializes that base, so pasted HTML can lose base font/color; modify runs_to_html (the block building html using at.default_style(), style_to_css(), and html_escape_into) to emit the base style as a wrapper—compute a CSS string for at.default_style() and, if non-empty, prepend "<span style=\"...\">" to html before iterating runs and append the matching "</span>" after, keeping the existing per-run span logic (so runs still only emit diffs) so the fragment is self-contained when pasted.crates/grida-text-edit/examples/wd_text_editor.rs (1)
1852-1886:⚠️ Potential issue | 🟠 MajorClear undo/redo after replacing the whole document.
Both drop handlers overwrite the active document but keep the previous history alive, so the next Undo can jump back into the old file instead of the newly loaded one.
Proposed fix
inner.editor.layout.invalidate(); inner.editor.scroll_y = 0.0; + inner.editor.history.clear(); inner.editor.reset_blink(); eprintln!("loaded plain text: {}", path.display()); inner.window.request_redraw(); @@ inner.editor.layout.invalidate(); inner.editor.scroll_y = 0.0; + inner.editor.history.clear(); inner.editor.reset_blink(); eprintln!("loaded HTML: {}", path.display()); inner.window.request_redraw();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/examples/wd_text_editor.rs` around lines 1852 - 1886, The load handlers replace the whole document but leave prior undo/redo history intact; after successfully assigning inner.editor.content (and updating inner.editor.state, layout, scroll_y, reset_blink, etc.) add a call to clear the editor history — e.g. call inner.editor.clear_history() if that helper exists, or explicitly clear the stacks (inner.editor.history.undo.clear(); inner.editor.history.redo.clear();) — in both the txt and html success blocks, placed after the state resets and before inner.window.request_redraw().
🧹 Nitpick comments (1)
crates/grida-text-edit/src/lib.rs (1)
153-180:word_segment_at()is still a full left-to-right scan.This helper now backs
BackspaceWord/DeleteWord, but it still walkssplit_word_bound_indices()from byte 0. Near the end of long documents those commands remain O(n), which does not match the bounded-window contract in the surrounding comments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-text-edit/src/lib.rs` around lines 153 - 180, word_segment_at currently scans from byte 0; change it to scan a bounded window around offset: pick a WINDOW_SIZE (e.g., 1024), compute window_start = offset.saturating_sub(WINDOW_SIZE) and let slice = &text[window_start..]; iterate slice.split_word_bound_indices(), add window_start to each byte_idx when comparing to offset, and return the adjusted (byte_idx + window_start, end + window_start) when found; if nothing is found in the window (edge cases near document start or end), fall back to the original full scan to preserve correctness. Ensure you update variables used in the function (offset variable, last_start) to account for the windowed indices and keep the original return fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-text-edit/examples/wd_text_editor.rs`:
- Around line 706-740: paste_attributed() mutates text/cursor/layout but skips
the same caret/cache/scroll maintenance that apply() performs, causing stale
cached caret geometry and IME/scroll issues; after you update self.state.text,
self.state.cursor and self.state.anchor, call the same routines apply() uses to
clear/update caret cache and make the caret visible (e.g. invalidate the
cached_caret_rect via the existing invalidation helper or by setting
cached_caret_rect = None, then call the method that ensures the cursor is
visible / updates scroll and IME rect — the same function(s) apply() invokes,
such as make_cursor_visible()/ensure_cursor_visible()/update_ime_rect), keeping
reset_blink() and layout.invalidate() in place.
- Around line 1643-1660: The clipboard HTML writes currently ignore errors so
the plain-text fallback is never written; update both places where
set_html(&html, Some(&plain)) is called (the copy and cut branches using
inner.editor.selection_range(), plain = inner.editor.selected_text(), and html =
runs_to_html(...)) to check the Result and, if set_html returns Err(_), call
self.clipboard.set_text(&plain) as an explicit fallback (and you can ignore or
log both results). Ensure you use the same plain variable for the fallback and
apply this change in both occurrences of self.clipboard.set_html.
In `@crates/grida-text-edit/src/attributed_text/mod.rs`:
- Around line 569-586: default_style_mut and paragraph_style_mut currently allow
callers to change render-affecting state without updating the cache generation
used by ensure_layout_attributed; change these methods so they bump the internal
generation (e.g., self.generation = self.generation.wrapping_add(1) or
equivalent) whenever they mutate default_style or paragraph_style. Additionally,
in default_style_mut handle the empty-text degenerate run case by updating the
run stored in self.runs[0] (or creating/updating the degenerate run) to reflect
the new default_style so the stored run doesn’t remain on the old style. Ensure
you reference the same fields (default_style, paragraph_style, runs, generation)
so cache invalidation and the degenerate run stay consistent with
ensure_layout_attributed.
In `@crates/grida-text-edit/src/skia_layout.rs`:
- Around line 1023-1038: In build_paragraph_for_attributed_slice(), stop seeding
ParagraphStyle and fallback TextStyle from self.config; instead initialize
para_style from the AttributedText instance (use at.paragraph_style() or the
AttributedText API that returns the paragraph-level style) and when building the
no-run fallback TextStyle use the attributed text's default text style (e.g.,
at.default_text_style() or equivalent) so paragraph-level and default run styles
from the content model reach Skia; update the code paths that currently set
para_style, ts, and font families (the para_style variable, the ts TextStyle
construction, and the runs.is_empty() / block_start >= block_content_end branch
that does builder.push_style and builder.add_text) to use the
AttributedText-provided styles instead of self.config.
In `@fixtures/text/editor.txt`:
- Around line 322-323: Replace the incorrect word-joiner (U+2060) sample in the
"Zero width joiner" section — currently shown as the literal line
"wordwordword" — with an actual U+200D character (ZWJ). Use a
shaping-sensitive sequence such as a family emoji (e.g. 👩👩👧👦) or an Indic
conjunct that includes U+200D so the fixture exercises real ZWJ behavior instead
of the word-joiner.
---
Outside diff comments:
In `@crates/grida-text-edit/examples/wd_text_editor.rs`:
- Around line 842-870: Mouse-driven caret updates don't clear the lingering
caret_style_override, so add a clear of self.caret_style_override (set to None)
whenever you update the caret from mouse events: in on_mouse_down after you set
self.state.cursor (and before invalidate_caret_cache/reset_blink), and in
on_mouse_move in the branches where you set self.state.cursor /
self.state.anchor (and where you initialize drag_anchor_utf8). Reference the
methods on_mouse_down and on_mouse_move and the fields caret_style_override,
state.cursor, state.anchor, and drag_anchor_utf8 when making the change.
---
Duplicate comments:
In `@crates/grida-text-edit/examples/wd_text_editor.rs`:
- Around line 1852-1886: The load handlers replace the whole document but leave
prior undo/redo history intact; after successfully assigning
inner.editor.content (and updating inner.editor.state, layout, scroll_y,
reset_blink, etc.) add a call to clear the editor history — e.g. call
inner.editor.clear_history() if that helper exists, or explicitly clear the
stacks (inner.editor.history.undo.clear(); inner.editor.history.redo.clear();) —
in both the txt and html success blocks, placed after the state resets and
before inner.window.request_redraw().
In `@crates/grida-text-edit/src/attributed_text/html.rs`:
- Around line 275-319: The parser currently treats namespaced tags (like <o:p>)
and unsupported/void opening tags as normal tags, which desynchronizes
self.style_stack; to fix, detect namespaced tags by checking the raw input char
after read_ident (if the next char is ':' then call skip_to_tag_end() and
return) and treat void/unsupported opening tags (e.g., "br", "img", "input", any
tag where is_block_tag/is_void_tag applies) as non-style-affecting so do not
push a new frame onto style_stack; update the logic around tag_name handling
(after read_ident() and before pushing to style_stack) to early-return for
namespaced tags and to skip pushing a style frame for known void/unsupported
tags (use the existing is_block_tag/is_closing checks and the style_stack
vector) so closing tags won't pop the wrong frame.
- Around line 36-64: The code diffs each run against at.default_style() but
never serializes that base, so pasted HTML can lose base font/color; modify
runs_to_html (the block building html using at.default_style(), style_to_css(),
and html_escape_into) to emit the base style as a wrapper—compute a CSS string
for at.default_style() and, if non-empty, prepend "<span style=\"...\">" to html
before iterating runs and append the matching "</span>" after, keeping the
existing per-run span logic (so runs still only emit diffs) so the fragment is
self-contained when pasted.
In `@crates/grida-text-edit/src/skia_layout.rs`:
- Around line 1383-1392: The current check treats one-code-unit lines as empty
because it uses lm.end_index.saturating_sub(lm.start_index) <= 1; change the
condition to only match truly empty lines (zero length). In the block that
iterates block.paragraph.get_line_metrics() replace the length check so it only
triggers when lm.end_index - lm.start_index == 0 (i.e., zero-length line) before
returning the byte offset via utf16_to_utf8_offset(slice, lm.start_index) and
adding block.byte_start, ensuring one-code-unit lines (e.g., "A") fall through
to Skia's hit-test.
---
Nitpick comments:
In `@crates/grida-text-edit/src/lib.rs`:
- Around line 153-180: word_segment_at currently scans from byte 0; change it to
scan a bounded window around offset: pick a WINDOW_SIZE (e.g., 1024), compute
window_start = offset.saturating_sub(WINDOW_SIZE) and let slice =
&text[window_start..]; iterate slice.split_word_bound_indices(), add
window_start to each byte_idx when comparing to offset, and return the adjusted
(byte_idx + window_start, end + window_start) when found; if nothing is found in
the window (edge cases near document start or end), fall back to the original
full scan to preserve correctness. Ensure you update variables used in the
function (offset variable, last_start) to account for the windowed indices and
keep the original return fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a041ee29-2412-4a89-bd0d-a1929c392e46
📒 Files selected for processing (10)
.gitignorecrates/grida-text-edit/examples/wd_text_editor.rscrates/grida-text-edit/src/attributed_text/html.rscrates/grida-text-edit/src/attributed_text/mod.rscrates/grida-text-edit/src/lib.rscrates/grida-text-edit/src/skia_layout.rsdocs/wg/feat-text-editing/attributed-text.mdfixtures/text/README.mdfixtures/text/cjk.txtfixtures/text/editor.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- fixtures/text/cjk.txt
- fixtures/text/README.md
Comment on lines
+706
to
+740
| fn paste_attributed(&mut self, pasted: &AttributedText) { | ||
| if pasted.is_empty() { | ||
| return; | ||
| } | ||
| self.history.push(&self.snapshot(), EditKind::Paste); | ||
|
|
||
| // Delete selection if any. | ||
| if let Some((lo, hi)) = self.selection_range() { | ||
| self.content.delete(lo, hi); | ||
| self.state.text = self.content.text().to_owned(); | ||
| self.state.cursor = lo; | ||
| self.state.anchor = None; | ||
| } | ||
|
|
||
| let pos = self.state.cursor; | ||
|
|
||
| // Insert each run from the pasted content with its own style. | ||
| for run in pasted.runs() { | ||
| let start = run.start as usize; | ||
| let end = run.end as usize; | ||
| if start >= end || end > pasted.text().len() { | ||
| continue; | ||
| } | ||
| let slice = &pasted.text()[start..end]; | ||
| let insert_at = pos + start; // offset within the growing text | ||
| self.content.insert_with_style(insert_at, slice, run.style.clone()); | ||
| } | ||
|
|
||
| self.state.text = self.content.text().to_owned(); | ||
| self.state.cursor = pos + pasted.text().len(); | ||
| self.state.anchor = None; | ||
| self.caret_style_override = None; | ||
| self.layout.invalidate(); | ||
| self.reset_blink(); | ||
| } |
There was a problem hiding this comment.
Run the same caret/cache/scroll maintenance after rich-text paste.
paste_attributed() bypasses apply(), so it mutates text/cursor/layout without invalidating cached_caret_rect or keeping the new caret visible. If a redraw populated the cache earlier, the next frame can paint the caret and update the IME rect from stale geometry after paste.
Proposed fix
fn paste_attributed(&mut self, pasted: &AttributedText) {
if pasted.is_empty() {
return;
}
self.history.push(&self.snapshot(), EditKind::Paste);
@@
self.state.text = self.content.text().to_owned();
self.state.cursor = pos + pasted.text().len();
self.state.anchor = None;
self.caret_style_override = None;
self.layout.invalidate();
+ self.invalidate_caret_cache();
+ self.layout.ensure_layout_attributed(&self.content);
+ self.ensure_cursor_visible();
self.reset_blink();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-text-edit/examples/wd_text_editor.rs` around lines 706 - 740,
paste_attributed() mutates text/cursor/layout but skips the same
caret/cache/scroll maintenance that apply() performs, causing stale cached caret
geometry and IME/scroll issues; after you update self.state.text,
self.state.cursor and self.state.anchor, call the same routines apply() uses to
clear/update caret cache and make the caret visible (e.g. invalidate the
cached_caret_rect via the existing invalidation helper or by setting
cached_caret_rect = None, then call the method that ensures the cursor is
visible / updates scroll and IME rect — the same function(s) apply() invokes,
such as make_cursor_visible()/ensure_cursor_visible()/update_ime_rect), keeping
reset_blink() and layout.invalidate() in place.
Comment on lines
+1643
to
1660
| if let Some((lo, hi)) = inner.editor.selection_range() { | ||
| let plain = inner.editor.selected_text().unwrap_or("").to_string(); | ||
| let html = runs_to_html(&inner.editor.content, lo, hi); | ||
| let _ = self.clipboard.set_html(&html, Some(&plain)); | ||
| } | ||
| } | ||
| PhysicalKey::Code(KeyCode::KeyX) if shift => { | ||
| // Cmd+Shift+X — toggle strikethrough | ||
| inner.editor.toggle_strikethrough(); | ||
| inner.window.request_redraw(); | ||
| } | ||
| PhysicalKey::Code(KeyCode::KeyX) => { | ||
| if let Some(sel) = inner.editor.selected_text() { | ||
| let _ = self.clipboard.set_text(sel.to_string()); | ||
| // Cmd+X — cut | ||
| if let Some((lo, hi)) = inner.editor.selection_range() { | ||
| let plain = inner.editor.selected_text().unwrap_or("").to_string(); | ||
| let html = runs_to_html(&inner.editor.content, lo, hi); | ||
| let _ = self.clipboard.set_html(&html, Some(&plain)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In arboard, if Clipboard::set_htmlreturns an error or HTML clipboard support is unavailable, does it still write the provided plain-text alternative, or should callers explicitly fall back toClipboard::set_text?
💡 Result:
Clipboard::set_html only guarantees the plain-text alternative is written if the call returns Ok(()).
- The API contract says it returns an error only if both the HTML and the
alt_textfailed to be stored—so if you getErr(_), you should assume the plain-text alternative was not written either. [1] - On at least the X11 backend,
set_htmlattempts to store the alternative text (if provided) and the HTML as separate clipboard formats in one write operation—there isn’t a “try HTML, then fall back to text” behavior on error. [2]
So: callers that care about always copying something should explicitly fall back to Clipboard::set_text when set_html returns Err(_) (and you should generally pass alt_text: Some(...) so set_html can succeed even where HTML support is missing/limited). [1]
Sources: [1] (docs.rs), [2] (docs.rs)
Citations:
- 1: https://docs.rs/arboard/latest/arboard/struct.Clipboard.html?utm_source=openai
- 2: https://docs.rs/arboard/latest/src/arboard/platform/linux/x11.rs.html
Honor the advertised plain-text fallback when HTML copy/cut fails.
Both branches ignore set_html(...) errors. When set_html fails, the plain-text alternative is not written either—there is no built-in fallback behavior. On X11 and most backends, set_html attempts to write both HTML and alt_text in a single operation, and if that fails, both are lost. To ensure the advertised "HTML + plain text fallback" behavior works on all platforms, add an explicit set_text fallback when set_html returns an error.
Proposed fix
if let Some((lo, hi)) = inner.editor.selection_range() {
let plain = inner.editor.selected_text().unwrap_or("").to_string();
let html = runs_to_html(&inner.editor.content, lo, hi);
- let _ = self.clipboard.set_html(&html, Some(&plain));
+ if self.clipboard.set_html(&html, Some(&plain)).is_err() {
+ let _ = self.clipboard.set_text(plain);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-text-edit/examples/wd_text_editor.rs` around lines 1643 - 1660,
The clipboard HTML writes currently ignore errors so the plain-text fallback is
never written; update both places where set_html(&html, Some(&plain)) is called
(the copy and cut branches using inner.editor.selection_range(), plain =
inner.editor.selected_text(), and html = runs_to_html(...)) to check the Result
and, if set_html returns Err(_), call self.clipboard.set_text(&plain) as an
explicit fallback (and you can ignore or log both results). Ensure you use the
same plain variable for the fallback and apply this change in both occurrences
of self.clipboard.set_html.
Comment on lines
+569
to
+586
| /// Mutable access to the default style. After modification, call | ||
| /// [`coalesce`](Self::coalesce) if needed. | ||
| #[inline] | ||
| pub fn default_style_mut(&mut self) -> &mut TextStyle { | ||
| &mut self.default_style | ||
| } | ||
|
|
||
| /// The paragraph-level style. | ||
| #[inline] | ||
| pub fn paragraph_style(&self) -> &ParagraphStyle { | ||
| &self.paragraph_style | ||
| } | ||
|
|
||
| /// Mutable access to paragraph style. | ||
| #[inline] | ||
| pub fn paragraph_style_mut(&mut self) -> &mut ParagraphStyle { | ||
| &mut self.paragraph_style | ||
| } |
There was a problem hiding this comment.
Raw mutable style access bypasses generation tracking.
ensure_layout_attributed() keys cache invalidation off generation, but default_style_mut() and paragraph_style_mut() let callers change render-affecting state without bumping it. On empty text, default_style_mut() also leaves the degenerate run on the old style.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-text-edit/src/attributed_text/mod.rs` around lines 569 - 586,
default_style_mut and paragraph_style_mut currently allow callers to change
render-affecting state without updating the cache generation used by
ensure_layout_attributed; change these methods so they bump the internal
generation (e.g., self.generation = self.generation.wrapping_add(1) or
equivalent) whenever they mutate default_style or paragraph_style. Additionally,
in default_style_mut handle the empty-text degenerate run case by updating the
run stored in self.runs[0] (or creating/updating the degenerate run) to reflect
the new default_style so the stored run doesn’t remain on the old style. Ensure
you reference the same fields (default_style, paragraph_style, runs, generation)
so cache invalidation and the degenerate run stay consistent with
ensure_layout_attributed.
Comment on lines
+781
to
+835
| self.generation += 1; | ||
| let hi = hi.min(self.text.len()); | ||
| debug_assert!( | ||
| self.text.is_char_boundary(lo) && self.text.is_char_boundary(hi), | ||
| "delete range [{lo}, {hi}) contains invalid char boundaries" | ||
| ); | ||
|
|
||
| let lo32 = lo as u32; | ||
| let hi32 = hi as u32; | ||
| let span = hi32 - lo32; | ||
|
|
||
| // Remove text. | ||
| self.text.drain(lo..hi); | ||
| let new_len = self.text.len() as u32; | ||
|
|
||
| // Adjust runs. | ||
| let mut i = 0; | ||
| while i < self.runs.len() { | ||
| let run = &mut self.runs[i]; | ||
|
|
||
| if run.end <= lo32 { | ||
| // Run is entirely before the deleted range — no change. | ||
| i += 1; | ||
| } else if run.start >= hi32 { | ||
| // Run is entirely after the deleted range — shift back. | ||
| run.start -= span; | ||
| run.end -= span; | ||
| i += 1; | ||
| } else if run.start >= lo32 && run.end <= hi32 { | ||
| // Run is entirely within the deleted range — remove it. | ||
| self.runs.remove(i); | ||
| // Don't increment i. | ||
| } else if run.start < lo32 && run.end > hi32 { | ||
| // Deleted range is entirely within this run — shrink it. | ||
| run.end -= span; | ||
| i += 1; | ||
| } else if run.start < lo32 { | ||
| // Run overlaps the start of the deleted range. | ||
| run.end = lo32; | ||
| i += 1; | ||
| } else { | ||
| // Run overlaps the end of the deleted range. | ||
| run.start = lo32; | ||
| run.end -= span; | ||
| i += 1; | ||
| } | ||
| } | ||
|
|
||
| // If all runs were removed (deleted entire text), add degenerate run. | ||
| if self.runs.is_empty() { | ||
| self.runs.push(StyledRun { | ||
| start: 0, | ||
| end: 0, | ||
| style: self.default_style.clone(), | ||
| }); |
There was a problem hiding this comment.
Deleting the whole buffer drops the active typing style.
When every run is removed, the replacement degenerate run always uses default_style. If the user had styled the whole paragraph via runs, typing after Select All → Delete reverts to the base style instead of preserving the caret style at the delete point.
💡 Minimal fix
pub fn delete(&mut self, lo: usize, hi: usize) {
if lo >= hi || lo >= self.text.len() {
return;
}
self.generation += 1;
let hi = hi.min(self.text.len());
+ let empty_style = self.caret_style_at(lo as u32).clone();
debug_assert!(
self.text.is_char_boundary(lo) && self.text.is_char_boundary(hi),
"delete range [{lo}, {hi}) contains invalid char boundaries"
@@
if self.runs.is_empty() {
self.runs.push(StyledRun {
start: 0,
end: 0,
- style: self.default_style.clone(),
+ style: empty_style,
});
}
Comment on lines
+1023
to
+1038
| let mut para_style = ParagraphStyle::new(); | ||
| para_style.set_apply_rounding_hack(false); | ||
| para_style.set_text_align(self.config.text_align.to_skia()); | ||
|
|
||
| let mut builder = ParagraphBuilder::new(¶_style, &self.font_collection); | ||
| let fallback_families: Vec<&str> = | ||
| self.config.font_families.iter().map(|s| s.as_str()).collect(); | ||
|
|
||
| // If no runs overlap, use config defaults. | ||
| if runs.is_empty() || block_start >= block_content_end { | ||
| let mut ts = TextStyle::new(); | ||
| ts.set_font_size(self.config.font_size); | ||
| let families: Vec<&str> = self.config.font_families.iter().map(|s| s.as_str()).collect(); | ||
| ts.set_font_families(&families); | ||
| builder.push_style(&ts); | ||
| builder.add_text(&text[block_start..block_content_end]); |
There was a problem hiding this comment.
The rich-text layout path ignores AttributedText paragraph/default styles.
build_paragraph_for_attributed_slice() seeds ParagraphStyle from TextConfig, and the no-run fallback also builds text style from TextConfig. So at.paragraph_style() never reaches Skia, and empty rich-text blocks/caret metrics use the editor config instead of the content model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-text-edit/src/skia_layout.rs` around lines 1023 - 1038, In
build_paragraph_for_attributed_slice(), stop seeding ParagraphStyle and fallback
TextStyle from self.config; instead initialize para_style from the
AttributedText instance (use at.paragraph_style() or the AttributedText API that
returns the paragraph-level style) and when building the no-run fallback
TextStyle use the attributed text's default text style (e.g.,
at.default_text_style() or equivalent) so paragraph-level and default run styles
from the content model reach Skia; update the code paths that currently set
para_style, ts, and font families (the para_style variable, the ts TextStyle
construction, and the runs.is_empty() / block_start >= block_content_end branch
that does builder.push_style and builder.add_text) to use the
AttributedText-provided styles instead of self.config.
Comment on lines
+322
to
+323
| Zero width joiner | ||
| wordwordword |
There was a problem hiding this comment.
Use an actual U+200D sample for the “Zero width joiner” case.
The current Zero width joiner fixture appears to reuse the word-joiner character, so this section likely never exercises real ZWJ behavior. That weakens the fixture right where cursoring/shaping bugs tend to show up.
Suggested fix
Zero width joiner
-wordwordword
+wordwordwordIf you want a stronger ZWJ test, an emoji or shaping-sensitive sequence is even better, e.g. a family emoji or an Indic conjunct.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Zero width joiner | |
| wordwordword | |
| Zero width joiner | |
| wordwordword |
🧰 Tools
🪛 LanguageTool
[grammar] ~322-~322: Use a hyphen to join words.
Context: ...= Zero width space wordwordword Zero width joiner wordwordword Zero width ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fixtures/text/editor.txt` around lines 322 - 323, Replace the incorrect
word-joiner (U+2060) sample in the "Zero width joiner" section — currently shown
as the literal line "wordwordword" — with an actual U+200D character (ZWJ).
Use a shaping-sensitive sequence such as a family emoji (e.g. 👩👩👧👦) or an
Indic conjunct that includes U+200D so the fixture exercises real ZWJ behavior
instead of the word-joiner.
softmarshmallow
changed the title
cg Enhance rich text editing with AttributedText, HTML support, and fontscg rich text editor from ground up
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
1 participant
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.
day-320-wip-skia-native-rich-text-editor.mp4
Note
Medium Risk
Moderate risk due to new rich-text data model, HTML parsing/serialization, and in-place edit+history refactors that change core editing/layout behavior and undo/redo semantics.
Overview
Adds a new run-based rich text model (
attributed_text) including style application/merging, invariant enforcement, and HTML serialize/deserialize helpers for clipboard-friendly formatted copy/paste.Refactors core editing and history to support rich snapshots:
apply_command_mutnow mutatesTextEditorStatein place and returns anEditDelta, and undo/redo is generalized viaGenericEditHistorywith a newEditKind::Stylefor non-mergeable formatting edits.Updates layout utilities for correctness/perf (adds
LineMetrics.left, switches line index lookup to binary search, and optimizes grapheme boundary scans), and upgrades thewd_text_editorexample into a rich-text prototype with formatting shortcuts, HTML copy/cut/paste, variable font loading, vertical scrolling, caret caching, and drag-and-drop loading of.txt/.html.Written by Cursor Bugbot for commit 66b9072. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
Tests