Bug 234987 - css/css-transitions/pseudo-elements-002.html WPT is a failure
Summary: css/css-transitions/pseudo-elements-002.html WPT is a failure
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
Keywords: InRadar
Depends on:
Blocks: 235130
  Show dependency treegraph
Reported: 2022-01-07 14:29 PST by Antoine Quint
Modified: 2022-01-12 14:33 PST (History)
10 users (show)

See Also:

Simplified failing testcase (height) (1.23 KB, text/html)
2022-01-12 00:37 PST, Antoine Quint
no flags Details
Simplified working testcase (color) (1.03 KB, text/html)
2022-01-12 00:38 PST, Antoine Quint
no flags Details
Patch (3.41 KB, patch)
2022-01-12 03:34 PST, Antoine Quint
koivisto: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (5.23 KB, patch)
2022-01-12 05:08 PST, Antoine Quint
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (4.72 KB, patch)
2022-01-12 07:37 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2022-01-07 14:29:22 PST
The WPT at css/css-transitions/pseudo-elements-002.html is a failure. It's a simple test, which looks like this:

      #inner::before {
        content: "This text should transition from red to green.";
        height: 100px;
        transition: height steps(2, start) 1s;
      .flex #inner::before {
        height: 300px;
      .flex { display: flex }

    <div id="outer">
      <div id="inner"></div>


      test(() => {
          assert_equals(getComputedStyle(inner, "::before").height, "100px");
          outer.className = "flex";
          assert_equals(getComputedStyle(inner, "::before").height, "200px");
      }, "Check that transitions run on a pseudo element whose ancestor changes display type.");

From what I can see via logging, we:

1. create a PseudoElement as we query the ::before computed style the first time
2. set the CSS "flex" class
3. return the ::before computed style for the second failed assertion
4. create the "height" transition from 100px to 300px (too late, the previous style check did not account for it)
4. destroy the PseudoElement, terminating the transition
5. create a new PseudoElement
6. eventually consider running a transition again but the before and after styles for height are both 300px so do nothing

Something is not being invalidated correctly between setting the "flex" CSS class and querying the computed style. We should have created the transition while the computed style was queried, but we didn't.
Comment 1 Antoine Quint 2022-01-07 14:34:32 PST
Adding a call to `outer.getAnimations({ subtree: true });` right after the call to `outer.className = "flex";` makes the test pass since it forces a style update which otherwise doesn't quite happen just by querying the computed style. But things are still wrong because the transition is removed as the pseudo-element is destroyed and recreated and the returned value for the getAnimations() call is empty while other browsers would return the generated CSSTransition.
Comment 2 Antoine Quint 2022-01-07 14:56:16 PST
In Document::resolveStyle(), after we're done with the call to Style::TreeResolver::resolve() and have obtained our styleUpdate, we set the m_inStyleRecalc flag to false and call `updateRenderTree(WTFMove(styleUpdate))` and this is under there that we remove the pseudo-element and recreate it.
Comment 3 Antoine Quint 2022-01-08 02:21:59 PST
I guess we need to either:

1. avoid the pseudo-element teardown/rebuild,
2. avoid clearing the animation-related data structures upon its removal
3. be able to restore the animation-related data structures on it after recreation
Comment 4 Antoine Quint 2022-01-08 02:40:15 PST
Well, actually, there remains the issue that the transition isn't created as the computed style is requested after setting the "flex" class on the parent, so there may be several issues here.
Comment 5 Antoine Quint 2022-01-12 00:37:59 PST
Created attachment 448912 [details]
Simplified failing testcase (height)

Attaching a simplified standalone testcase which shows that we fail to call createAnimatedElementUpdate() when changing the class on the parent. The style is correctly updated (we get 300px) but the transition is not started.
Comment 6 Antoine Quint 2022-01-12 00:38:34 PST
Created attachment 448913 [details]
Simplified working testcase (color)

The same test using the "color" property works.
Comment 7 Antoine Quint 2022-01-12 01:01:42 PST
So for the simplified height case, when ComputedStyleExtractor::propertyValue() is called we call into updateStyleIfNeededForProperty() and hasValidStyle() is true within that function so we do _not_ end up calling document.updateStyleIfNeeded().

In the simplified color case, we return false for hasValidStyleProperty due to this clause:

    if ((isInherited || maybeExplicitlyInherited) && ancestor.styleValidity() == Style::Validity::ElementInvalid)
        return false;

… and end up calling document.updateStyleIfNeeded() which then lets the transition start.
Comment 8 Antoine Quint 2022-01-12 01:25:44 PST
If I change the "height" test to not involve pseudo-elements, then hasValidStyleProperty() returns false due to:

    if (element.styleValidity() != Style::Validity::Valid)
        return false;
Comment 9 Antoine Quint 2022-01-12 03:34:37 PST
Created attachment 448922 [details]
Comment 10 Antoine Quint 2022-01-12 05:08:46 PST
Created attachment 448933 [details]
Patch for landing
Comment 11 Antoine Quint 2022-01-12 07:37:11 PST
Created attachment 448943 [details]
Patch for landing
Comment 12 Antoine Quint 2022-01-12 08:55:10 PST
Committed r287926 (245959@trunk): <https://commits.webkit.org/245959@trunk>
Comment 13 Radar WebKit Bug Importer 2022-01-12 08:56:17 PST
Comment 14 Antoine Quint 2022-01-12 14:33:11 PST
We'll clean up the added use of PseudoElement in bug 235158.