Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-display-4] [css-animations-2] clarify and test how display animations involving none interact with pseudo-elements #10111

Open
graouts opened this issue Mar 21, 2024 · 4 comments

Comments

@graouts
Copy link
Contributor

graouts commented Mar 21, 2024

Chrome has added support for the display property and work to add similar support in WebKit is ongoing (bug 267762). As part of this work, I noticed the Blink test fast/css-generated-content/pseudo-animation-display.html was yielding different results in Chrome and WebKit and it was not obvious to me which of the two engines, if any, is correct. Here is the relevant part of this test:

#target:after {
    display: block;
    content: "";
    background-color: blue;    
}

#target.animated:after {
    animation: anim 1ms forwards;
}

@keyframes anim {
    from { left: 0px; display: block; }
    to { left: 100px; display: none; }
}
const target = document.getElementById('target');
target.addEventListener('animationend', () => {
    const style = getComputedStyle(target, ':after');
    test(style, 'left', '100px');
    test(style, 'display', 'block');
});
target.classList.add('animated');

In WebKit, the obtained values will be (once the mentioned feature bug is fixed) left: 100px; display: none; matching the to keyframe of the forwards-filling animation. In Chrome, the values are left: auto; display: block;, indicating that the animation is not applied.

It's not immediately obvious to me what the right behavior is here, and additionally I don't think WPT has test coverage that matches this.

@graouts
Copy link
Contributor Author

graouts commented Mar 21, 2024

Cc @josepharhar who seems to have done the bulk of the work for display animation in Chrome and @flackr who has reviewed it.

@graouts
Copy link
Contributor Author

graouts commented Mar 21, 2024

If I modify the test to apply to a non-pseudo element, Chrome and WebKit agree on the results and the to animation keyframe values are reflected in the computed style after the animation completes. I think the question when it comes to the ::after pseudo-element in this case is whether display: none has any specific bearing.

webkit-commit-queue pushed a commit to graouts/WebKit that referenced this issue Mar 21, 2024
https://bugs.webkit.org/show_bug.cgi?id=271372

Reviewed by Tim Nguyen.

As part of the work to add interpolation support for the `display` property (see bug 267762) we need
to make that property animatable. We add basic support with a new wrapper in CSSPropertyAnimation.cpp,
but more work will be needed to deal with the more tricky aspects of `display: none` values encountered
before and after applying animations during style resolution.

It is expected that the WPT test `css/css-display/animations/display-interpolation.html` now has FAIL
results since this test is not aware of `display` being an animatable property. That test is superseded
by `css/css-display/animations/display-interpolation.tentative.html` but both tests currently co-exist
in the WPT suite so we retain both of them.

Likewise, we alter the WPT test `web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html`
to no longer assume the `display` property is not animatable.

Finally, the test `imported/blink/fast/css-generated-content/pseudo-animation-display.html` now fails.
That test also fails in Chrome [0] after they've added animation support for the `display` property, but our
failures aren't the same. I've filed an issue with the CSS WG [1] to discuss this and ensure we get WPT coverage.

[0] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/TestExpectations;l=3487;bpv=1;bpt=1
[1] w3c/csswg-drafts#10111

* LayoutTests/imported/blink/fast/css-generated-content/pseudo-animation-display-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/display-interpolation-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/display-none-dont-cancel.tentative-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-display/animations/display-interpolation-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-display/animations/display-interpolation.tentative-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html:
* Source/WebCore/animation/CSSPropertyAnimation.cpp:
(WebCore::blendFunc):
(WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap):
* Source/WebCore/rendering/style/RenderStyle.cpp:
(WebCore::RenderStyle::conservativelyCollectChangedAnimatableProperties const):

Canonical link: https://commits.webkit.org/276464@main
webkit-commit-queue pushed a commit to graouts/WebKit that referenced this issue Mar 22, 2024
…mation-display.html is a failure

https://bugs.webkit.org/show_bug.cgi?id=271375
rdar://125156782

Reviewed by Antti Koivisto.

Pseudo-elements represented by a `PseudoElement`, namely `::before` and `::after`, are torn down if
a `display: none` style is set. An exception to that rule is if such a pseudo-element is targeted
by a script-originated `Animation` object.

Now that the `display` property can be animated, it is possible that a `PseudoElement` is targeted
by an animation that will set `display: none`. So we now ensure that we no longer tear down such
`PseudoElement` objects.

To achieve this, first we move the `animationsAffectedDisplay` bit from `ElementUpdate` to `RenderStyle`,
renaming it as `hasDisplayAffectedByAnimations`, such that it is accessible throughout the style update
since pseudo-element style updates don't have access to the `ElementUpdate` yielded for that specific
pseudo-element, on the one yielded for their owner element.

Second, we also add a new static function `elementHasDisplayAnimationForPseudoId()` we we use within
`RenderTreeUpdater::GeneratedContent::updatePseudoElement()` to introduce another case where we don't
tear down `PseudoElement` objects if `display: none` is set: the case where such a pseudo-element is
targeted by an animation affecting the `display` property.

Finally, we can remove the now-outdated `imported/blink/fast/css-generated-content/pseudo-animation-display.html`
test and replace it with a new WPT test. That test is marked as tentative because I filed a CSS WG issue
(w3c/csswg-drafts#10111) about whether the approach taken here is correct.

* LayoutTests/imported/blink/fast/css-generated-content/pseudo-animation-display-expected.txt: Removed.
* LayoutTests/imported/blink/fast/css-generated-content/pseudo-animation-display.html: Removed.
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/display-none-dont-cancel-pseudo.tentative-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-animations/display-none-dont-cancel-pseudo.tentative.html: Added.
* Source/WebCore/rendering/style/RenderStyle.h:
* Source/WebCore/rendering/style/RenderStyleInlines.h:
(WebCore::RenderStyle::hasDisplayAffectedByAnimations const):
* Source/WebCore/rendering/style/RenderStyleSetters.h:
(WebCore::RenderStyle::setHasDisplayAffectedByAnimations):
* Source/WebCore/rendering/style/StyleMiscNonInheritedData.cpp:
(WebCore::StyleMiscNonInheritedData::StyleMiscNonInheritedData):
(WebCore::StyleMiscNonInheritedData::operator== const):
* Source/WebCore/rendering/style/StyleMiscNonInheritedData.h:
* Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:
(WebCore::RenderTreeUpdater::updateElementRenderer):
* Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:
(WebCore::keyframeEffectStackForElementAndPseudoId):
(WebCore::elementIsTargetedByKeyframeEffectRequiringPseudoElement):
(WebCore::elementHasDisplayAnimationForPseudoId):
(WebCore::RenderTreeUpdater::GeneratedContent::updatePseudoElement):
* Source/WebCore/style/StyleTreeResolver.cpp:
(WebCore::Style::TreeResolver::createAnimatedElementUpdate):
* Source/WebCore/style/StyleUpdate.h:

Canonical link: https://commits.webkit.org/276568@main
@graouts
Copy link
Contributor Author

graouts commented Mar 22, 2024

I landed a new tentative WPT test for this in web-platform-tests/wpt#45274 and made it pass in WebKit in WebKit/WebKit#26312. Happy to revisit this if this is warranted, but I think ::after and ::before should not cancel display animations if the computed value is none.

@josepharhar
Copy link
Contributor

Sorry for the slow reply

::after and ::before should not cancel display animations if the computed value is none.

This sounds reasonable to me! I filed an issue here: https://issues.chromium.org/issues/336842915

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants