Skip to content

Conversation

@mrobinson
Copy link
Member

@mrobinson mrobinson commented Nov 5, 2025

Otherwise the RefreshObsever can get wedged. This does mean that we
will overpaint a bit when processing input events and animating, but we
could integrate scroll / zoom event handling with the animation loop to
eliminate this overpaint.

Testing: This kind of fix for touch interaction is currently very hard to test as it
relies heavily on the order that operations happen between Servo and the
RefreshDriver. It is easy to reproduce in interactive usage though and this
change fixes it.

@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 5, 2025
@mrobinson mrobinson requested a review from mukilan November 20, 2025 05:33
}

self.waiting_for_frame.load(Ordering::Relaxed)
pub(crate) fn wait_to_paint(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the doc comment need to be updated as well, as the code no longer checks the repaint reason?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! I've updated this.

self.webview.set_animating(self.animating());
}

pub(crate) fn update_touch_handling_at_new_frame_start(&mut self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could add a doc comment to explain what the boolean return value represents.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that makes a lot of sense. I've done that.

}

self.touch_handler
.add_touch_move_refresh_obsever_if_necessary(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.add_touch_move_refresh_obsever_if_necessary(
.add_touch_move_refresh_observer_if_necessary(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Fixed.

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 20, 2025
Otherwise the `RefreshObsever` can get wedged. This does mean that we
will overpaint a bit when processing input events and animating, but we
could integrate scroll / zoom event handling with the animation loop to
eliminate this overpaint.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>
@mrobinson mrobinson force-pushed the do-trigger-animations-when-non-animation-paint-happens branch from 043c7e9 to c025635 Compare November 20, 2025 19:06
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 20, 2025
@mrobinson mrobinson enabled auto-merge November 20, 2025 19:07
@mrobinson mrobinson added this pull request to the merge queue Nov 20, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 20, 2025
Merged via the queue into servo:main with commit 75a249c Nov 20, 2025
36 checks passed
@mrobinson mrobinson deleted the do-trigger-animations-when-non-animation-paint-happens branch November 20, 2025 19:45
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-awaiting-review There is new code that needs to be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants