Skip to content

Commit

Permalink
[CSSTransitionDiscrete] Fix custom property issues
Browse files Browse the repository at this point in the history
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 <transform-function> and <transform-list> 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 <andruud@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1118779}
  • Loading branch information
andruud authored and Chromium LUCI CQ committed Mar 17, 2023
1 parent 15059b8 commit dc7679b
Show file tree
Hide file tree
Showing 12 changed files with 117 additions and 71 deletions.
Expand Up @@ -265,4 +265,8 @@ bool CustomProperty::SupportsGuaranteedInvalid() const {
return !registration_ || registration_->Syntax().IsUniversal();
}

bool CustomProperty::HasUniversalSyntax() const {
return registration_ && registration_->Syntax().IsUniversal();
}

} // namespace blink
Expand Up @@ -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:
Expand Down
23 changes: 22 additions & 1 deletion third_party/blink/renderer/core/css/resolver/style_builder.cc
Expand Up @@ -52,6 +52,21 @@

namespace blink {

namespace {

#if DCHECK_IS_ON()

bool IsCustomPropertyWithUniversalSyntax(const CSSProperty& property) {
if (const auto* custom_property = DynamicTo<CustomProperty>(property)) {
return custom_property->HasUniversalSyntax();
}
return false;
}

#endif // DCHECK_IS_ON()

} // namespace

void StyleBuilder::ApplyProperty(const CSSPropertyName& name,
StyleResolverState& state,
const CSSValue& value,
Expand Down Expand Up @@ -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<int>(id)
Expand Down
11 changes: 0 additions & 11 deletions third_party/blink/web_tests/TestExpectations
Expand Up @@ -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 ]
Expand Down
Expand Up @@ -37,7 +37,7 @@
});

assertNoInterpolation({
property: '--ident',
property: '--token-stream',
from: 'unset',
to: 'value tokens',
});
Expand Down
Expand Up @@ -6,10 +6,11 @@
<div id="target"></div>
<script>

no_transition_test({
transition_test({
syntax: "<custom-ident>",
from: "from",
to: "to",
}, 'A custom property of type <custom-ident> cannot yield a CSS Transition');
expected: "to",
}, 'A custom property of type <custom-ident> can yield a discrete CSS Transition');

</script>
Expand Up @@ -6,10 +6,11 @@
<div id="target"></div>
<script>

no_transition_test({
transition_test({
syntax: "<image>",
from: 'url("https://example.com/from")',
to: 'url("https://example.com/to")',
}, 'A custom property of type <image> cannot yield a CSS Transition');
expected: 'url("https://example.com/to")',
}, 'A custom property of type <image> can yield a CSS Transition');

</script>
Expand Up @@ -6,148 +6,172 @@
<div id="target"></div>
<script>

no_transition_test({
transition_test({
syntax: "<angle>#",
from: '100deg, 200deg',
to: '300deg',
}, 'A custom property of type <angle># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300deg',
}, 'A custom property of type <angle># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<angle>+",
from: '100deg 200deg',
to: '300deg',
}, 'A custom property of type <angle>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300deg',
}, 'A custom property of type <angle>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<color>#",
from: 'rgb(100, 100, 100), rgb(150, 150, 150)',
to: 'rgb(200, 200, 200)',
}, 'A custom property of type <color># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: 'rgb(200, 200, 200)',
}, 'A custom property of type <color># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<color>+",
from: 'rgb(100, 100, 100) rgb(150, 150, 150)',
to: 'rgb(200, 200, 200)',
}, 'A custom property of type <color>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: 'rgb(200, 200, 200)',
}, 'A custom property of type <color>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<custom-ident>#",
from: 'foo, bar',
to: 'baz',
}, 'A custom property of type <custom-ident># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: 'baz',
}, 'A custom property of type <custom-ident># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<custom-ident>+",
from: 'foo bar',
to: 'baz',
}, 'A custom property of type <custom-ident>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: 'baz',
}, 'A custom property of type <custom-ident>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<image>#",
from: 'url("https://example.com/foo"), url("https://example.com/bar")',
to: 'url("https://example.com/to")',
}, 'A custom property of type <image># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: 'url("https://example.com/to")',
}, 'A custom property of type <image># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<image>+",
from: 'url("https://example.com/foo") url("https://example.com/bar")',
to: 'url("https://example.com/to")',
}, 'A custom property of type <image>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: 'url("https://example.com/to")',
}, 'A custom property of type <image>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<integer>#",
from: '100, 200',
to: '300',
}, 'A custom property of type <integer># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300',
}, 'A custom property of type <integer># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<integer>+",
from: '100 200',
to: '300',
}, 'A custom property of type <integer>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300',
}, 'A custom property of type <integer>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<length-percentage>#",
from: '100px, 200px',
to: '300%',
}, 'A custom property of type <length-percentage># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300%',
}, 'A custom property of type <length-percentage># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<length-percentage>+",
from: '100px 200px',
to: '300%',
}, 'A custom property of type <length-percentage>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300%',
}, 'A custom property of type <length-percentage>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<length>#",
from: '100px, 200px',
to: '300px',
}, 'A custom property of type <length># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300px',
}, 'A custom property of type <length># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<length>+",
from: '100px 200px',
to: '300px',
}, 'A custom property of type <length>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300px',
}, 'A custom property of type <length>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<number>#",
from: '100, 200',
to: '300',
}, 'A custom property of type <number># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300',
}, 'A custom property of type <number># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<number>+",
from: '100 200',
to: '300',
}, 'A custom property of type <number>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300',
}, 'A custom property of type <number>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<percentage>#",
from: '100%, 200%',
to: '300%',
}, 'A custom property of type <percentage># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300%',
}, 'A custom property of type <percentage># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<percentage>+",
from: '100% 200%',
to: '300%',
}, 'A custom property of type <percentage>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300%',
}, 'A custom property of type <percentage>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<resolution>#",
from: '100dppx, 200dppx',
to: '300dppx',
}, 'A custom property of type <resolution># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300dppx',
}, 'A custom property of type <resolution># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<resolution>+",
from: '100dppx 200dppx',
to: '300dppx',
}, 'A custom property of type <resolution>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300dppx',
}, 'A custom property of type <resolution>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<time>#",
from: '100s, 200s',
to: '300s',
}, 'A custom property of type <time># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300s',
}, 'A custom property of type <time># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<time>+",
from: '100s 200s',
to: '300s',
}, 'A custom property of type <time>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: '300s',
}, 'A custom property of type <time>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<url>#",
from: 'url("https://example.com/foo"), url("https://example.com/bar")',
to: 'url("https://example.com/to")',
}, 'A custom property of type <url># does not yield a CSS Transition if the lists do not contain the same number of values');
expected: 'url("https://example.com/to")',
}, 'A custom property of type <url># yields a discrete CSS Transition if the lists do not contain the same number of values');

no_transition_test({
transition_test({
syntax: "<url>+",
from: 'url("https://example.com/foo") url("https://example.com/bar")',
to: 'url("https://example.com/to")',
}, 'A custom property of type <url>+ does not yield a CSS Transition if the lists do not contain the same number of values');
expected: 'url("https://example.com/to")',
}, 'A custom property of type <url>+ yields a discrete CSS Transition if the lists do not contain the same number of values');

</script>
@@ -1,4 +1,4 @@
This is a testharness.js-based test.
FAIL A custom property of type <transform-function> can yield a CSS Transition assert_equals: A single animation is running expected 1 but got 0
FAIL A custom property of type <transform-function> 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.

@@ -1,4 +1,4 @@
This is a testharness.js-based test.
FAIL A custom property of type <transform-list> can yield a CSS Transition assert_equals: A single animation is running expected 1 but got 0
FAIL A custom property of type <transform-list> 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.

Expand Up @@ -6,10 +6,11 @@
<div id="target"></div>
<script>

no_transition_test({
transition_test({
syntax: "<url>",
from: 'url("https://example.com/from")',
to: 'url("https://example.com/to")',
}, 'A custom property of type <url> cannot yield a CSS Transition');
expected: 'url("https://example.com/to")',
}, 'A custom property of type <url> can yield a discrete CSS Transition');

</script>
Expand Up @@ -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`;
Expand Down

0 comments on commit dc7679b

Please sign in to comment.