Skip to content

GitLab

T
tangled
Commit 5379bbb8 authored by George Steel's avatar George Steel Committed by Commit Bot
Browse files
Options

Perpare to fix cancelling when unsetting transition and fix metric 

Add logic to distinguish between transitions which should (base
computed style changes) and shouldn't be cancelled when transition is
completely unset (as the default is all 0s). Currently, both are still
cancelled, the same as the old behaviour, but the histogram
kCSSTransitionCancelledByRemovingStyle is now only incremented when
transitions are cancelled incorrectly.

Add WPT for not cancelling transitions then unsetting transition
property, fix web_tests to cancel transitions by setting transition to
none instead of unsetting.

Bug: 934700
Change-Id: Iaf4bf324e6e2bd6ca22a66fd8ae2320620a874dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2363173Reviewed-by: default avatarKevin Ellis <kevers@chromium.org>
Commit-Queue: George Steel <gtsteel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799336}
parent 4b5ea5f3
Hide whitespace changes
Inline Side-by-side
Showing with 107 additions and 87 deletions
third_party/blink/renderer/core/animation/css/css_animations.cc
View file @ 5379bbb8
......@@ -1060,7 +1060,15 @@ void CSSAnimations::CalculateTransitionUpdateForProperty(
active_transition_iter->value;
if (CSSPropertyEquality::PropertiesEqual(property, state.style,
*running_transition->to)) {
return;
if (!state.transition_data) {
UseCounter::Count(state.animating_element->GetDocument(),
WebFeature::kCSSTransitionCancelledByRemovingStyle);
// TODO(crbug.com/934700): Add a return to this branch to correctly
// continue transitions under default settings (all 0s) in the absence
// of a change in base computed style.
} else {
return;
}
}
state.update.CancelTransition(property);
DCHECK(!state.animating_element->GetElementAnimations() ||
......@@ -1075,6 +1083,11 @@ void CSSAnimations::CalculateTransitionUpdateForProperty(
}
}
// In the default configutation (transition: all 0s) we continue and cancel
// transitions but do not start them.
if (!state.transition_data)
return;
const PropertyRegistry* registry =
state.animating_element->GetDocument().GetPropertyRegistry();
if (property.IsCSSCustomProperty()) {
......@@ -1144,7 +1157,7 @@ void CSSAnimations::CalculateTransitionUpdateForProperty(
// If we have multiple transitions on the same property, we will use the
// last one since we iterate over them in order.
Timing timing = state.transition_data.ConvertToTiming(transition_index);
Timing timing = state.transition_data->ConvertToTiming(transition_index);
// CSS Transitions always have a valid duration (i.e. the value 'auto' is not
// supported), so iteration_duration will always be set.
if (timing.start_delay + timing.iteration_duration->InSecondsF() <= 0) {
......@@ -1298,7 +1311,7 @@ void CSSAnimations::CalculateTransitionUpdate(CSSAnimationUpdate& update,
bool any_transition_had_transition_all = false;
const ComputedStyle* old_style = animating_element->GetComputedStyle();
if (!animation_style_recalc && style.Display() != EDisplay::kNone &&
old_style && !old_style->IsEnsuredInDisplayNone() && transition_data) {
old_style && !old_style->IsEnsuredInDisplayNone()) {
TransitionUpdateState state = {update,
animating_element,
*old_style,
......@@ -1307,23 +1320,34 @@ void CSSAnimations::CalculateTransitionUpdate(CSSAnimationUpdate& update,
/*cloned_style=*/nullptr,
active_transitions,
listed_properties,
*transition_data};
for (wtf_size_t transition_index = 0;
transition_index < transition_data->PropertyList().size();
++transition_index) {
const CSSTransitionData::TransitionProperty& transition_property =
transition_data->PropertyList()[transition_index];
if (transition_property.unresolved_property == CSSPropertyID::kAll) {
any_transition_had_transition_all = true;
transition_data};
if (transition_data) {
for (wtf_size_t transition_index = 0;
transition_index < transition_data->PropertyList().size();
++transition_index) {
const CSSTransitionData::TransitionProperty& transition_property =
transition_data->PropertyList()[transition_index];
if (transition_property.unresolved_property == CSSPropertyID::kAll) {
any_transition_had_transition_all = true;
}
if (property_pass == PropertyPass::kCustom) {
CalculateTransitionUpdateForCustomProperty(state, transition_property,
transition_index);
} else {
DCHECK_EQ(property_pass, PropertyPass::kStandard);
CalculateTransitionUpdateForStandardProperty(
state, transition_property, transition_index, style);
}
}
if (property_pass == PropertyPass::kCustom) {
CalculateTransitionUpdateForCustomProperty(state, transition_property,
transition_index);
} else {
DCHECK_EQ(property_pass, PropertyPass::kStandard);
CalculateTransitionUpdateForStandardProperty(state, transition_property,
transition_index, style);
} else if (active_transitions && active_transitions->size()) {
// !transition_data implies transition: all 0s
any_transition_had_transition_all = true;
if (property_pass == PropertyPass::kStandard) {
CSSTransitionData::TransitionProperty default_property(
CSSPropertyID::kAll);
CalculateTransitionUpdateForStandardProperty(state, default_property, 0,
style);
}
}
}
......@@ -1338,12 +1362,6 @@ void CSSAnimations::CalculateTransitionUpdate(CSSAnimationUpdate& update,
if (!any_transition_had_transition_all && !animation_style_recalc &&
!listed_properties.Contains(property)) {
update.CancelTransition(property);
// Measure how often transitions are canceled by removing their style.
// See https://crbug.com/934700.
if (!transition_data) {
UseCounter::Count(animating_element->GetDocument(),
WebFeature::kCSSTransitionCancelledByRemovingStyle);
}
} else if (entry.value->animation->FinishedInternal()) {
update.FinishTransition(property);
}
......
third_party/blink/renderer/core/animation/css/css_animations.h
View file @ 5379bbb8
......@@ -187,7 +187,7 @@ class CORE_EXPORT CSSAnimations final {
scoped_refptr<const ComputedStyle> cloned_style;
const TransitionMap* active_transitions;
HashSet<PropertyHandle>& listed_properties;
const CSSTransitionData& transition_data;
const CSSTransitionData* transition_data;
};
static void CalculateTransitionUpdateForCustomProperty(
......
third_party/blink/web_tests/external/wpt/css/css-transitions/changing-while-transition-004-expected.txt 0 → 100644
View file @ 5379bbb8
This is a testharness.js-based test.
FAIL Unsettign transition should not affect in-flight transitions assert_equals: Even after unsetting transition, the transition should be 50% complete expected "50px" but got "100px"
Harness: the test ran to completion.
third_party/blink/web_tests/external/wpt/css/css-transitions/changing-while-transition-004.html 0 → 100644
View file @ 5379bbb8
<!doctype html>
<html>
<head>
<meta charset=utf-8>
<title>CSS Transitions Test: behavior when transition changes to default while transitioning</title>
<meta name="assert" content="Checks a change to the transition-duration
property does not affect an in-flight transition">
<link rel="help" title="3. Starting of transitions" href="https://drafts.csswg.org/css-transitions/#starting">
<script src="/resources/testharness.js" type="text/javascript"></script>
<script src="/resources/testharnessreport.js" type="text/javascript"></script>
<script src="./support/helper.js" type="text/javascript"></script>
</head>
<body>
<div id="log"></div>
<script>
promise_test(async t => {
// Start a 100s transition 50% of the way through
const div = addDiv(t, {
style: 'transition: height 100s -50s linear; height: 0px',
});
getComputedStyle(div).height;
div.style.height = '100px';
assert_equals(
getComputedStyle(div).height,
'50px',
'Transition should be initially 50% complete'
);
// Unset the transition property (with default all 0s).
div.style.transition = '';
// If the change to the transition-duration was reflected, the
// transition would be cancelled and the computed height
// would now be '25px'.
assert_equals(
getComputedStyle(div).height,
'50px',
'Even after unsetting transition, the transition should be 50% complete'
);
// Wait a frame just to be sure that the changed duration is not later
// updated.
await waitForFrame();
assert_greater_than_equal(
parseInt(getComputedStyle(div).height),
50,
'Even in the next frame the updated transition should not apply'
);
}, 'Unsettign transition should not affect in-flight transitions');
</script>
</body>
</html>
third_party/blink/web_tests/inspector-protocol/animation/animation-empty-transition-cancel.js
View file @ 5379bbb8
......@@ -12,7 +12,7 @@
node.offsetTop;
node.style.width = "200px";
node.offsetTop;
node.style.transition = "";
node.style.transition = "none";
node.offsetTop;
`);
testRunner.completeTest();
......
third_party/blink/web_tests/inspector-protocol/animation/animation-start-cancel.js
View file @ 5379bbb8
......@@ -20,7 +20,7 @@
// before we cancel it by clearing the transition.
window.requestAnimationFrame(function() {
window.requestAnimationFrame(function() {
node.style.transition = '';
node.style.transition = 'none';
});
});
`);
......
third_party/blink/web_tests/transitions/remove-transition-style.html deleted 100644 → 0
View file @ 4b5ea5f3
<!DOCTYPE html>
<html>
<head>
<style>
#box {
height: 200px;
width: 200px;
margin: 20px;
background-color: blue;
}
.animated {
-webkit-transition: opacity 2000s step-end;
}
</style>
<script>
if (window.testRunner) {
testRunner.waitUntilDone();
testRunner.dumpAsText();
}
function log(s)
{
var results = document.getElementById('results');
results.innerHTML += s + '<br>';
}
function testTransitions()
{
var box = document.getElementById('box');
box.className = 'animated';
box.style.opacity = '0.5';
if (getComputedStyle(box).opacity == '0.5')
log("Transition didn't start: FAIL");
else {
box.className = '';
if (getComputedStyle(box).opacity == '0.5')
log('No running transitions: PASS');
else
log('Transition still running: FAIL')
}
if (window.testRunner) {
testRunner.notifyDone();
}
}
window.addEventListener('load', testTransitions, false);
</script>
</head>
<body>
<div id="box"></div>
<div id="results"></div>
</body>
</html>
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment