Fullscreen will interrupt transform animation
Reported by
liuhao...@gmail.com,
Oct 30
|
|||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.77 Safari/537.36 Steps to reproduce the problem: 1. open attachment(index.html) in chrome 2. click start transform button 3. then click toggle fullscreen button What is the expected behavior? The `TEXT TEXT TEXT` div would being transforming. What went wrong? The `TEXT TEXT TEXT` div had been ended. Did this work before? Yes not sure, maybe chrome 68 Does this work in other browsers? Yes Chrome version: 70.0.3538.77 Channel: stable OS Version: 10.0 Flash Version:
,
Oct 30
,
Oct 31
liuhao.me@ Thanks for the issue. Able to reproduce this issue on Windows 10, Mac OS 10.13.6 and Ubuntu 17.10 on the reported version 70.0.3538.77 and latest Canary 72.0.3596.0. Bisect Information: =================== Good Build: 69.0.3446.0 Bad Build : 69.0.3447.0 By running the per-revision bisect script, below is the Changelog URL. https://chromium.googlesource.com/chromium/src/+log/fbf576282bc9b2281a6a2299883ef910c5d71e70..14eeb1d5b6369a4fefa57973bf6b646469f72e35 Reviewed On: https://chromium-review.googlesource.com/1066600 dtapuska@ Please check and confirm if this issue is related to your change, else help us in assigning to the right owner. Thanks..
,
Oct 31
Suspecting a duplicate issue 896151 . Please confirm once this issue gets fixed.
,
Oct 31
Yes this was caused by the top layer change but I don't think reverting it is the appropriate fix since this is a long performance and compatibility fix. It is unfortunate that the regression only came up now. I believe this is a problem of the Animation system that it doesn't survive a layout tree rebuild. When an item is moved into the top layer calling Element::SetIsInTopLayer destroys the LayoutObject and a new compositor ID will be allocated.
,
Nov 1
,
Nov 5
This is a P1 regression, and needs an owner. Or lower the priority if it shouldn't block a release.
,
Nov 5
,
Nov 5
Rob, could you triage this and assign an owner?
,
Nov 6
This is tricky, by calling LazyReattachIfAttached we detach the node which implicitly cancels the transition as there is no old_style in the call to blink::CSSAnimations::CalculateTransitionUpdate due to the LayoutObject having been detached. I.e. we determine from the style change that there should no longer be any transition, and this is correct since the element was reparented. I'm not sure how we can expect this to work correctly as long as it is doing a reattach.
,
Nov 6
I put together a test case for reparenting an element: https://jsbin.com/celuzow/edit?html,css,js,output Chrome, Firefox and Safari exhibit the same behavior of finishing the transition immediately. Interestingly, Edge continues the transition after the element is reparented. That suggests that this reattached transition behavior is somewhat of an interop issue.
,
Nov 6
FYI I filed issue 902328 for the clarifying the interop issue of whether reparented elements should continue transitions.
,
Nov 6
Reparenting an element makes the element disconnected, so I'm ~sure edge is wrong here. However, fullscreen doesn't reparent the element right? Just the LayoutObject.
,
Nov 6
Yeah that was my thought as well, that edge is probably wrong here. The problem in blink is that the layoutobject is the attached state of the element. Reparenting it tears down the old state and recreates it so we need to add special code to at least keep the computed style around.
,
Nov 8
Emil, would this be something that layout or style team could help with? Making it such that we at least preserve the computed style since the fullscreened element is still conceptually parented by the same DOM node.
,
Nov 12
,
Nov 12
I'm currently working on decoupling ComputedStyle from LayoutObject and always store it on NodeRenderingData. That should fix this issue, but it will take a while.
On another note, re-attaching during style recalc does not cancel the transition/animation like:
<!DOCTYPE html>
<style>
#trans { transition: color 5s }
</style>XX
<div id="inline">
<span id="trans">
TRANSITIONCOLOR
</span>
</div>
<script>
trans.offsetTop;
trans.style.color = "white";
setTimeout(() => inline.style.display = "inline", 2000);
</script>
LazyReattachIfAttached is actually a bad pattern we should get rid of and wait to do the DetachLayoutTree part until the lifecycle update. It's something we currently use when changes in ComputedStyle does not imply the layout tree change we want, like for <object> fallback and fullscreen reparenting.
A fix here would be to get rid of the LazyReattachIfAttached() call and signal the re-attachment during style recalc instead.
I can see if I can come up with something along those lines.
,
Nov 13
,
Nov 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/71f16b992e49df7d439340908caffbee4ef068d6 commit 71f16b992e49df7d439340908caffbee4ef068d6 Author: Rune Lillesveen <futhark@chromium.org> Date: Thu Nov 15 21:50:23 2018 Keep animations running going to/from fullscreen. LazyReattachIfAttached() nukes the computed styles synchronously so that we are not able to keep animations alive for the next lifecycle update. Add a bit on Node to say that we need a forced layout tree reattach and trigger the re-attach from style recalc instead. Bug: 900162 Change-Id: I22f51f5e091932fc8a63213af88903c7866abbb4 Reviewed-on: https://chromium-review.googlesource.com/c/1333809 Reviewed-by: Robert Flack <flackr@chromium.org> Commit-Queue: Rune Lillesveen <futhark@chromium.org> Cr-Commit-Position: refs/heads/master@{#608530} [add] https://crrev.com/71f16b992e49df7d439340908caffbee4ef068d6/third_party/WebKit/LayoutTests/external/wpt/fullscreen/rendering/fullscreen-css-transition.html [modify] https://crrev.com/71f16b992e49df7d439340908caffbee4ef068d6/third_party/blink/renderer/core/dom/document.cc [modify] https://crrev.com/71f16b992e49df7d439340908caffbee4ef068d6/third_party/blink/renderer/core/dom/element.cc [modify] https://crrev.com/71f16b992e49df7d439340908caffbee4ef068d6/third_party/blink/renderer/core/dom/layout_tree_builder_traversal.cc [modify] https://crrev.com/71f16b992e49df7d439340908caffbee4ef068d6/third_party/blink/renderer/core/dom/node.cc [modify] https://crrev.com/71f16b992e49df7d439340908caffbee4ef068d6/third_party/blink/renderer/core/dom/node.h
,
Nov 15
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by dtapu...@chromium.org
, Oct 30