LibWeb: Prefer transitioned property values over important

CSS transitions have a higher precedence in the cascade than important
properties

Author: Calme1709

Libraries/LibWeb/CSS/ComputedProperties.cpp

@@ -92,6 +92,14 @@ bool ComputedProperties::is_animated_property_inherited(PropertyID property_id)
     return m_animated_property_inherited[n / 8] & (1 << (n % 8));
 }
 
+bool ComputedProperties::is_animated_property_result_of_transition(PropertyID property_id) const
+{
+    VERIFY(property_id >= first_longhand_property_id && property_id <= last_longhand_property_id);
+
+    size_t n = to_underlying(property_id) - to_underlying(first_longhand_property_id);
+    return m_animated_property_result_of_transition[n / 8] & (1 << (n % 8));
+}
+
 void ComputedProperties::set_property_inherited(PropertyID property_id, Inherited inherited)
 {
     VERIFY(property_id >= first_longhand_property_id && property_id <= last_longhand_property_id);
@@ -114,6 +122,17 @@ void ComputedProperties::set_animated_property_inherited(PropertyID property_id,
         m_animated_property_inherited[n / 8] &= ~(1 << (n % 8));
 }
 
+void ComputedProperties::set_animated_property_result_of_transition(PropertyID property_id, AnimatedPropertyResultOfTransition animated_value_result_of_transition)
+{
+    VERIFY(property_id >= first_longhand_property_id && property_id <= last_longhand_property_id);
+
+    size_t n = to_underlying(property_id) - to_underlying(first_longhand_property_id);
+    if (animated_value_result_of_transition == AnimatedPropertyResultOfTransition::Yes)
+        m_animated_property_result_of_transition[n / 8] |= (1 << (n % 8));
+    else
+        m_animated_property_result_of_transition[n / 8] &= ~(1 << (n % 8));
+}
+
 void ComputedProperties::set_property(PropertyID id, NonnullRefPtr<StyleValue const> value, Inherited inherited, Important important)
 {
     VERIFY(id >= first_longhand_property_id && id <= last_longhand_property_id);
@@ -149,10 +168,11 @@ void ComputedProperties::set_display_before_box_type_transformation(Display valu
     m_display_before_box_type_transformation = value;
 }
 
-void ComputedProperties::set_animated_property(PropertyID id, NonnullRefPtr<StyleValue const> value, Inherited inherited)
+void ComputedProperties::set_animated_property(PropertyID id, NonnullRefPtr<StyleValue const> value, AnimatedPropertyResultOfTransition animated_value_result_of_transition, Inherited inherited)
 {
     m_animated_property_values.set(id, move(value));
     set_animated_property_inherited(id, inherited);
+    set_animated_property_result_of_transition(id, animated_value_result_of_transition);
 }
 
 void ComputedProperties::remove_animated_property(PropertyID id)
@@ -169,8 +189,8 @@ StyleValue const& ComputedProperties::property(PropertyID property_id, WithAnima
 {
     VERIFY(property_id >= first_longhand_property_id && property_id <= last_longhand_property_id);
 
-    // Important properties override animated properties
-    if (!is_property_important(property_id) && return_animated_value == WithAnimationsApplied::Yes) {
+    // Important properties override animated but not transitioned properties
+    if ((!is_property_important(property_id) || is_animated_property_result_of_transition(property_id)) && return_animated_value == WithAnimationsApplied::Yes) {
         if (auto animated_value = m_animated_property_values.get(property_id); animated_value.has_value())
             return *animated_value.value();
     }

Libraries/LibWeb/CSS/ComputedProperties.h

@@ -26,6 +26,11 @@
 
 namespace Web::CSS {
 
+enum class AnimatedPropertyResultOfTransition : u8 {
+    No,
+    Yes
+};
+
 class WEB_API ComputedProperties final : public JS::Cell {
     GC_CELL(ComputedProperties, JS::Cell);
     GC_DECLARE_ALLOCATOR(ComputedProperties);
@@ -55,13 +60,15 @@ class WEB_API ComputedProperties final : public JS::Cell {
     bool is_property_important(PropertyID property_id) const;
     bool is_property_inherited(PropertyID property_id) const;
     bool is_animated_property_inherited(PropertyID property_id) const;
+    bool is_animated_property_result_of_transition(PropertyID property_id) const;
     void set_property_important(PropertyID, Important);
     void set_property_inherited(PropertyID, Inherited);
     void set_animated_property_inherited(PropertyID, Inherited);
+    void set_animated_property_result_of_transition(PropertyID, AnimatedPropertyResultOfTransition);
 
     void set_property(PropertyID, NonnullRefPtr<StyleValue const> value, Inherited = Inherited::No, Important = Important::No);
     void set_property_without_modifying_flags(PropertyID, NonnullRefPtr<StyleValue const> value);
-    void set_animated_property(PropertyID, NonnullRefPtr<StyleValue const> value, Inherited = Inherited::No);
+    void set_animated_property(PropertyID, NonnullRefPtr<StyleValue const> value, AnimatedPropertyResultOfTransition, Inherited = Inherited::No);
     void remove_animated_property(PropertyID);
     enum class WithAnimationsApplied {
         No,
@@ -294,6 +301,7 @@ class WEB_API ComputedProperties final : public JS::Cell {
     Array<u8, ceil_div(number_of_longhand_properties, 8uz)> m_property_important {};
     Array<u8, ceil_div(number_of_longhand_properties, 8uz)> m_property_inherited {};
     Array<u8, ceil_div(number_of_longhand_properties, 8uz)> m_animated_property_inherited {};
+    Array<u8, ceil_div(number_of_longhand_properties, 8uz)> m_animated_property_result_of_transition {};
 
     HashMap<PropertyID, NonnullRefPtr<StyleValue const>> m_animated_property_values;
 

Libraries/LibWeb/CSS/StyleComputer.cpp

@@ -950,7 +950,7 @@ void StyleComputer::collect_animation_into(DOM::AbstractElement abstract_element
                 auto const& specified_value_with_css_wide_keywords_applied = [&]() -> StyleValue const& {
                     if (longhand_value.is_inherit() || (longhand_value.is_unset() && is_inherited_property(longhand_id))) {
                         if (auto inherited_animated_value = get_animated_inherit_value(longhand_id, abstract_element); inherited_animated_value.has_value())
-                            return inherited_animated_value.value();
+                            return inherited_animated_value->value;
 
                         return get_non_animated_inherit_value(longhand_id, abstract_element);
                     }
@@ -1070,6 +1070,8 @@ void StyleComputer::collect_animation_into(DOM::AbstractElement abstract_element
         VERIFY_NOT_REACHED();
     };
 
+    auto is_result_of_transition = animation->is_css_transition() ? AnimatedPropertyResultOfTransition::Yes : AnimatedPropertyResultOfTransition::No;
+
     auto start_composite_operation = to_composite_operation(keyframe_values.composite);
     auto end_composite_operation = to_composite_operation(keyframe_end_values.composite);
 
@@ -1079,7 +1081,7 @@ void StyleComputer::collect_animation_into(DOM::AbstractElement abstract_element
 
         if (!resolved_end_property) {
             if (resolved_start_property) {
-                computed_properties.set_animated_property(it.key, *resolved_start_property);
+                computed_properties.set_animated_property(it.key, *resolved_start_property, is_result_of_transition);
                 dbgln_if(LIBWEB_CSS_ANIMATION_DEBUG, "No end property for property {}, using {}", string_from_property_id(it.key), resolved_start_property->to_string(SerializationMode::Normal));
             }
             continue;
@@ -1094,7 +1096,9 @@ void StyleComputer::collect_animation_into(DOM::AbstractElement abstract_element
         auto start = resolved_start_property.release_nonnull();
         auto end = resolved_end_property.release_nonnull();
 
-        if (computed_properties.is_property_important(it.key)) {
+        // OPTIMIZATION: Values resulting from animations other than CSS transitions are overriden by important
+        //               properties so there's no need to calculate them
+        if (!animation->is_css_transition() && computed_properties.is_property_important(it.key)) {
             continue;
         }
 
@@ -1107,11 +1111,11 @@ void StyleComputer::collect_animation_into(DOM::AbstractElement abstract_element
 
         if (auto next_value = interpolate_property(*effect->target(), it.key, *start, *end, progress_in_keyframe, AllowDiscrete::Yes)) {
             dbgln_if(LIBWEB_CSS_ANIMATION_DEBUG, "Interpolated value for property {} at {}: {} -> {} = {}", string_from_property_id(it.key), progress_in_keyframe, start->to_string(SerializationMode::Normal), end->to_string(SerializationMode::Normal), next_value->to_string(SerializationMode::Normal));
-            computed_properties.set_animated_property(it.key, *next_value);
+            computed_properties.set_animated_property(it.key, *next_value, is_result_of_transition);
         } else {
             // If interpolate_property() fails, the element should not be rendered
             dbgln_if(LIBWEB_CSS_ANIMATION_DEBUG, "Interpolated value for property {} at {}: {} -> {} is invalid", string_from_property_id(it.key), progress_in_keyframe, start->to_string(SerializationMode::Normal), end->to_string(SerializationMode::Normal));
-            computed_properties.set_animated_property(PropertyID::Visibility, KeywordStyleValue::create(Keyword::Hidden));
+            computed_properties.set_animated_property(PropertyID::Visibility, KeywordStyleValue::create(Keyword::Hidden), is_result_of_transition);
         }
     }
 }
@@ -1591,15 +1595,20 @@ NonnullRefPtr<StyleValue const> StyleComputer::get_non_animated_inherit_value(Pr
     return parent_element->computed_properties()->property(property_id, ComputedProperties::WithAnimationsApplied::No);
 }
 
-Optional<NonnullRefPtr<StyleValue const>> StyleComputer::get_animated_inherit_value(PropertyID property_id, DOM::AbstractElement abstract_element)
+Optional<StyleComputer::AnimatedInheritValue> StyleComputer::get_animated_inherit_value(PropertyID property_id, DOM::AbstractElement abstract_element)
 {
     auto parent_element = abstract_element.element_to_inherit_style_from();
 
     if (!parent_element.has_value() || !parent_element->computed_properties())
         return {};
 
     if (auto animated_value = parent_element->computed_properties()->animated_property_values().get(property_id); animated_value.has_value())
-        return *animated_value.value();
+        return AnimatedInheritValue {
+            .value = *animated_value.value(),
+            .is_result_of_transition = parent_element->computed_properties()->is_animated_property_result_of_transition(property_id)
+                ? AnimatedPropertyResultOfTransition::Yes
+                : AnimatedPropertyResultOfTransition::No
+        };
 
     return {};
 }
@@ -2508,7 +2517,6 @@ GC::Ref<ComputedProperties> StyleComputer::compute_properties(DOM::AbstractEleme
         auto property_id = static_cast<CSS::PropertyID>(i);
         auto value = cascaded_properties.property(property_id);
         auto inherited = ComputedProperties::Inherited::No;
-        Optional<NonnullRefPtr<StyleValue const>> animated_value;
 
         // NOTE: We've already handled font-size above.
         if (property_id == PropertyID::FontSize && !value && new_font_size)
@@ -2530,17 +2538,17 @@ GC::Ref<ComputedProperties> StyleComputer::compute_properties(DOM::AbstractEleme
 
         // FIXME: Logical properties should inherit from their parent's equivalent unmapped logical property.
         if (should_inherit) {
-            value = get_non_animated_inherit_value(property_id, abstract_element);
-            animated_value = get_animated_inherit_value(property_id, abstract_element);
             inherited = ComputedProperties::Inherited::Yes;
+            value = get_non_animated_inherit_value(property_id, abstract_element);
+
+            if (auto animated_value = get_animated_inherit_value(property_id, abstract_element); animated_value.has_value())
+                computed_style->set_animated_property(property_id, animated_value->value, animated_value->is_result_of_transition, ComputedProperties::Inherited::Yes);
         }
 
         if (!value || value->is_initial() || value->is_unset())
             value = property_initial_value(property_id);
 
         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));

Libraries/LibWeb/CSS/StyleComputer.h

@@ -110,7 +110,11 @@ class WEB_API StyleComputer final : public GC::Cell {
 public:
     static void for_each_property_expanding_shorthands(PropertyID, StyleValue const&, Function<void(PropertyID, StyleValue const&)> const& set_longhand_property);
     static NonnullRefPtr<StyleValue const> get_non_animated_inherit_value(PropertyID, DOM::AbstractElement);
-    static Optional<NonnullRefPtr<StyleValue const>> get_animated_inherit_value(PropertyID, DOM::AbstractElement);
+    struct AnimatedInheritValue {
+        NonnullRefPtr<StyleValue const> value;
+        AnimatedPropertyResultOfTransition is_result_of_transition;
+    };
+    static Optional<AnimatedInheritValue> get_animated_inherit_value(PropertyID, DOM::AbstractElement);
 
     static Optional<String> user_agent_style_sheet_source(StringView name);
 

Libraries/LibWeb/DOM/Element.cpp

@@ -851,7 +851,7 @@ CSS::RequiredInvalidationAfterStyleChange Element::recompute_inherited_style()
 
         if (computed_properties->is_animated_property_inherited(property_id) || !computed_properties->animated_property_values().contains(property_id)) {
             if (auto new_animated_value = CSS::StyleComputer::get_animated_inherit_value(property_id, { *this }); new_animated_value.has_value())
-                computed_properties->set_animated_property(property_id, new_animated_value.value(), CSS::ComputedProperties::Inherited::Yes);
+                computed_properties->set_animated_property(property_id, new_animated_value->value, new_animated_value->is_result_of_transition, CSS::ComputedProperties::Inherited::Yes);
             else if (computed_properties->animated_property_values().contains(property_id))
                 computed_properties->remove_animated_property(property_id);
         }

Libraries/LibWeb/Forward.h

@@ -397,6 +397,7 @@ enum class MediaFeatureID : u8;
 enum class PropertyID : u16;
 enum class PaintOrder : u8;
 enum class ValueType : u8;
+enum class AnimatedPropertyResultOfTransition : u8;
 
 struct BackgroundLayerData;
 struct CalculationContext;

Tests/LibWeb/Text/expected/wpt-import/css/css-transitions/transition-important.txt

@@ -0,0 +1,6 @@
+Harness status: OK
+
+Found 1 tests
+
+1 Pass
+Pass	!important should not override transition

Tests/LibWeb/Text/input/wpt-import/css/css-transitions/transition-important.html

@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<title>CSS Transitions Test: !important property</title>
+<link rel="help" href="https://drafts.csswg.org/css-cascade-5/#cascade-sort">
+<script src="../../resources/testharness.js" type="text/javascript"></script>
+<script src="../../resources/testharnessreport.js" type="text/javascript"></script>
+<style>
+#target {
+    width: 200px;
+    height: 200px;
+    background-color: green;
+    transition: background-color 100s;
+}
+.red {
+    background-color: red !important;
+}
+</style>
+<div id="target"></div>
+<script>
+test(t => {
+    assert_equals(getComputedStyle(target).backgroundColor, "rgb(0, 128, 0)");
+    target.className = "red";
+    assert_equals(getComputedStyle(target).backgroundColor, "rgb(0, 128, 0)");
+}, "!important should not override transition");
+</script>