Skip to content

Commit

Permalink
Auto merge of #29664 - mrobinson:fix-animation-flakiness, r=mukilan
Browse files Browse the repository at this point in the history 
Fix flakiness in animation tests

1. When updating the animation timeline, ensure that nodes that are
    animating are marked dirty, if necessary, so any style queries will
    force a layout flush.
2. Disable the problematic transition test suites, as they are in Gecko.
    These suites often fail when Servo is so overloaded that it cannot
    deliver frames fast enough to get more than two samples during the
    animation lifecycle.

Fixes #28334.
Fixes #26435.
Fixes #21486.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #28334, #26435, #21486.

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
  • Loading branch information
@bors-servo
bors-servo committed May 2, 2023
2 parents 2691d2a + 17e3f34 commit 0a5fbd4
Show file tree
Hide file tree
Showing 19 changed files with 51 additions and 4,359 deletions.
22 changes: 18 additions & 4 deletions components/script/animations.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */

#![deny(missing_docs)]

//! The set of animations for a document.

use crate::dom::animationevent::AnimationEvent;
Expand Down Expand Up @@ -47,6 +45,11 @@ pub(crate) struct Animations {

/// A list of pending animation-related events.
pending_events: DomRefCell<Vec<TransitionOrAnimationEvent>>,

/// The timeline value at the last time all animations were marked dirty.
/// This is used to prevent marking animations dirty when the timeline
/// has not changed.
timeline_value_at_last_dirty: Cell<f64>,
}

impl Animations {
Expand All @@ -56,6 +59,7 @@ impl Animations {
has_running_animations: Cell::new(false),
rooted_nodes: Default::default(),
pending_events: Default::default(),
timeline_value_at_last_dirty: Cell::new(0.0),
}
}

Expand All @@ -65,12 +69,23 @@ impl Animations {
self.pending_events.borrow_mut().clear();
}

pub(crate) fn mark_animating_nodes_as_dirty(&self) {
// Mark all animations dirty, if they haven't been marked dirty since the
// specified `current_timeline_value`. Returns true if animations were marked
// dirty or false otherwise.
pub(crate) fn mark_animating_nodes_as_dirty(&self, current_timeline_value: f64) -> bool {
if current_timeline_value <= self.timeline_value_at_last_dirty.get() {
return false;
}
self.timeline_value_at_last_dirty
.set(current_timeline_value);

let sets = self.sets.sets.read();
let rooted_nodes = self.rooted_nodes.borrow();
for node in sets.keys().filter_map(|key| rooted_nodes.get(&key.node)) {
node.dirty(NodeDamage::NodeStyleDamaged);
}

true
}

pub(crate) fn update_for_new_timeline_value(&self, window: &Window, now: f64) {
Expand All @@ -97,7 +112,6 @@ impl Animations {
}

self.unroot_unused_nodes(&sets);
//self.update_running_animations_presence(window);
}

/// Cancel animations for the given node, if any exist.
Expand Down
12 changes: 12 additions & 0 deletions components/script/dom/document.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3852,6 +3852,18 @@ impl Document {
.update_for_new_timeline_value(&self.window, current_timeline_value);
}

pub(crate) fn maybe_mark_animating_nodes_as_dirty(&self) {
let current_timeline_value = self.current_animation_timeline_value();
let marked_dirty = self
.animations
.borrow()
.mark_animating_nodes_as_dirty(current_timeline_value);

if marked_dirty {
self.window().add_pending_reflow();
}
}

pub(crate) fn current_animation_timeline_value(&self) -> f64 {
self.animation_timeline.borrow().current_value()
}
Expand Down
7 changes: 4 additions & 3 deletions components/script/script_thread.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1702,10 +1702,12 @@ impl ScriptThread {
true
}

// Perform step 11.10 from https://html.spec.whatwg.org/multipage/#event-loops.
// Perform step 7.10 from https://html.spec.whatwg.org/multipage/#event-loop-processing-model.
// Described at: https://drafts.csswg.org/web-animations-1/#update-animations-and-send-events
fn update_animations_and_send_events(&self) {
for (_, document) in self.documents.borrow().iter() {
document.update_animation_timeline();
document.maybe_mark_animating_nodes_as_dirty();
}

for (_, document) in self.documents.borrow().iter() {
Expand Down Expand Up @@ -3012,8 +3014,7 @@ impl ScriptThread {
document.run_the_animation_frame_callbacks();
}
if tick_type.contains(AnimationTickType::CSS_ANIMATIONS_AND_TRANSITIONS) {
document.animations().mark_animating_nodes_as_dirty();
document.window().add_pending_reflow();
document.maybe_mark_animating_nodes_as_dirty();
}
}

Expand Down
216 changes: 1 addition & 215 deletions tests/wpt/metadata-layout-2020/css/css-transitions/properties-value-001.html.ini
View file
Original file line number Diff line number Diff line change
@@ -1,216 +1,2 @@
[properties-value-001.html]
[background-position length(pt) / values]
expected: FAIL

[background-position length(pt) / events]
expected: FAIL

[background-position length(px) / values]
expected: FAIL

[background-position length(cm) / events]
expected: FAIL

[background-position length(mm) / values]
expected: FAIL

[background-position length(in) / events]
expected: FAIL

[background-position length(em) / events]
expected: FAIL

[background-position length(ex) / values]
expected: FAIL

[background-position length(cm) / values]
expected: FAIL

[background-position length(ex) / events]
expected: FAIL

[background-position length(pc) / events]
expected: FAIL

[background-position length(in) / values]
expected: FAIL

[background-position length(pc) / values]
expected: FAIL

[background-position percentage(%) / values]
expected: FAIL

[background-position length(mm) / events]
expected: FAIL

[background-position length(em) / values]
expected: FAIL

[background-position percentage(%) / events]
expected: FAIL

[background-position length(px) / events]
expected: FAIL

[text-shadow shadow(shadow) / values]
expected: FAIL

[text-shadow shadow(shadow) / events]
expected: FAIL

[outline-color color(rgba) / values]
expected: FAIL

[outline-color color(rgba) / events]
expected: FAIL

[outline-offset length(pt) / values]
expected: FAIL

[outline-offset length(pt) / events]
expected: FAIL

[outline-offset length(pc) / values]
expected: FAIL

[outline-offset length(pc) / events]
expected: FAIL

[outline-offset length(px) / values]
expected: FAIL

[outline-offset length(px) / events]
expected: FAIL

[outline-offset length(em) / values]
expected: FAIL

[outline-offset length(em) / events]
expected: FAIL

[outline-offset length(ex) / values]
expected: FAIL

[outline-offset length(ex) / events]
expected: FAIL

[outline-offset length(mm) / values]
expected: FAIL

[outline-offset length(mm) / events]
expected: FAIL

[outline-offset length(cm) / values]
expected: FAIL

[outline-offset length(cm) / events]
expected: FAIL

[outline-offset length(in) / values]
expected: FAIL

[outline-offset length(in) / events]
expected: FAIL

[outline-width length(pt) / values]
expected: FAIL

[outline-width length(pt) / events]
expected: FAIL

[outline-width length(pc) / values]
expected: FAIL

[outline-width length(pc) / events]
expected: FAIL

[outline-width length(px) / values]
expected: FAIL

[outline-width length(px) / events]
expected: FAIL

[outline-width length(em) / values]
expected: FAIL

[outline-width length(em) / events]
expected: FAIL

[outline-width length(ex) / values]
expected: FAIL

[outline-width length(ex) / events]
expected: FAIL

[outline-width length(mm) / values]
expected: FAIL

[outline-width length(mm) / events]
expected: FAIL

[outline-width length(cm) / values]
expected: FAIL

[outline-width length(cm) / events]
expected: FAIL

[outline-width length(in) / values]
expected: FAIL

[outline-width length(in) / events]
expected: FAIL

[vertical-align length(pt) / values]
expected: FAIL

[vertical-align length(pt) / events]
expected: FAIL

[vertical-align length(pc) / values]
expected: FAIL

[vertical-align length(pc) / events]
expected: FAIL

[vertical-align length(px) / values]
expected: FAIL

[vertical-align length(px) / events]
expected: FAIL

[vertical-align length(em) / values]
expected: FAIL

[vertical-align length(em) / events]
expected: FAIL

[vertical-align length(ex) / values]
expected: FAIL

[vertical-align length(ex) / events]
expected: FAIL

[vertical-align length(mm) / values]
expected: FAIL

[vertical-align length(mm) / events]
expected: FAIL

[vertical-align length(cm) / values]
expected: FAIL

[vertical-align length(cm) / events]
expected: FAIL

[vertical-align length(in) / values]
expected: FAIL

[vertical-align length(in) / events]
expected: FAIL

[vertical-align percentage(%) / values]
expected: FAIL

[vertical-align percentage(%) / events]
expected: FAIL
disabled: https://bugzilla.mozilla.org/show_bug.cgi?id=1472172
6 changes: 1 addition & 5 deletions tests/wpt/metadata-layout-2020/css/css-transitions/properties-value-002.html.ini
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,2 @@
[properties-value-002.html]
[vertical-align vertical(keyword) / values]
expected: FAIL

[vertical-align vertical(keyword) / events]
expected: FAIL
disabled: https://bugzilla.mozilla.org/show_bug.cgi?id=1472172

0 comments on commit 0a5fbd4

Please sign in to comment.