-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
paint: Do trigger animations when a non-animation paint happens #40433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
paint: Do trigger animations when a non-animation paint happens #40433
Conversation
| } | ||
|
|
||
| self.waiting_for_frame.load(Ordering::Relaxed) | ||
| pub(crate) fn wait_to_paint(&self) -> bool { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .add_touch_move_refresh_obsever_if_necessary( | |
| .add_touch_move_refresh_observer_if_necessary( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. Fixed.
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>
043c7e9 to
c025635
Compare
Otherwise the
RefreshObsevercan get wedged. This does mean that wewill 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 thischange fixes it.