css/css-transitions/pseudo-elements-002.html WPT is a failure

[Antoine Quint]

The WPT at css/css-transitions/pseudo-elements-002.html is a failure. It's a simple test, which looks like this:

    <style>
      #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 }
    </style>

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

    <script>

      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.");
    </script>

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.

[Antoine Quint]

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.

[Antoine Quint]

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.

[Antoine Quint]

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

[Antoine Quint]

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.

[Antoine Quint]

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.

[Antoine Quint]

Created attachment 448913 [details]
Simplified working testcase (color)

The same test using the "color" property works.

[Antoine Quint]

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.

[Antoine Quint]

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;

[Antoine Quint]

Created attachment 448922 [details]
Patch

[Antoine Quint]

Created attachment 448933 [details]
Patch for landing

[Antoine Quint]

Created attachment 448943 [details]
Patch for landing

[Antoine Quint]

Committed r287926 (245959@trunk): <https://commits.webkit.org/245959@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/87461742>

[Antoine Quint]

We'll clean up the added use of PseudoElement in bug 235158.