From ca4cb0656a116705cab49bfb732c75ae673659c5 Mon Sep 17 00:00:00 2001
From: Boris Chiou <boris.chiou@gmail.com>
Date: Tue, 30 Apr 2024 05:06:54 +0000
Subject: [PATCH] Bug 1834876 - Part 4: Fix the case when changing the display
 from none. r=layout-reviewers,firefox-style-system-reviewers,emilio

Add one extra branch if we have before-change style but its display
is none, and the new style is not display:none. Also, we add an extra
subtest if we use the container query to change the display property.

Differential Revision: https://phabricator.services.mozilla.com/D208572
---
 servo/components/style/matching.rs            | 17 +++---
 .../starting-style-cascade.html.ini           |  6 ---
 .../starting-style-rule-basic.html.ini        |  3 --
 .../starting-style-size-container.html        | 53 ++++++++++++++++---
 4 files changed, 57 insertions(+), 22 deletions(-)
 delete mode 100644 testing/web-platform/meta/css/css-transitions/starting-style-cascade.html.ini
 delete mode 100644 testing/web-platform/meta/css/css-transitions/starting-style-rule-basic.html.ini

diff --git a/servo/components/style/matching.rs b/servo/components/style/matching.rs
index 80f4a59f7892..739504d260ed 100644
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -426,8 +426,9 @@ trait PrivateMatchMethods: TElement {
         // Note: Basically, we have to remove transition rules because the starting style for an
         // element is the after-change style with @starting-style rules applied in addition.
         // However, we expect there is no transition rules for this element when calling this
-        // function because we do this only when we don't have before-change style and it's
-        // unlikely to have running transitions on this element.
+        // function because we do this only when we don't have before-change style or we change
+        // from display:none. In these cases, it's unlikely to have running transitions on this
+        // element.
         let mut resolver = StyleResolverForElement::new(
             *self,
             context,
@@ -460,11 +461,13 @@ trait PrivateMatchMethods: TElement {
             return None;
         }
 
-        // If we don't have before-change-style, we don't have to resolve starting style.
-        // FIXME: we may have to resolve starting style if the old style is display:none and the new
-        // style change the display property. We will have a tentative solution in the following
-        // patches.
-        if old_values.is_some() {
+        // We resolve starting style only if we don't have before-change-style, or we change from
+        // display:none.
+        if old_values.is_some()
+            && !new_styles
+                .primary_style()
+                .is_display_property_changed_from_none(old_values.map(|s| &**s))
+        {
             return None;
         }
 
diff --git a/testing/web-platform/meta/css/css-transitions/starting-style-cascade.html.ini b/testing/web-platform/meta/css/css-transitions/starting-style-cascade.html.ini
deleted file mode 100644
index d1f89b8c15aa..000000000000
--- a/testing/web-platform/meta/css/css-transitions/starting-style-cascade.html.ini
+++ /dev/null
@@ -1,6 +0,0 @@
-[starting-style-cascade.html]
-  [@starting-style with higher specificity]
-    expected: FAIL
-
-  [Starting style inheriting from parent's after-change style while parent transitioning]
-    expected: FAIL
diff --git a/testing/web-platform/meta/css/css-transitions/starting-style-rule-basic.html.ini b/testing/web-platform/meta/css/css-transitions/starting-style-rule-basic.html.ini
deleted file mode 100644
index 00a49cb86cc1..000000000000
--- a/testing/web-platform/meta/css/css-transitions/starting-style-rule-basic.html.ini
+++ /dev/null
@@ -1,3 +0,0 @@
-[starting-style-rule-basic.html]
-  [Triggered transition from display:none to display:block]
-    expected: FAIL
diff --git a/testing/web-platform/tests/css/css-transitions/starting-style-size-container.html b/testing/web-platform/tests/css/css-transitions/starting-style-size-container.html
index 92ad6e61251e..1ad609dd9093 100644
--- a/testing/web-platform/tests/css/css-transitions/starting-style-size-container.html
+++ b/testing/web-platform/tests/css/css-transitions/starting-style-size-container.html
@@ -5,32 +5,58 @@
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="/css/css-transitions/support/helper.js"></script>
-<div id="container" style="width: 200px">
-  <div id="target" style="display: none"></div>
-</div>
+<body>
+</body>
 <style>
   #container {
     container-type: inline-size;
+    width: 100px;
   }
   #target {
     transition-property: background-color;
     transition-duration: 100s;
     transition-timing-function: steps(2, start);
     background-color: lime;
+    display: none;
   }
   @container (width > 300px) {
     @starting-style {
       #target { background-color: white; }
     }
   }
-  @container (width < 300px) {
+  @container ((width > 200px) and (width < 300px)) {
+    #target {
+      display: block;
+    }
+    @starting-style {
+      #target { background-color: white; }
+    }
+  }
+  @container (width < 200px) {
     @starting-style {
       #target { background-color: red; }
     }
   }
 </style>
 <script>
+  function setup(test) {
+    let container = document.createElement("div");
+    container.id = "container";
+    document.body.appendChild(container);
+
+    let target = document.createElement("div");
+    target.id = "target";
+    container.appendChild(target);
+
+    test.add_cleanup(() => {
+      target.remove();
+      container.remove();
+    });
+    return [container, target];
+  }
+
   promise_test(async t => {
+    let [container, target] = setup(t);
     await waitForAnimationFrames(2);
     assert_equals(getComputedStyle(target).backgroundColor, "rgb(0, 255, 0)",
                   "No transition while display:none");
@@ -38,6 +64,21 @@
     target.style.display = "block";
     await waitForAnimationFrames(2);
     assert_equals(getComputedStyle(target).backgroundColor, "rgb(128, 255, 128)",
-                  "@starting-style based on the size query evaluation from the same frame");
-  }, "Triggered transition from first style update based on up-to-date container query");
+                  "@starting-style based on the size query evaluation from " +
+                  "the same frame");
+  }, "Triggered transition from first style update based on up-to-date " +
+     "container query");
+
+  promise_test(async t => {
+    let [container, target] = setup(t);
+    await waitForAnimationFrames(2);
+    assert_equals(getComputedStyle(target).backgroundColor, "rgb(0, 255, 0)",
+                  "No transition while display:none");
+    container.style.width = "250px";
+    await waitForAnimationFrames(2);
+    assert_equals(getComputedStyle(target).backgroundColor, "rgb(128, 255, 128)",
+                  "@starting-style based on the size query evaluation from " +
+                  "the same frame");
+  }, "Triggered transition from the display change inside the up-to-date " +
+     "container query");
 </script>