paint: Add support for a blinking text caret#43128
Merged
Merged
Conversation
|
🔨 Triggering try run (#22908626710) for Linux (WPT) |
|
Test results for linux-wpt from try job (#22908626710): Flaky unexpected result (25)
Stable unexpected results that are known to be intermittent (20)
Stable unexpected results (1)
|
|
|
|
🔨 Triggering try run (#22910632629) for Linux (WPT) |
|
Test results for linux-wpt from try job (#22910632629): Flaky unexpected result (26)
Stable unexpected results that are known to be intermittent (16)
|
|
✨ Try run (#22910632629) succeeded. |
Loirooriol
reviewed
Mar 10, 2026
| WindowGLContext::initialize_image_handler(&mut external_image_handlers); | ||
|
|
||
| let embedder_to_constellation_sender = paint.embedder_to_constellation_sender.clone(); | ||
| let timer_refresh_driver = LazyCell::new(Default::default); |
Contributor
There was a problem hiding this comment.
Suggested change
| let timer_refresh_driver = LazyCell::new(Default::default); | |
| let timer_refresh_driver = LazyCell::default(); |
| let value = if caret_visible || self.remaining_blink_count == 0 { | ||
| self.original_caret_color | ||
| } else { | ||
| ColorF::new(0., 0., 0., 0.) |
Contributor
There was a problem hiding this comment.
Suggested change
| ColorF::new(0., 0., 0., 0.) | |
| ColorF::TRANSPARENT |
87e6cbb to
61c303d
Compare
mukilan
approved these changes
Mar 12, 2026
mukilan
left a comment
Member
There was a problem hiding this comment.
Nice! One question - when I load google.com, I see that the caret starts blinking only after a couple of seconds (approximately). Is that due to some other issue in our layout system?
| } | ||
| } | ||
|
|
||
| /// This structure tracks the animations active for a given pipeline. Currently only caret |
Member
There was a problem hiding this comment.
Given that the struct called CaretAnimation which is very specific, I'm not sure if it make sense to imply that it tracks all animations in the pipeline. Was this doc comment meant to be for the PipelineAnimations struct?
Member
Author
There was a problem hiding this comment.
Oh, exactly. I've moved this to PipelineAnimations and have added relevant rustdoc comment here.
| let insertion_point_common = builder.common_properties(insertion_point_rect, &parent_style); | ||
| builder.wr().push_rect( | ||
|
|
||
| let property_binding_key = PropertyBindingKey::new(1); |
Member
There was a problem hiding this comment.
I'm not familar with the WebRender animation system. Why is it safe here to use a fixed key of 1?
Member
Author
There was a problem hiding this comment.
Hrm. This should likely be based on the pipeline. I'll make that change.
| self.need_update.store(false, Ordering::Relaxed); | ||
|
|
||
| if colors.is_empty() { | ||
| self.caret_visible.set(true); |
Member
There was a problem hiding this comment.
It is not obvious to me why we need to set caret_visible to true here. Is it because all pipelines have reached the cursor timeout? Perhaps we can add a comment here?
Member
Author
There was a problem hiding this comment.
I'll add a comment clarifying this. Thanks for pointing it out.
Member
Author
Thanks for the review. I think the behavior you are seeing on the site is the result of lag and the fact that the cursor starts in the visible state. That said, this matches the behavior I see when clicking on things in Firefox as well (such as the URL bar), so I think it isn't so bad. |
This change adds support for a blinking text caret using WebRender `DynamicProperties` updates. This ensures that text caret updates are as lightweight as possible. As part of this change, the initial framework for paint-side animations is added. Though this is driven by a timer for now (in order to not have to run full speed animations for text carets), the idea is that more animations can be driven by `Paint` in the future (in order to avoid doing layouts during animations). In addition, a preference is added which controls the caret blink speed. This is used to disable caret blinking during WPT testing. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
61c303d to
a199076
Compare
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.
This change adds support for a blinking text caret using WebRender
DynamicPropertiesupdates. This ensures that text caret updates are aslightweight as possible. As part of this change, the initial framework
for paint-side animations is added. Though this is driven by a timer for
now (in order to not have to run full speed animations for text carets),
the idea is that more animations can be driven by
Paintin the future(in order to avoid doing layouts during animations).
In addition, a preference is added which controls the caret blink speed.
This is used to disable caret blinking during WPT testing.
Testing: There are no tests for this change. We do not currently have a good
way to test caret blinking as the caret is render only and fully testing
blinking would require well timed screenshot creation.
Fixes: #33237.