script: Add a Rope type and use it for TextInput#41650
Merged
mrobinson merged 1 commit intoservo:mainfrom Jan 6, 2026
Merged
Conversation
servo-highfive
added
the
S-awaiting-review
|
🔨 Triggering try run (#20677166239) for Linux (WPT) |
|
Test results for linux-wpt from try job (#20677166239): Flaky unexpected result (31)
Stable unexpected results that are known to be intermittent (23)
|
|
✨ Try run (#20677166239) succeeded. |
Member
Author
|
Apologies for the large change here. To replace the functionality in FWIW, the motivation for this change is what we have planned improvements to interactivity in Servo and this is the first step, which will make followup work a lot easier. |
Contributor
|
I think this is a good change as it cleans up the textinput code. However, I am wondering about the impact on Domstring conversions. If I understand correctly, the Rope will always be a vector of Rust Strings, hence, any strings from the JS Engine need to be converted. Is that right? |
Member
Author
Yes, I think that is effectively the case anyway though, as most string operations require Rust strings currently anyway. For instance, as soon as you type a character into the input field. My long term plan is that we eventually factor out the non-JavaScript string types into |
mukilan
reviewed
Jan 6, 2026
components/shared/base/rope.rs
Outdated
| } | ||
|
|
||
| let start_line = self.line_for_index_mut(start_index); | ||
| let last_line = new_contents.last().expect("Should have at last one line"); |
Member
There was a problem hiding this comment.
Suggested change
| let last_line = new_contents.last().expect("Should have at last one line"); | |
| let last_line = new_contents.last().expect("Should have at least one line"); |
components/shared/base/rope.rs
Outdated
| boundary_value | ||
| } | ||
|
|
||
| /// Given a [`RopeIndex`] clamp it, meaning that its indices are all bound by the |
Member
There was a problem hiding this comment.
Suggested change
| /// Given a [`RopeIndex`] clamp it, meaning that its indices are all bound by the | |
| /// Given a [`RopeIndex`], clamp it, meaning that its indices are all bound by the |
components/shared/base/rope.rs
Outdated
| } | ||
|
|
||
| /// Given a [`RopeIndex`] clamp it, meaning that its indices are all bound by the | ||
| /// actual size of the line and the number of lines in this [`Rope] |
Member
There was a problem hiding this comment.
Suggested change
| /// actual size of the line and the number of lines in this [`Rope] | |
| /// actual size of the line and the number of lines in this [`Rope`]. |
components/shared/base/rope.rs
Outdated
| RopeIndex::new(line_index, rope_index.code_point.min(line_length_utf8)) | ||
| } | ||
|
|
||
| /// Convert a TextPoint into a byte offset from the start of the content. |
Member
There was a problem hiding this comment.
Suggested change
| /// Convert a TextPoint into a byte offset from the start of the content. | |
| /// Convert a [`RopeIndex`] into a byte offset from the start of the content. |
components/shared/base/rope.rs
Outdated
| let final_line = self.line(rope_index.line); | ||
|
|
||
| // The offset might be past the end of the line due to being an exclusive offset and | ||
| // also the fact that every line has a virtual newline at the end (apart from the last). |
Member
There was a problem hiding this comment.
I think this comment has been carried over from the previous code. Aren't all the lines now represented with a real newline character at the end? Also, maybe RopeIndex documentation needs to mention that it can point to the final newline and so APIs like replace_range can potentially delete the newline to "append" to the end of a line, if the start index is directly pointing to the end of the line, rather than beyond it.
Member
Author
There was a problem hiding this comment.
Yeah, this is an old comment. I've also added more information to the rustdoc for RopeIndex as you suggested.
| range: Range<RopeIndex>, | ||
| string: impl Into<String>, | ||
| ) -> RopeIndex { | ||
| let start_index = range.start; |
Member
There was a problem hiding this comment.
Should start_index be clamped first or is it expected that this API should only be called for a valid range within the rope and thus can panic otherwise?
Member
Author
There was a problem hiding this comment.
Hrm. I can add an assertion here, which I think will address your question.
components/shared/base/rope.rs
Outdated
| /// [rope data structure]: https://en.wikipedia.org/wiki/Rope_(data_structure) | ||
| #[derive(MallocSizeOf)] | ||
| pub struct Rope { | ||
| /// The lines of the rope. Each an owned strings each with a trailing newline (`\n`), |
Member
There was a problem hiding this comment.
Suggested change
| /// The lines of the rope. Each an owned strings each with a trailing newline (`\n`), | |
| /// The lines of the rope. Each line is an owned string that ends with a trailing newline (`\n`), |
components/shared/base/rope.rs
Outdated
|
|
||
| /// Return a [`RopeIndex`] which points to the start of the subsequent line. | ||
| /// If the given [`RopeIndex`] is already on the final line, this will return | ||
| /// the final line of the entire [`Rope`]. |
Member
There was a problem hiding this comment.
Suggested change
| /// the final line of the entire [`Rope`]. | |
| /// the last index of the entire [`Rope`]. |
"will return the final line" seems confusing.
Member
Author
There was a problem hiding this comment.
Yeah. It should say "final index" here.
| RopeIndex::new(line_index, self.line(line_index).len()) | ||
| } | ||
|
|
||
| /// Replace the given range of [`RopeIndex`]s with the given string. Returns the |
Member
There was a problem hiding this comment.
The API assumes that the replacement string has at least one newline at the end. This allows the user of the API to violate the invariant that every line except the last has a final new line.
For example,
let mut rope = Rope::new("A\nBB\nCCC", Lines::Multiple);
rope.replace_range(RopeIndex::new(0, 2)..RopeIndex::new(1, 0), "D");
will put the rope into an invalid state
A\nD
BB\n
CCC
Not sure if it is sufficient to document the requirements of the string parameter here.
Member
Author
There was a problem hiding this comment.
Great point. I've adjust the way that clamping indexes works, so that now clamped indexes never point past the trailing newline in any line. I also now clamp all inputs to fn replace_range and fn delete_range. Finally I've added unit tests for these cases.
components/shared/base/rope.rs
Outdated
|
|
||
| /// Convert a byte offset from the start of the content into a RopeIndex. | ||
| pub fn utf8_offset_to_rope_index(&self, utf8_offset: Utf8CodeUnitLength) -> RopeIndex { | ||
| let mut current_utf8_offset = utf8_offset; |
Member
There was a problem hiding this comment.
It seems like if current_utf8_offset were declared as usize i.e
let mut current_utf8_offset = utf8_offset.0;
we could avoid all the unwrapping and wrapping in the code below. Not sure if this is written this way for documentation, as the Utf8CodeUnitLength itself doesn't perform any validation and the code uses only the Sub implementation of the wrapper type, which is identical to usize.
Member
Author
|
@mukilan Thank you kindly for the review. I think I have addressed all of your concerns. |
mukilan
reviewed
Jan 6, 2026
components/shared/base/rope.rs
Outdated
| /// [rope data structure]: https://en.wikipedia.org/wiki/Rope_(data_structure) | ||
| #[derive(MallocSizeOf)] | ||
| pub struct Rope { | ||
| /// The lines of the rope. Each line is an owned strings that ends with a newline |
Member
There was a problem hiding this comment.
Suggested change
| /// The lines of the rope. Each line is an owned strings that ends with a newline | |
| /// The lines of the rope. Each line is an owned string that ends with a newline |
components/shared/base/rope.rs
Outdated
| fn test_rope_replace_slice() { | ||
| let mut rope = Rope::new("AAA\nBBB\nCCC", Lines::Multiple); | ||
| rope.replace_range(RopeIndex::new(0, 1)..RopeIndex::new(0, 2), "x"); | ||
| assert_eq!(rope.contents(), "Ax\nBBB\nCCC",); |
Member
There was a problem hiding this comment.
The CI shows a failure at this assertion. Looks like just the expectation needs to be updated as the input range only includes one 'A' character in the middle.
Suggested change
| assert_eq!(rope.contents(), "Ax\nBBB\nCCC",); | |
| assert_eq!(rope.contents(), "AxA\nBBB\nCCC",); |
Member
Author
There was a problem hiding this comment.
I've modified this now.
components/shared/base/rope.rs
Outdated
| rope.replace_range(RopeIndex::new(0, 2)..RopeIndex::new(1, 0), "D"); | ||
| assert_eq!( | ||
| rope.contents(), | ||
| "ADBB\nCCC", |
Member
There was a problem hiding this comment.
This seems to be passing only because rope.contents() joins the individual lines and thus hides the missing new line. Perhaps the test needs to check each individual line by asserting the value of rope.lines.
Member
Author
There was a problem hiding this comment.
Ah, I see the issue. I've rewritten delete_range to address this and improved the tests to detect the bug you have found.
Member
Author
|
@mukilan I think I've addressed your concerns. Please take another look. |
mukilan
approved these changes
Jan 6, 2026
components/shared/base/rope.rs
Outdated
| "Trying to delete a slice beyond the last index should not put the Rope in an invalid state." | ||
| ); | ||
| rope.delete_range(RopeIndex::new(0, 3)..RopeIndex::new(0, 4)); | ||
| assert_eq!(rope.lines.len(), 3); |
Member
There was a problem hiding this comment.
We can use arrays to assert both the length and contents together, which is also more compact.
i.e
assert_eq!(rope.lines, ["ABC\n", "DEF\n", ""]);
But this just a stylistic suggestion.
Member
Author
There was a problem hiding this comment.
This makes sense. I've done this.
servo-highfive
removed
the
S-awaiting-review
`TextInput` internally stores a rope of text content, one segment for each line. This change separates out the rope into a separate `Rope` data structure that can be shared with other parts of Servo. `TextInput` needs to move through text content in a variety of ways, depending on the keys pressed when interacting with a text area. This change provides a unified movement API in both `Rope` and in `TextInput` ensuring that selection is handled consistently (apart from a few minor quirks [^1]). This simplifies the code an improves interactive behavior. [^1] One of these quirks is that the edit point will move to directional end of the motion when collapsing a selection, but only when moving by grapheme or word (and not by line). These quirks existed in an undocumented way previously and they are preserved with code comments. Signed-off-by: Martin Robinson <[email protected]>
servo-highfive
added
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
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
4 participants
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.
TextInputinternally stores a rope of text content, one segment foreach line. This change separates out the rope into a separate
Ropedata structure that can be shared with other parts of Servo.
TextInputneeds to move through text content in a variety of ways,depending on the keys pressed when interacting with a text area. This
change provides a unified movement API in both
Ropeand inTextInputensuring that selection is handled consistently (apart froma few minor quirks 1). This simplifies the code an improves
interactive behavior.
Testing: This is covered by existing unit tests (updated for the new API) and
the WPT suite.
Footnotes
One of these quirks is that the edit point will move to
directional end of the motion when collapsing a selection, but only when
moving by grapheme or word (and not by line). These quirks existed in an
undocumented way previously and they are preserved with code comments. ↩