[css-transitions] transition property should use shortest serializa…

…tion rule

https://bugs.webkit.org/show_bug.cgi?id=266234

Reviewed by Tim Nguyen.

As discussed in web-platform-tests/wpt#43574, serializing
the `transition` shorthand should always strive to produce the shortest serialization
and omit initial values.

We update the relevant tests, including WPT, to match this expectation and add a new test
for the case where `transition-delay` is not the initial value while `transition-duration`
is since those two properties have the same `<time>` type and `transition-duration` serializes
first.

* LayoutTests/fast/css/shorthand-mismatched-list-crash-expected.txt:
* LayoutTests/fast/css/shorthand-mismatched-list-crash.html:
* LayoutTests/fast/css/transform-inline-style-expected.txt:
* LayoutTests/fast/css/transform-inline-style-remove-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/first-letter-allowed-properties-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/first-line-allowed-properties-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/parsing/transition-behavior.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/parsing/transition-computed-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/parsing/transition-computed.html:
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::addValueForAnimationPropertyToList):
(WebCore::animationShorthandValue):
(WebCore::singleTransitionValue):
(WebCore::transitionShorthandValue):
(WebCore::ComputedStyleExtractor::valueForPropertyInStyle const):
* Source/WebCore/css/ShorthandSerializer.cpp:
(WebCore::ShorthandSerializer::serializeLayered const):
* Source/WebCore/platform/animation/Animation.cpp:
(WebCore::Animation::animationsMatch const):
* Source/WebCore/platform/animation/Animation.h:
(WebCore::Animation::TransitionProperty::operator== const):

Canonical link: https://commits.webkit.org/272513@main

LayoutTests/fast/css/shorthand-mismatched-list-crash-expected.txt

@@ -1,7 +1,7 @@
 Test for WebKit bug 31559: Crash with mismatched lists and shorthands.
 
-PASS para.style.webkitTransition is "width 1s ease 0s, left 1s ease 0s, all 1s ease 0s"
-PASS para.style.webkitTransition is "width 1s ease 0s, left 1s ease 0s, top 0s ease 0s"
+PASS para.style.webkitTransition is "width 1s, left 1s, 1s"
+PASS para.style.webkitTransition is "width 1s, left 1s, top"
 PASS successfullyParsed is true
 
 TEST COMPLETE

LayoutTests/fast/css/shorthand-mismatched-list-crash.html

@@ -17,14 +17,14 @@
   para.style.webkitTransition = 'width 1s, left 1s, top 1s';
   para.style.webkitTransitionProperty = 'width, left';
 
-  shouldBeEqualToString("para.style.webkitTransition", "width 1s ease 0s, left 1s ease 0s, all 1s ease 0s");
+  shouldBeEqualToString("para.style.webkitTransition", "width 1s, left 1s, 1s");
 
   // Test shorter shorthand
   para.style.webkitTransition = 'width 1s, left 1s';
   para.style.webkitTransitionProperty = 'width, left, top';
 
   // the next line will crash
-  shouldBeEqualToString("para.style.webkitTransition", "width 1s ease 0s, left 1s ease 0s, top 0s ease 0s");
+  shouldBeEqualToString("para.style.webkitTransition", "width 1s, left 1s, top");
 </script>
 <script src="../../resources/js-test-post.js"></script>
 </body>

LayoutTests/fast/css/transform-inline-style-expected.txt

@@ -1,6 +1,6 @@
 Tests reading inline style of transition, and -webkit-transform-origin
 https://bugs.webkit.org/show_bug.cgi?id=22594
 
-style: all 1s ease 0s, left 3s cubic-bezier(0.2, 0.3, 0.6, 0.8) 2s
+style: 1s, left 3s cubic-bezier(0.2, 0.3, 0.6, 0.8) 2s
 style: left 30%
 

LayoutTests/fast/css/transform-inline-style-remove-expected.txt

@@ -3,7 +3,7 @@ https://bugs.webkit.org/show_bug.cgi?id=22605
 
 All "after" results should be null/empty
 
-transition (before): all 1s ease 0s, left 3s cubic-bezier(0.2, 0.3, 0.6, 0.8) 2s
+transition (before): 1s, left 3s cubic-bezier(0.2, 0.3, 0.6, 0.8) 2s
 transition property (before): all, left
 transition duration (before): 1s, 3s
 transition timing function (before): ease, cubic-bezier(0.2, 0.3, 0.6, 0.8)

LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/first-letter-allowed-properties-expected.txt

@@ -32,7 +32,7 @@ PASS textUnderlinePosition should be applied to first-letter pseudo elements.
 PASS verticalAlign should be applied to first-letter pseudo elements.
 PASS wordSpacing should be applied to first-letter pseudo elements.
 FAIL position should not be applied to first-letter pseudo elements. assert_equals: expected "static" but got "absolute"
-FAIL transition should not be applied to first-letter pseudo elements. assert_equals: expected "all 0s ease 0s" but got "transform 1s ease 0s"
+FAIL transition should not be applied to first-letter pseudo elements. assert_equals: expected "all" but got "transform 1s"
 PASS transform should not be applied to first-letter pseudo elements.
 FAIL wordBreak should not be applied to first-letter pseudo elements. assert_equals: expected "normal" but got "break-all"
 

LayoutTests/imported/w3c/web-platform-tests/css/css-pseudo/first-line-allowed-properties-expected.txt

@@ -34,7 +34,7 @@ FAIL borderRadius should not be applied to first-line pseudo elements. assert_eq
 FAIL margin should not be applied to first-line pseudo elements. assert_equals: expected "0px" but got "10px 20px 30px 40px"
 FAIL padding should not be applied to first-line pseudo elements. assert_equals: expected "0px" but got "10px 20px 30px 40px"
 FAIL position should not be applied to first-line pseudo elements. assert_equals: expected "static" but got "absolute"
-FAIL transition should not be applied to first-line pseudo elements. assert_equals: expected "all 0s ease 0s" but got "transform 1s ease 0s"
+FAIL transition should not be applied to first-line pseudo elements. assert_equals: expected "all" but got "transform 1s"
 PASS transform should not be applied to first-line pseudo elements.
 FAIL wordBreak should not be applied to first-line pseudo elements. assert_equals: expected "normal" but got "break-all"
 

LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/parsing/transition-behavior.html

@@ -13,17 +13,17 @@
 test_valid_value('transition-behavior', 'allow-discrete');
 test_computed_value('transition-behavior', 'allow-discrete');
 
-test_valid_value('transition', 'allow-discrete display', 'display 0s ease 0s allow-discrete');
-test_computed_value('transition', 'allow-discrete display', 'display 0s ease 0s allow-discrete');
+test_valid_value('transition', 'allow-discrete display', 'display allow-discrete');
+test_computed_value('transition', 'allow-discrete display', 'display allow-discrete');
 
-test_valid_value('transition', 'allow-discrete display 3s', 'display 3s ease 0s allow-discrete');
-test_computed_value('transition', 'allow-discrete display 3s', 'display 3s ease 0s allow-discrete');
+test_valid_value('transition', 'allow-discrete display 3s', 'display 3s allow-discrete');
+test_computed_value('transition', 'allow-discrete display 3s', 'display 3s allow-discrete');
 
-test_valid_value('transition', 'allow-discrete display 3s 1s', 'display 3s ease 1s allow-discrete');
-test_computed_value('transition', 'allow-discrete display 3s 1s', 'display 3s ease 1s allow-discrete');
+test_valid_value('transition', 'allow-discrete display 3s 1s', 'display 3s 1s allow-discrete');
+test_computed_value('transition', 'allow-discrete display 3s 1s', 'display 3s 1s allow-discrete');
 
-test_valid_value('transition', 'allow-discrete display 3s ease-in-out', 'display 3s ease-in-out 0s allow-discrete');
-test_computed_value('transition', 'allow-discrete display 3s ease-in-out', 'display 3s ease-in-out 0s allow-discrete');
+test_valid_value('transition', 'allow-discrete display 3s ease-in-out', 'display 3s ease-in-out allow-discrete');
+test_computed_value('transition', 'allow-discrete display 3s ease-in-out', 'display 3s ease-in-out allow-discrete');
 
 test_valid_value('transition', 'allow-discrete display 3s ease-in-out 1s', 'display 3s ease-in-out 1s allow-discrete');
 test_computed_value('transition', 'allow-discrete display 3s ease-in-out 1s', 'display 3s ease-in-out 1s allow-discrete');
@@ -44,14 +44,14 @@
 // Serialization with multiple shorthands, including different order
 test_valid_value('transition',
   'allow-discrete display, normal opacity, color',
-  'display 0s ease 0s allow-discrete, opacity 0s ease 0s, color 0s ease 0s');
+  'display allow-discrete, opacity, color');
 test_computed_value('transition',
   'allow-discrete display, normal opacity, color',
-  'display 0s ease 0s allow-discrete, opacity 0s ease 0s, color 0s ease 0s');
+  'display allow-discrete, opacity, color');
 test_valid_value('transition',
   'normal opacity, color, allow-discrete display',
-  'opacity 0s ease 0s, color 0s ease 0s, display 0s ease 0s allow-discrete');
+  'opacity, color, display allow-discrete');
 test_computed_value('transition',
   'normal opacity, color, allow-discrete display',
-  'opacity 0s ease 0s, color 0s ease 0s, display 0s ease 0s allow-discrete');
+  'opacity, color, display allow-discrete');
 </script>

LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/parsing/transition-computed-expected.txt

@@ -7,4 +7,6 @@ PASS Property transition value 'none'
 PASS Property transition value 'top'
 PASS Property transition value '1s -3s cubic-bezier(0, -2, 1, 3) top'
 PASS Property transition value '1s -3s, cubic-bezier(0, -2, 1, 3) top'
+PASS Property transition value 'all, all'
+PASS Transition with a delay but no duration
 

LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/parsing/transition-computed.html

@@ -16,17 +16,26 @@
 // <time> || <easing-function> || <time>
 
 test(() => {
-  assert_equals(getComputedStyle(document.getElementById('target')).transition, "all 0s ease 0s");
+  assert_equals(getComputedStyle(document.getElementById('target')).transition, "all");
 }, "Default transition value");
 
-test_computed_value("transition", "1s", "all 1s ease 0s");
-test_computed_value("transition", "cubic-bezier(0, -2, 1, 3)", "all 0s cubic-bezier(0, -2, 1, 3) 0s");
-test_computed_value("transition", "1s -3s", "all 1s ease -3s");
-test_computed_value("transition", "none", "none 0s ease 0s");
-test_computed_value("transition", "top", "top 0s ease 0s");
+test_computed_value("transition", "1s", "1s");
+test_computed_value("transition", "cubic-bezier(0, -2, 1, 3)", "cubic-bezier(0, -2, 1, 3)");
+test_computed_value("transition", "1s -3s", "1s -3s");
+test_computed_value("transition", "none", "none");
+test_computed_value("transition", "top", "top");
 
 test_computed_value("transition", "1s -3s cubic-bezier(0, -2, 1, 3) top", "top 1s cubic-bezier(0, -2, 1, 3) -3s");
-test_computed_value("transition", "1s -3s, cubic-bezier(0, -2, 1, 3) top", "all 1s ease -3s, top 0s cubic-bezier(0, -2, 1, 3) 0s");
+test_computed_value("transition", "1s -3s, cubic-bezier(0, -2, 1, 3) top", "1s -3s, top cubic-bezier(0, -2, 1, 3)");
+
+test_computed_value("transition", "all, all", "all, all");
+
+test(() => {
+  const target = document.getElementById('target');
+  target.style.transition = "initial";
+  target.style.transitionDelay = "1s";
+  assert_equals(getComputedStyle(target).transition, "0s 1s");
+}, "Transition with a delay but no duration");
 
 // TODO: Add test with a single timing-function keyword.
 </script>

Source/WebCore/css/ComputedStyleExtractor.cpp

@@ -1576,17 +1576,11 @@ static Ref<CSSValue> valueForAnimationTimingFunction(const TimingFunction& timin
     RELEASE_ASSERT_NOT_REACHED();
 }
 
-enum IsShorthand : bool { No, Yes };
-static void addValueForAnimationPropertyToList(CSSValueListBuilder& list, CSSPropertyID property, const Animation* animation, IsShorthand isShorthand = IsShorthand::No)
+static void addValueForAnimationPropertyToList(CSSValueListBuilder& list, CSSPropertyID property, const Animation* animation)
 {
     if (property == CSSPropertyTransitionBehavior) {
-        if (animation) {
-            if (animation->isAllowsDiscreteTransitionsFilled())
-                return;
-            if (isShorthand == IsShorthand::Yes && animation->allowsDiscreteTransitions() == Animation::initialAllowsDiscreteTransitions())
-                return;
-        }
-        list.append(valueForTransitionBehavior(animation ? animation->allowsDiscreteTransitions() : Animation::initialAllowsDiscreteTransitions()));
+        if (!animation || !animation->isAllowsDiscreteTransitionsFilled())
+            list.append(valueForTransitionBehavior(animation ? animation->allowsDiscreteTransitions() : Animation::initialAllowsDiscreteTransitions()));
     } else if (property == CSSPropertyAnimationDuration || property == CSSPropertyTransitionDuration) {
         if (!animation || !animation->isDurationFilled())
             list.append(valueForAnimationDuration(animation ? animation->duration() : Animation::initialDuration()));
@@ -1646,7 +1640,7 @@ static Ref<CSSValueList> animationShorthandValue(CSSPropertyID property, const A
     auto addAnimation = [&](Ref<Animation> animation) {
         CSSValueListBuilder childList;
         for (auto longhand : shorthandForProperty(property))
-            addValueForAnimationPropertyToList(childList, longhand, animation.ptr(), IsShorthand::Yes);
+            addValueForAnimationPropertyToList(childList, longhand, animation.ptr());
         parentList.append(CSSValueList::createSpaceSeparated(WTFMove(childList)));
     };
     if (animationList && !animationList->isEmpty()) {
@@ -1657,6 +1651,44 @@ static Ref<CSSValueList> animationShorthandValue(CSSPropertyID property, const A
     return CSSValueList::createCommaSeparated(WTFMove(parentList));
 }
 
+static Ref<CSSValue> singleTransitionValue(const Animation& transition)
+{
+    static NeverDestroyed<Ref<TimingFunction>> initialTimingFunction(Animation::initialTimingFunction());
+
+    // If we have a transition-delay but no transition-duration set, we must serialze
+    // the transition-duration because they're both <time> values and transition-delay
+    // comes first.
+    auto showsDelay = transition.delay() != Animation::initialDelay();
+    auto showsDuration = showsDelay || transition.duration() != Animation::initialDuration();
+
+    CSSValueListBuilder list;
+    if (transition.property() != Animation::initialProperty())
+        list.append(createTransitionPropertyValue(transition));
+    if (showsDuration)
+        list.append(valueForAnimationDuration(transition.duration()));
+    if (auto* timingFunction = transition.timingFunction(); *timingFunction != initialTimingFunction.get())
+        list.append(valueForAnimationTimingFunction(*timingFunction));
+    if (showsDelay)
+        list.append(valueForAnimationDelay(transition.delay()));
+    if (transition.allowsDiscreteTransitions() != Animation::initialAllowsDiscreteTransitions())
+        list.append(valueForTransitionBehavior(transition.allowsDiscreteTransitions()));
+    if (list.isEmpty())
+        return CSSPrimitiveValue::create(CSSValueAll);
+    return CSSValueList::createSpaceSeparated(WTFMove(list));
+}
+
+static Ref<CSSValue> transitionShorthandValue(const AnimationList* transitions)
+{
+    if (!transitions || transitions->isEmpty())
+        return CSSPrimitiveValue::create(CSSValueAll);
+
+    CSSValueListBuilder list;
+    for (auto& transition : *transitions)
+        list.append(singleTransitionValue(transition));
+    ASSERT(!list.isEmpty());
+    return CSSValueList::createCommaSeparated(WTFMove(list));
+}
+
 static Ref<CSSValue> createLineBoxContainValue(OptionSet<LineBoxContain> lineBoxContain)
 {
     if (!lineBoxContain)
@@ -4217,7 +4249,7 @@ RefPtr<CSSValue> ComputedStyleExtractor::valueForPropertyInStyle(const RenderSty
     case CSSPropertyTransitionProperty:
         return valueListForAnimationOrTransitionProperty(propertyID, style.transitions());
     case CSSPropertyTransition:
-        return animationShorthandValue(propertyID, style.transitions());
+        return transitionShorthandValue(style.transitions());
     case CSSPropertyPointerEvents:
         return createConvertingToCSSValueID(style.pointerEvents());
     case CSSPropertyWebkitLineGrid:

Source/WebCore/css/ShorthandSerializer.cpp

@@ -641,13 +641,6 @@ String ShorthandSerializer::serializeLayered() const
         for (unsigned j = 0; j < length(); j++) {
             auto longhand = longhandProperty(j);
 
-            // We always want to force the serialization of the transition longhands,
-            // except for transition-behavior which should only serialize if non-default.
-            if (m_shorthand.id() == CSSPropertyTransition) {
-                if (longhand != CSSPropertyTransitionBehavior)
-                    layerValues.skip(j) = false;
-            }
-
             // A single box value sets both background-origin and background-clip.
             // A single geometry-box value sets both mask-origin and mask-clip.
             // A single geometry-box value sets both mask-origin and -webkit-mask-clip.

Source/WebCore/platform/animation/Animation.cpp

@@ -140,7 +140,7 @@ bool Animation::animationsMatch(const Animation& other, bool matchProperties) co
     if (!result)
         return false;
 
-    return !matchProperties || (m_property.mode == other.m_property.mode && m_property.animatableProperty == other.m_property.animatableProperty && m_propertySet == other.m_propertySet);
+    return !matchProperties || (m_property == other.m_property && m_propertySet == other.m_propertySet);
 }
 
 auto Animation::initialName() -> const Style::ScopedName&

Source/WebCore/platform/animation/Animation.h

@@ -116,6 +116,7 @@ class Animation : public RefCounted<Animation> {
     struct TransitionProperty {
         TransitionMode mode;
         AnimatableCSSProperty animatableProperty;
+        bool operator==(const TransitionProperty& o) const { return mode == o.mode && animatableProperty == o.animatableProperty; }
     };
 
     enum class Direction : uint8_t {