LibWeb: Ensure registered transitions are reflective of properties

Calme1709 · gmta · commit b2b889e1dab3 · 2025-11-23T09:43:24.000+01:00
Previously we would only update these if:
a) We had a cascaded value for `transition-property`
b) The source of that cascaded value had changed since we last
   registered transitions

This meant that there were a lot of changes we didn't apply:
 - Changes exclusively to properties other than `transition-property`
   (e.g. `transition-duration`, `transition-behavior`, etc)
 - Removing the `transition-property` property
 - Updating the `transition-property` property in a way that didn't
   change it's source (e.g. setting it within inline-style)

Unfortunately this does mean that we now register transitions for all
properties on most elements since "all" is the initial value for
"transition-property" which isn't great for performance, but that can be
looked at in later commits.
diff --git a/Libraries/LibWeb/Animations/Animatable.cpp b/Libraries/LibWeb/Animations/Animatable.cpp
@@ -22,7 +22,6 @@ namespace Web::Animations {
 struct Animatable::Transition {
     HashMap<CSS::PropertyID, size_t> transition_attribute_indices;
     Vector<TransitionAttributes> transition_attributes;
-    GC::Ptr<CSS::CSSStyleDeclaration const> cached_transition_property_source;
     HashMap<CSS::PropertyID, GC::Ref<CSS::CSSTransition>> associated_transitions;
 };
 
@@ -255,7 +254,6 @@ void Animatable::visit_edges(JS::Cell::Visitor& visitor)
 
     for (auto const& transition : impl.transitions) {
         if (transition) {
-            visitor.visit(transition->cached_transition_property_source);
             visitor.visit(transition->associated_transitions);
         }
     }
@@ -291,22 +289,6 @@ HashMap<FlyString, GC::Ref<Animation>>* Animatable::css_defined_animations(Optio
     return impl.css_defined_animations[index];
 }
 
-GC::Ptr<CSS::CSSStyleDeclaration const> Animatable::cached_transition_property_source(Optional<CSS::PseudoElement> pseudo_element) const
-{
-    auto* maybe_transition = ensure_transition(pseudo_element);
-    if (!maybe_transition)
-        return {};
-    return maybe_transition->cached_transition_property_source;
-}
-
-void Animatable::set_cached_transition_property_source(Optional<CSS::PseudoElement> pseudo_element, GC::Ptr<CSS::CSSStyleDeclaration const> value)
-{
-    auto* maybe_transition = ensure_transition(pseudo_element);
-    if (!maybe_transition)
-        return;
-    maybe_transition->cached_transition_property_source = value;
-}
-
 Animatable::Impl& Animatable::ensure_impl() const
 {
     if (!m_impl)
diff --git a/Libraries/LibWeb/Animations/Animatable.h b/Libraries/LibWeb/Animations/Animatable.h
@@ -58,9 +58,6 @@ class WEB_API Animatable {
     void add_css_animation(FlyString name, Optional<CSS::PseudoElement>, GC::Ref<Animation>);
     void remove_css_animation(FlyString name, Optional<CSS::PseudoElement>);
 
-    GC::Ptr<CSS::CSSStyleDeclaration const> cached_transition_property_source(Optional<CSS::PseudoElement>) const;
-    void set_cached_transition_property_source(Optional<CSS::PseudoElement>, GC::Ptr<CSS::CSSStyleDeclaration const> value);
-
     void add_transitioned_properties(Optional<CSS::PseudoElement>, Vector<Vector<CSS::PropertyID>> properties, CSS::StyleValueVector delays, CSS::StyleValueVector durations, CSS::StyleValueVector timing_functions, CSS::StyleValueVector transition_behaviors);
     Vector<CSS::PropertyID> property_ids_with_matching_transition_property_entry(Optional<CSS::PseudoElement>) const;
     Optional<TransitionAttributes const&> property_transition_attributes(Optional<CSS::PseudoElement>, CSS::PropertyID) const;
diff --git a/Libraries/LibWeb/CSS/ComputedProperties.cpp b/Libraries/LibWeb/CSS/ComputedProperties.cpp
@@ -55,7 +55,6 @@ ComputedProperties::~ComputedProperties() = default;
 void ComputedProperties::visit_edges(Visitor& visitor)
 {
     Base::visit_edges(visitor);
-    visitor.visit(m_transition_property_source);
 }
 
 bool ComputedProperties::is_property_important(PropertyID property_id) const
diff --git a/Libraries/LibWeb/CSS/ComputedProperties.h b/Libraries/LibWeb/CSS/ComputedProperties.h
@@ -70,9 +70,6 @@ class WEB_API ComputedProperties final : public JS::Cell {
     StyleValue const& property(PropertyID, WithAnimationsApplied = WithAnimationsApplied::Yes) const;
     void revert_property(PropertyID, ComputedProperties const& style_for_revert);
 
-    GC::Ptr<CSSStyleDeclaration const> transition_property_source() const { return m_transition_property_source; }
-    void set_transition_property_source(GC::Ptr<CSSStyleDeclaration const> declaration) { m_transition_property_source = declaration; }
-
     Size size_value(PropertyID) const;
     [[nodiscard]] Variant<LengthPercentage, NormalGap> gap_value(PropertyID) const;
     Length length(PropertyID) const;
@@ -291,8 +288,6 @@ class WEB_API ComputedProperties final : public JS::Cell {
     Vector<ShadowData> shadow(PropertyID, Layout::Node const&) const;
     Position position_value(PropertyID) const;
 
-    GC::Ptr<CSSStyleDeclaration const> m_transition_property_source;
-
     Array<RefPtr<StyleValue const>, number_of_longhand_properties> m_property_values;
     Array<u8, ceil_div(number_of_longhand_properties, 8uz)> m_property_important {};
     Array<u8, ceil_div(number_of_longhand_properties, 8uz)> m_property_inherited {};
diff --git a/Libraries/LibWeb/CSS/StyleComputer.cpp b/Libraries/LibWeb/CSS/StyleComputer.cpp
@@ -1221,19 +1221,15 @@ static void apply_dimension_attribute(CascadedProperties& cascaded_properties, D
 
 static void compute_transitioned_properties(ComputedProperties const& style, DOM::AbstractElement abstract_element)
 {
-    auto const source_declaration = style.transition_property_source();
-    if (!source_declaration)
-        return;
+    // FIXME: For now we don't bother registering transitions on the first computation since they can't run (because
+    //        there is nothing to transition from) but this will change once we implement @starting-style
     if (!abstract_element.computed_properties())
         return;
     // FIXME: Add transition helpers on AbstractElement.
     auto& element = abstract_element.element();
     auto pseudo_element = abstract_element.pseudo_element();
-    if (source_declaration == element.cached_transition_property_source(pseudo_element))
-        return;
-    // Reparse this transition property
+
     element.clear_registered_transitions(pseudo_element);
-    element.set_cached_transition_property_source(pseudo_element, *source_declaration);
 
     auto coordinated_transition_list = style.assemble_coordinated_value_list(
         PropertyID::TransitionProperty,
@@ -2541,10 +2537,6 @@ GC::Ref<ComputedProperties> StyleComputer::compute_properties(DOM::AbstractEleme
         computed_style->set_property(property_id, value.release_nonnull(), inherited, cascaded_properties.is_property_important(property_id) ? Important::Yes : Important::No);
         if (animated_value.has_value())
             computed_style->set_animated_property(property_id, animated_value.value(), inherited);
-
-        if (property_id == PropertyID::TransitionProperty) {
-            computed_style->set_transition_property_source(cascaded_properties.property_source(property_id));
-        }
     }
 
     // Compute the value of custom properties
diff --git a/Tests/LibWeb/Text/expected/css/transition-properties-updated-on-duration-change.txt b/Tests/LibWeb/Text/expected/css/transition-properties-updated-on-duration-change.txt
@@ -0,0 +1 @@
+2000
diff --git a/Tests/LibWeb/Text/expected/css/transition-properties-updated-on-property-name-changed-in-same-css-style-properties.txt b/Tests/LibWeb/Text/expected/css/transition-properties-updated-on-property-name-changed-in-same-css-style-properties.txt
@@ -0,0 +1 @@
+1
diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/css-pseudo/parsing/marker-supported-properties-in-animation.txt b/Tests/LibWeb/Text/expected/wpt-import/css/css-pseudo/parsing/marker-supported-properties-in-animation.txt
@@ -2,8 +2,8 @@ Harness status: OK
 
 Found 96 tests
 
-39 Pass
-57 Fail
+41 Pass
+55 Fail
 Pass	Animation of font in ::marker
 Pass	Animation of font-family in ::marker
 Fail	Animation of font-feature-settings in ::marker
@@ -59,7 +59,7 @@ Pass	Transition of font-kerning in ::marker
 Fail	Transition of font-size in ::marker
 Fail	Transition of font-size-adjust in ::marker
 Fail	Transition of font-stretch in ::marker
-Fail	Transition of font-style in ::marker
+Pass	Transition of font-style in ::marker
 Fail	Transition of font-synthesis in ::marker
 Fail	Transition of font-synthesis-small-caps in ::marker
 Fail	Transition of font-synthesis-style in ::marker
@@ -91,7 +91,7 @@ Fail	Transition of text-emphasis in ::marker
 Fail	Transition of text-emphasis-color in ::marker
 Fail	Transition of text-emphasis-position in ::marker
 Fail	Transition of text-emphasis-style in ::marker
-Fail	Transition of text-shadow in ::marker
+Pass	Transition of text-shadow in ::marker
 Pass	Transition of cursor in ::marker
 Fail	Transition of display in ::marker
 Fail	Transition of position in ::marker
diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/css-transitions/CSSTransition-ready.tentative.txt b/Tests/LibWeb/Text/expected/wpt-import/css/css-transitions/CSSTransition-ready.tentative.txt
@@ -2,7 +2,6 @@ Harness status: OK
 
 Found 2 tests
 
-1 Pass
-1 Fail
-Fail	ready promise is rejected when a transition is canceled by updating transition-property
+2 Pass
+Pass	ready promise is rejected when a transition is canceled by updating transition-property
 Pass	ready promise is rejected when a transition is canceled by changing the transition property to something not interpolable
diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/css-transitions/transition-remove-and-change-immediate.txt b/Tests/LibWeb/Text/expected/wpt-import/css/css-transitions/transition-remove-and-change-immediate.txt
@@ -0,0 +1,6 @@
+Harness status: OK
+
+Found 1 tests
+
+1 Pass
+Pass	Removing transition and changing width applies change immediately
diff --git a/Tests/LibWeb/Text/input/css/transition-properties-updated-on-duration-change.html b/Tests/LibWeb/Text/input/css/transition-properties-updated-on-duration-change.html
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<div id="foo" style="transition: top 1s; top: 0px"></div>
+<script src="../include.js"></script>
+<script>
+    test(() => {
+        // Transitions properties aren't set until the second computation as a performance optimization so
+        // force a recomputation
+        foo.style.left = "0px";
+        getComputedStyle(foo).left;
+
+        foo.style.transitionDuration = "2s";
+        foo.style.top = "10px";
+        println(document.getAnimations()[0].effect.getComputedTiming().duration);
+    });
+</script>
diff --git a/Tests/LibWeb/Text/input/css/transition-properties-updated-on-property-name-changed-in-same-css-style-properties.html b/Tests/LibWeb/Text/input/css/transition-properties-updated-on-property-name-changed-in-same-css-style-properties.html
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<div id="foo" style="transition: none 1s; top: 0px"></div>
+<script src="../include.js"></script>
+<script>
+    test(() => {
+        // Transitions aren't registered until the second computation as a performance optimization so
+        // force a recomputation
+        foo.style.left = "0px";
+        getComputedStyle(foo).left;
+
+        foo.style.transitionProperty = "top";
+        foo.style.top = "10px";
+        println(document.getAnimations().length);
+    });
+</script>
diff --git a/Tests/LibWeb/Text/input/wpt-import/css/css-transitions/transition-remove-and-change-immediate.html b/Tests/LibWeb/Text/input/wpt-import/css/css-transitions/transition-remove-and-change-immediate.html
@@ -0,0 +1,51 @@
+<!doctype html>
+<html>
+<head>
+<meta charset=utf-8>
+<title>CSS Transitions Test: Removing transition and changing width applies change immediately</title>
+<meta name="assert" content="When a transition is removed and a width is changed after a previous transition completes, the change is immediate.">
+<link rel="help" href="https://drafts.csswg.org/css-transitions/#starting">
+
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script src="./support/helper.js"></script>
+
+</head>
+<body>
+<div id="log"></div>
+
+<script>
+promise_test(async t => {
+  const div = addDiv(t, {
+    style: 'transition: width 0.02s; width: 0px;'
+  });
+
+  // Flush initial state
+  getComputedStyle(div).width;
+  div.style.width = '100px';
+
+  // Wait for transition to complete
+  await waitForTransitionEnd(div);
+
+  // Verify the width after first transition
+  const afterFirst = getComputedStyle(div).width;
+  assert_equals(afterFirst, '100px', 'width should be 100px after first transition');
+
+  // Set width back to 0 and remove transition
+  div.style.width = '0px';
+  div.style.transition = '';
+
+  // Force layout update to ensure style computation
+  const afterSecond = getComputedStyle(div).width;
+  // Check width is immediately updated
+  assert_equals(
+    afterSecond,
+    '0px',
+    'width should reset to 0 immediately'
+  );
+
+}, 'Removing transition and changing width applies change immediately');
+</script>
+
+</body>
+</html>

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,6 @@ ComputedProperties::~ComputedProperties() = default;`
`55`	`55`	`void ComputedProperties::visit_edges(Visitor& visitor)`
`56`	`56`	`{`
`57`	`57`	`Base::visit_edges(visitor);`
`58`		`- visitor.visit(m_transition_property_source);`
`59`	`58`	`}`
`60`	`59`
`61`	`60`	`bool ComputedProperties::is_property_important(PropertyID property_id) const`