RESOLVED FIXED 234987
css/css-transitions/pseudo-elements-002.html WPT is a failure
https://bugs.webkit.org/show_bug.cgi?id=234987
Summary css/css-transitions/pseudo-elements-002.html WPT is a failure
Antoine Quint
Reported 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: <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.
Attachments
Simplified failing testcase (height) (1.23 KB, text/html)
2022-01-12 00:37 PST, Antoine Quint
no flags
Simplified working testcase (color) (1.03 KB, text/html)
2022-01-12 00:38 PST, Antoine Quint
no flags
Patch (3.41 KB, patch)
2022-01-12 03:34 PST, Antoine Quint
koivisto: review+
ews-feeder: commit-queue-
Patch for landing (5.23 KB, patch)
2022-01-12 05:08 PST, Antoine Quint
ews-feeder: commit-queue-
Patch for landing (4.72 KB, patch)
2022-01-12 07:37 PST, Antoine Quint
no flags
Antoine Quint
Comment 1 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.
Antoine Quint
Comment 2 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.
Antoine Quint
Comment 3 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
Antoine Quint
Comment 4 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.
Antoine Quint
Comment 5 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.
Antoine Quint
Comment 6 2022-01-12 00:38:34 PST
Created attachment 448913 [details] Simplified working testcase (color) The same test using the "color" property works.
Antoine Quint
Comment 7 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.
Antoine Quint
Comment 8 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;
Antoine Quint
Comment 9 2022-01-12 03:34:37 PST
Antoine Quint
Comment 10 2022-01-12 05:08:46 PST
Created attachment 448933 [details] Patch for landing
Antoine Quint
Comment 11 2022-01-12 07:37:11 PST
Created attachment 448943 [details] Patch for landing
Antoine Quint
Comment 12 2022-01-12 08:55:10 PST
Radar WebKit Bug Importer
Comment 13 2022-01-12 08:56:17 PST
Antoine Quint
Comment 14 2022-01-12 14:33:11 PST
We'll clean up the added use of PseudoElement in bug 235158.
Note You need to log in before you can comment on or make changes to this bug.