From dc7679bf650f49effacab46914052bfca6a8f7f5 Mon Sep 17 00:00:00 2001 From: Anders Hartvoll Ruud Date: Fri, 17 Mar 2023 18:28:46 +0000 Subject: [PATCH] [CSSTransitionDiscrete] Fix custom property issues When attempting to transition a custom property with universal syntax, we hit a DCHECK in StyleBuilder::ApplyProperty because we send a CSSVariableReferenceValue, which is normally expected to be resolved at this point. However, such custom properties represent their computed values as CSSVariableReferenceValues, and the discrete transitions machinery sends those computed values to ApplyProperty without "transformations". This is expected behavior as of CL:4315803. Representing the computed value of universal custom properties with CSSVariableReferenceValue is a bit suspicious and might warrant further investigation, but this CL does not attempt to address that. Instead we alter the DCHECK to expect CSSVariableReferenceValues for relevant custom properties. Also: - Fixed usage of irrelevant custom property in the WPT token-stream-type-type-interpolation.html. - Fixed accumulation of "transitionrun" event listeners when testing multiple transitions in the same .html file (e.g. custom-property-transition-mismatched-list.html). WPTs related to and are still failing, because we don't implement interpolation for those syntaxes yet. Bug: 1399631 Change-Id: I22f0766921061216421839def84bc305edda2a41 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4334509 Commit-Queue: Anders Hartvoll Ruud Reviewed-by: Joey Arhar Cr-Commit-Position: refs/heads/main@{#1118779} --- .../properties/longhands/custom_property.cc | 4 + .../properties/longhands/custom_property.h | 3 + .../core/css/resolver/style_builder.cc | 23 +++- third_party/blink/web_tests/TestExpectations | 11 -- .../token-stream-type-interpolation.html | 2 +- ...stom-property-transition-custom-ident.html | 5 +- .../custom-property-transition-image.html | 5 +- ...m-property-transition-mismatched-list.html | 120 +++++++++++------- ...transition-transform-function-expected.txt | 2 +- ...rty-transition-transform-list-expected.txt | 2 +- .../custom-property-transition-url.html | 5 +- .../resources/utils.js | 6 +- 12 files changed, 117 insertions(+), 71 deletions(-) diff --git a/third_party/blink/renderer/core/css/properties/longhands/custom_property.cc b/third_party/blink/renderer/core/css/properties/longhands/custom_property.cc index 6062ba205609a..a9589b58338b3 100644 --- a/third_party/blink/renderer/core/css/properties/longhands/custom_property.cc +++ b/third_party/blink/renderer/core/css/properties/longhands/custom_property.cc @@ -265,4 +265,8 @@ bool CustomProperty::SupportsGuaranteedInvalid() const { return !registration_ || registration_->Syntax().IsUniversal(); } +bool CustomProperty::HasUniversalSyntax() const { + return registration_ && registration_->Syntax().IsUniversal(); +} + } // namespace blink diff --git a/third_party/blink/renderer/core/css/properties/longhands/custom_property.h b/third_party/blink/renderer/core/css/properties/longhands/custom_property.h index c12aa6b5d2ce7..ad141896fde2e 100644 --- a/third_party/blink/renderer/core/css/properties/longhands/custom_property.h +++ b/third_party/blink/renderer/core/css/properties/longhands/custom_property.h @@ -65,6 +65,9 @@ class CORE_EXPORT CustomProperty : public Variable { // https://drafts.csswg.org/css-variables/#guaranteed-invalid-value bool SupportsGuaranteedInvalid() const; + // https://drafts.css-houdini.org/css-properties-values-api-1/#universal-syntax-definition + bool HasUniversalSyntax() const; + void Trace(Visitor* visitor) const { visitor->Trace(registration_); } private: diff --git a/third_party/blink/renderer/core/css/resolver/style_builder.cc b/third_party/blink/renderer/core/css/resolver/style_builder.cc index 5f7ebd17e9d60..ad38b4c99b40d 100644 --- a/third_party/blink/renderer/core/css/resolver/style_builder.cc +++ b/third_party/blink/renderer/core/css/resolver/style_builder.cc @@ -52,6 +52,21 @@ namespace blink { +namespace { + +#if DCHECK_IS_ON() + +bool IsCustomPropertyWithUniversalSyntax(const CSSProperty& property) { + if (const auto* custom_property = DynamicTo(property)) { + return custom_property->HasUniversalSyntax(); + } + return false; +} + +#endif // DCHECK_IS_ON() + +} // namespace + void StyleBuilder::ApplyProperty(const CSSPropertyName& name, StyleResolverState& state, const CSSValue& value, @@ -87,10 +102,16 @@ void StyleBuilder::ApplyPhysicalProperty(const CSSProperty& property, CSSPropertyID id = property.PropertyID(); // These values must be resolved by StyleCascade before application: - DCHECK(!value.IsVariableReferenceValue()); DCHECK(!value.IsPendingSubstitutionValue()); DCHECK(!value.IsRevertValue()); DCHECK(!value.IsRevertLayerValue()); + // CSSVariableReferenceValues should have been resolved as well, *except* + // for custom properties with universal syntax, which actually use + // CSSVariableReferenceValue to represent their computed value. +#if DCHECK_IS_ON() + DCHECK(!value.IsVariableReferenceValue() || + IsCustomPropertyWithUniversalSyntax(property)); +#endif // DCHECK_IS_ON() DCHECK(!property.IsShorthand()) << "Shorthand property id = " << static_cast(id) diff --git a/third_party/blink/web_tests/TestExpectations b/third_party/blink/web_tests/TestExpectations index 81f0a83a0a4f9..e799e0e9e1029 100644 --- a/third_party/blink/web_tests/TestExpectations +++ b/third_party/blink/web_tests/TestExpectations @@ -3700,21 +3700,10 @@ crbug.com/1052642 external/wpt/service-workers/service-worker/unregister-immedia crbug.com/708994 http/tests/security/cross-frame-mouse-source-capabilities.html [ Pass Timeout ] # Crashes with CSSTransitionDiscrete enabled -crbug.com/1399631 animations/custom-properties/custom-ident-type-interpolation.html [ Crash Failure Pass Timeout ] -crbug.com/1399631 animations/custom-properties/custom-list-type-interpolation.html [ Crash Failure Pass Timeout ] -crbug.com/1399631 animations/custom-properties/ident-type-interpolation.html [ Crash Failure Pass Timeout ] -crbug.com/1399631 animations/custom-properties/token-stream-type-interpolation.html [ Crash Failure Pass Timeout ] -crbug.com/1399631 animations/custom-properties/url-type-interpolation.html [ Crash Failure Pass Timeout ] crbug.com/1399631 animations/interpolation/svg-fill-interpolation.html [ Crash Failure Pass Timeout ] crbug.com/1399631 animations/interpolation/svg-stroke-interpolation.html [ Crash Failure Pass Timeout ] crbug.com/1399631 external/wpt/css/css-properties-values-api/at-property-animation.html [ Crash Failure Pass Timeout ] crbug.com/1399631 external/wpt/css/css-properties-values-api/register-property.html [ Crash Failure Pass Timeout ] -crbug.com/1399631 external/wpt/css/css-properties-values-api/animation/custom-property-transition-custom-ident.html [ Crash Failure Pass Timeout ] -crbug.com/1399631 external/wpt/css/css-properties-values-api/animation/custom-property-transition-image.html [ Crash Failure Pass Timeout ] -crbug.com/1399631 external/wpt/css/css-properties-values-api/animation/custom-property-transition-mismatched-list.html [ Crash Failure Pass Timeout ] -crbug.com/1399631 external/wpt/css/css-properties-values-api/animation/custom-property-transition-transform-function.html [ Crash Failure Pass Timeout ] -crbug.com/1399631 external/wpt/css/css-properties-values-api/animation/custom-property-transition-transform-list.html [ Crash Failure Pass Timeout ] -crbug.com/1399631 external/wpt/css/css-properties-values-api/animation/custom-property-transition-url.html [ Crash Failure Pass Timeout ] crbug.com/1399631 external/wpt/css/css-sizing/animation/aspect-ratio-interpolation.html [ Crash Failure Pass Timeout ] crbug.com/1399631 external/wpt/css/filter-effects/animation/backdrop-filter-interpolation-001.html [ Crash Failure Pass Timeout ] crbug.com/1399631 external/wpt/css/filter-effects/animation/backdrop-filter-interpolation-003.html [ Crash Failure Pass Timeout ] diff --git a/third_party/blink/web_tests/animations/custom-properties/token-stream-type-interpolation.html b/third_party/blink/web_tests/animations/custom-properties/token-stream-type-interpolation.html index 84d06bb778be4..fea6b0844f3c0 100644 --- a/third_party/blink/web_tests/animations/custom-properties/token-stream-type-interpolation.html +++ b/third_party/blink/web_tests/animations/custom-properties/token-stream-type-interpolation.html @@ -37,7 +37,7 @@ }); assertNoInterpolation({ - property: '--ident', + property: '--token-stream', from: 'unset', to: 'value tokens', }); diff --git a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-custom-ident.html b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-custom-ident.html index e22d9ce882f51..73898d350dbbf 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-custom-ident.html +++ b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-custom-ident.html @@ -6,10 +6,11 @@
\ No newline at end of file diff --git a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-image.html b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-image.html index b7fc68e6b8240..30fe35db2bec8 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-image.html +++ b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-image.html @@ -6,10 +6,11 @@
\ No newline at end of file diff --git a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-mismatched-list.html b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-mismatched-list.html index 7796e36a2e38f..84473ca916afe 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-mismatched-list.html +++ b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-mismatched-list.html @@ -6,148 +6,172 @@
\ No newline at end of file diff --git a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-transform-function-expected.txt b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-transform-function-expected.txt index 8cf3e971f7908..dc59e903c892a 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-transform-function-expected.txt +++ b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-transform-function-expected.txt @@ -1,4 +1,4 @@ This is a testharness.js-based test. -FAIL A custom property of type can yield a CSS Transition assert_equals: A single animation is running expected 1 but got 0 +FAIL A custom property of type can yield a CSS Transition assert_equals: Element has the expected animated value expected "translateX(150px)" but got "translateX(200px)" Harness: the test ran to completion. diff --git a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-transform-list-expected.txt b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-transform-list-expected.txt index 57b23feb06527..2ff3d2307f90d 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-transform-list-expected.txt +++ b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-transform-list-expected.txt @@ -1,4 +1,4 @@ This is a testharness.js-based test. -FAIL A custom property of type can yield a CSS Transition assert_equals: A single animation is running expected 1 but got 0 +FAIL A custom property of type can yield a CSS Transition assert_equals: Element has the expected animated value expected "translateX(150px) scale(3)" but got "translateX(200px) scale(4)" Harness: the test ran to completion. diff --git a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-url.html b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-url.html index 785db04562237..4f22a9132598e 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-url.html +++ b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/animation/custom-property-transition-url.html @@ -6,10 +6,11 @@
\ No newline at end of file diff --git a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/resources/utils.js b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/resources/utils.js index f659e123feb9d..8cf16e367f1d1 100644 --- a/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/resources/utils.js +++ b/third_party/blink/web_tests/external/wpt/css/css-properties-values-api/resources/utils.js @@ -195,10 +195,12 @@ function transition_test(options, description) { assert_equals(getComputedStyle(target).getPropertyValue(customProperty), options.from, "Element has the expected initial value"); const transitionEventPromise = new Promise(resolve => { - target.addEventListener("transitionrun", event => { + let listener = event => { + target.removeEventListener("transitionrun", listener); assert_equals(event.propertyName, customProperty, "TransitionEvent has the expected property name"); resolve(); - }); + }; + target.addEventListener("transitionrun", listener); }); target.style.transition = `${options.transitionProperty} 1s -500ms linear`;