Issue metadata
Sign in to add a comment
|
Sibling of animated element incorrectly animates under certain conditions |
||||||||||||||||||||||
Issue descriptionChrome Version: 56.0.2924.87 OS: macOS 10.12.3 What steps will reproduce the problem? (1) Open this JSBin http://output.jsbin.com/saqupu (2) Click toggle (possibly multiple times) (3) While the window.alert() is open, note how the sibling paragraph also rotates when it shouldn't What is the expected result? Only the red box rotates. What happens instead? Both the red box and the adjacent paragraph rotates.
,
Feb 23 2017
Able to repro on Linux as well.
,
Feb 23 2017
,
Feb 23 2017
Able to reproduce the issue on windows 7, Ubuntu 14.04 and Mac 10.12.3 using chrome version 56.0.2924.87 and canary 58.0.3020.0. This is regression issue broken in M54.Please find the bisect status as below Narrow Bisect:: Good :: 54.0.2789.0 --- (build revision 403806) Bad:: 54.0.2790.0 -- (build revision 404030) Change Log:: https://chromium.googlesource.com/chromium/src/+log/53feaa41b65987c69f54da1152d50f45e04deaac..a46d892723fe86a7f49113c46e3c40c2343724ce Possible suspect:: https://chromium.googlesource.com/chromium/src/+/a46d892723fe86a7f49113c46e3c40c2343724ce alancutter@ could you please look into this issue if it is related to your change,else please help us in finding the appropriate owner for this issue. Thanks,
,
Feb 27 2017
Taking a look. Thanks for the minimal test case. Out of curiosity how does this issue affect Polymer?
,
Feb 27 2017
Doesn't affect the Polymer library, but affects one of our elements (paper-spinner, ref https://github.com/PolymerElements/paper-spinner/issues/65).
,
Feb 28 2017
https://chromium.googlesource.com/chromium/src/+/a46d892723fe86a7f49113c46e3c40c2343724ce moved the decision of whether isStackingContext is true to outside style resolution so that it could capture the effects of the animation update being applied. Moving it back into StyleAdjuster appears to fix the bug. I should be able to change this patch so that only the bits relating to compositor animations are handled outside of StyleAdjuster and every other isStackingContext factor remains where it used to be.
,
Feb 28 2017
Uh oh. Seems like the bug that revision a46d892723 fixes might have been hiding a bug elsewhere in layer squashing.
If I move
style.updateIsStackingContext(
element == element->document().documentElement(),
element->isInTopLayer());
back into StyleAdjuster::adjustComputedStyle() and only do
if (style->hasCurrentTransformAnimation()) {
style->setIsStackingContext(true);
}
in Element::styleForLayoutObject() then the alert() bug still occurs while if I remove the stacking context update from Element::styleForLayoutObject() (which reintroduces another bug where the stacking context flag is delayed by one frame) then the alert() bug doesn't happen.
,
Feb 28 2017
+trchen who brought up the issue regarding ComputedStyle::isStackingContext() and transform animations originally.
,
Feb 28 2017
Running a test for whether the change that https://chromium.googlesource.com/chromium/src/+/a46d892723fe86a7f49113c46e3c40c2343724ce made to when isStackingContext gets calculated is needed anymore as the original test for it no longer fails if the calculation is moved back to where it used to be. Passing this onto Blink>Compositing. I'm afraid I have little expertise beyond producing the ComputedStyle object and request someone further down the rendering pipeline please take over this one.
,
Feb 28 2017
Need an owner for this regression. Even though it was introduced elsewhere, fixing it requires changes to our animation code.
,
Mar 2 2017
Slightly reduced test case attached. Have not been able to repro without the button to start rotation. Something about pressing the button affects the test case for whatever reason.
,
Mar 2 2017
,
Mar 16 2017
Hi, what's the status of this bug? It's marked as p1.
,
Mar 17 2017
Apologies, I thought I linked to my experimental CL in #10. See https://codereview.chromium.org/2722553003 for a code version of what I described at the end of #8. We may be able to do that, introducing an old bug and hiding this bug if we decide this bug is worse.
,
Mar 17 2017
Thanks I will take a look at this in the morning.
,
Mar 17 2017
Recap to help with archaeology and as I gather background. Two bugs potentially relate, both still open: http://crbug.com/616674 - FATAL:CompositedLayerMapping.cpp(649)] Check failed: layers[i].paintLayer->transformAncestor() == commonTransformAncestor. http://crbug.com/375982 - z-index inherits used value, not computed value. Three changes potentially relate: http://crrev.com/2722553003 - [WIP] Test moving stacking context flag update on ComputedStyle http://crrev.com/2047283002 - Avoid touching z-index in StyleAdjuster by using an isStackingContext flag instead http://crrev.com/2035793007 - Make sure z-index is adjusted after applying pending animations on elements Enough time has passed that we may as well sort of re-investigate from first principles keeping this backstory in mind. It's not really clear to me whether this is a true P1. The only test case I've been able to reduce from the jsbin seems obscure, though certainly undesirable. I question priority since fixing could involve debugging and fixing legacy squashing/compositing logic which we are amidst reworking for SPv2. It's worth spending a day or so aiming to understand better in any case, so I'll do that.
,
Mar 17 2017
Repro from trchen@ without a button or alert: http://output.jsbin.com/voporukeve
,
Mar 17 2017
Attached is further refined test case. When we click toggle we see layer backing reason update checks in CompositingLayerAssigner::assignLayersToBackingsInternal for ids 'b' and 'c', but perhaps one or both of them should have a squashing disallowed reasons, and they don't. If we comment out the removal of the animation class, we see subsequent compositing update with reason=(transform ancestor mismatch) for id 'c'.
,
Mar 17 2017
Potentially we need to modify CompositingInputsUpdater where we check for transforms on an ancestor to also consider whether an animation on that paint layer is started and will potentially mutate the transform. This would apply to opacity as well. We expect we could create a similarly failing test case for opacity (and should do so).
,
Mar 21 2017
On first frame after starting animation, PaintLayers for 'a' and 'b' have transformAncestor() = nullptr in CompositingInputsUpdater::updateRecursive, and we incorrectly squash 'c' into 'b''s layer. If we leave animation running, we eventually see recomposite where both 'a' and 'b' have transformAncestor() = 'spinner', after which we see the desired result. transformAncestor() value is sourced from ancestorDependentCompositingInputs. CompositingInputsUpdater checks PaintLayer::transform() when computing PaintLayer::AncestorDependentCompositingInputs.transformAncestor. This method is at least documented as incorporating any transform from accelerated animations (see method docs on PaintLayer::currentTransform() which in turn calls transform()). High level control flow from FrameView, focusing just on SPv1 for now, looks like: 1. update compositing state 2. update descendant dependent flags 3. update animations So, if the descendant dependent flags/compositing inputs are stale, which looks possible given above, it could explain what we are seeing. Perhaps it was not possible for them to be stale before http://crrev.com/2047283002. Further validation of this theory and exploration of potential solutions forthcoming.
,
Mar 21 2017
We do see animation-reflective compositing reasons for 'spinner' in PaintLayer::attemptDirectCompositingUpdate as potentialCompositingReasonsFromStyle="activeAnimation,willChange,transformWithCompositedDescendants", oldPotentialCompositingReasonsFromStyle="willChange". Root issue seems to be that the transform ancestor is not set correctly. Transform ancestor is directly derived from existence of transform in the PaintLayer's rare data. Thus the transform seems not to be updated/set when it should be. PaintLayer::updateTransform early-outs if the old and new style transformDataEquivalent() == true. Commenting out this early-out fixes the issue. PaintLayer::attemptDirectCompositingUpdate(), which call precedes the call to update transform, includes a comment as: // The potentialCompositingReasonsFromStyle could have changed without // a corresponding StyleDifference if an animation started or ended.
,
Mar 22 2017
http://crrev.com/2767783003 in review.
,
Mar 22 2017
Attaching final layout test used during development to repro situation. Not yet automatable or de-flaky.
,
Mar 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/463b2dfe64df899fda4359f2d360ee1cba46e26b commit 463b2dfe64df899fda4359f2d360ee1cba46e26b Author: wkorman <wkorman@chromium.org> Date: Wed Mar 22 22:45:33 2017 Incorporate ComputedStyle::hasTransform when diffing transform styles. BUG= 695125 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2767783003 Cr-Commit-Position: refs/heads/master@{#458907} [modify] https://crrev.com/463b2dfe64df899fda4359f2d360ee1cba46e26b/third_party/WebKit/Source/core/paint/PaintLayer.cpp [modify] https://crrev.com/463b2dfe64df899fda4359f2d360ee1cba46e26b/third_party/WebKit/Source/core/paint/PaintLayer.h [modify] https://crrev.com/463b2dfe64df899fda4359f2d360ee1cba46e26b/third_party/WebKit/Source/core/paint/PaintLayerTest.cpp [modify] https://crrev.com/463b2dfe64df899fda4359f2d360ee1cba46e26b/third_party/WebKit/Source/core/style/ComputedStyle.cpp [modify] https://crrev.com/463b2dfe64df899fda4359f2d360ee1cba46e26b/third_party/WebKit/Source/core/style/ComputedStyle.h [modify] https://crrev.com/463b2dfe64df899fda4359f2d360ee1cba46e26b/third_party/WebKit/Source/core/style/ComputedStyleTest.cpp
,
Mar 22 2017
,
Mar 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/15eb85b8cb9d52cb79dc37dff7d2cfcd2400e778 commit 15eb85b8cb9d52cb79dc37dff7d2cfcd2400e778 Author: wkorman <wkorman@chromium.org> Date: Fri Mar 24 23:25:08 2017 Add operator<< to StyleDifference for debug logging. Similar to http://crrev.com/2732643004. I wanted this while debugging animation related style changes and thought it would be potentially useful for others ongoing. BUG= 695125 Review-Url: https://codereview.chromium.org/2763643003 Cr-Commit-Position: refs/heads/master@{#459595} [modify] https://crrev.com/15eb85b8cb9d52cb79dc37dff7d2cfcd2400e778/third_party/WebKit/Source/core/BUILD.gn [modify] https://crrev.com/15eb85b8cb9d52cb79dc37dff7d2cfcd2400e778/third_party/WebKit/Source/core/style/BUILD.gn [add] https://crrev.com/15eb85b8cb9d52cb79dc37dff7d2cfcd2400e778/third_party/WebKit/Source/core/style/StyleDifference.cpp [modify] https://crrev.com/15eb85b8cb9d52cb79dc37dff7d2cfcd2400e778/third_party/WebKit/Source/core/style/StyleDifference.h [add] https://crrev.com/15eb85b8cb9d52cb79dc37dff7d2cfcd2400e778/third_party/WebKit/Source/core/style/StyleDifferenceTest.cpp
,
Mar 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb14d83f2208140bbde345c88f02b7a45736d553 commit eb14d83f2208140bbde345c88f02b7a45736d553 Author: hayato <hayato@chromium.org> Date: Mon Mar 27 10:56:36 2017 Revert of Add operator<< to StyleDifference for debug logging. (patchset #5 id:80001 of https://codereview.chromium.org/2763643003/ ) Reason for revert: Consistent failure: webkit_unit_tests failing on chromium.webkit/WebKit Android (Nexus4) WebKit Android (Nexus4) [50 since first detection] https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Android%20%28Nexus4%29: The first failure: - https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Android%20%28Nexus4%29/builds/62765 Test output: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit_Android__Nexus4_%2F62765%2F%2B%2Frecipes%2Fsteps%2Fwebkit_unit_tests%2F0%2Fstdout C 169.736s Main [FAIL] StyleDifferenceTest.StreamOutputAllFieldsMutated: C 169.736s Main [ RUN ] StyleDifferenceTest.StreamOutputAllFieldsMutated C 169.736s Main ../../third_party/WebKit/Source/core/style/StyleDifferenceTest.cpp:40: Failure C 169.736s Main Value of: stringStream.str() C 169.736s Main Actual: "StyleDifference{layoutType=PositionedMovement, paintInvalidationType=PaintInvalidationObject, recomputeOverflow=1, visualRectUpdate=1, propertySpecificDifferences=TransformChanged|ScrollAnchorDisablingPropertyChanged}" C 169.736s Main Expected: "StyleDifference{layoutType=PositionedMovement, " "paintInvalidationType=PaintInvalidationObject, recomputeOverflow=1, " "visualRectUpdate=1, " "propertySpecificDifferences=TransformChanged|" "ScrollAnchorDisablingPropertyChanged|TransformChanged|" "ScrollAnchorDisablingPropertyChanged}" C 169.736s Main Which is: "StyleDifference{layoutType=PositionedMovement, paintInvalidationType=PaintInvalidationObject, recomputeOverflow=1, visualRectUpdate=1, propertySpecificDifferences=TransformChanged|ScrollAnchorDisablingPropertyChanged|TransformChanged|ScrollAnchorDisablingPropertyChanged}" Original issue's description: > Add operator<< to StyleDifference for debug logging. > > Similar to http://crrev.com/2732643004. I wanted this while debugging animation > related style changes and thought it would be potentially useful for others > ongoing. > > BUG= 695125 > > Review-Url: https://codereview.chromium.org/2763643003 > Cr-Commit-Position: refs/heads/master@{#459595} > Committed: https://chromium.googlesource.com/chromium/src/+/15eb85b8cb9d52cb79dc37dff7d2cfcd2400e778 TBR=alancutter@chromium.org,wangxianzhu@chromium.org,wkorman@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 695125 Review-Url: https://codereview.chromium.org/2777803002 Cr-Commit-Position: refs/heads/master@{#459746} [modify] https://crrev.com/eb14d83f2208140bbde345c88f02b7a45736d553/third_party/WebKit/Source/core/BUILD.gn [modify] https://crrev.com/eb14d83f2208140bbde345c88f02b7a45736d553/third_party/WebKit/Source/core/style/BUILD.gn [delete] https://crrev.com/06cde11f9f3843eb13e3d3d73296a07e9e8601f5/third_party/WebKit/Source/core/style/StyleDifference.cpp [modify] https://crrev.com/eb14d83f2208140bbde345c88f02b7a45736d553/third_party/WebKit/Source/core/style/StyleDifference.h [delete] https://crrev.com/06cde11f9f3843eb13e3d3d73296a07e9e8601f5/third_party/WebKit/Source/core/style/StyleDifferenceTest.cpp
,
Mar 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b0f213ebc01011ec377f4bfd19566e97bfedff8 commit 9b0f213ebc01011ec377f4bfd19566e97bfedff8 Author: wkorman <wkorman@chromium.org> Date: Thu Mar 30 15:05:59 2017 Add operator<< to StyleDifference for debug logging. Reland of http://crrev.com/2763643003 with bit iteration bug fixed and updated tests. Also moved back to prior patch set logic re: visual rect and scroll anchor disabling property, as http://crrev.com/2772353002 which conflicts was also reverted. Original description: Similar to http://crrev.com/2732643004. I wanted this while debugging animation related style changes and thought it would be potentially useful for others ongoing. BUG= 695125 TBR=wangxianzhu Review-Url: https://codereview.chromium.org/2779353002 Cr-Commit-Position: refs/heads/master@{#460754} [modify] https://crrev.com/9b0f213ebc01011ec377f4bfd19566e97bfedff8/third_party/WebKit/Source/core/BUILD.gn [modify] https://crrev.com/9b0f213ebc01011ec377f4bfd19566e97bfedff8/third_party/WebKit/Source/core/style/BUILD.gn [add] https://crrev.com/9b0f213ebc01011ec377f4bfd19566e97bfedff8/third_party/WebKit/Source/core/style/StyleDifference.cpp [modify] https://crrev.com/9b0f213ebc01011ec377f4bfd19566e97bfedff8/third_party/WebKit/Source/core/style/StyleDifference.h [add] https://crrev.com/9b0f213ebc01011ec377f4bfd19566e97bfedff8/third_party/WebKit/Source/core/style/StyleDifferenceTest.cpp
,
May 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2c693969ab57ca41b67582265c08c1094a71a0bc commit 2c693969ab57ca41b67582265c08c1094a71a0bc Author: Walter Korman <wkorman@chromium.org> Date: Tue May 16 18:37:00 2017 Incorporate ComputedStyle::hasTransform when diffing transform styles. BUG= 695125 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2767783003 Cr-Original-Commit-Position: refs/heads/master@{#458907} Review-Url: https://codereview.chromium.org/2891453003 . Cr-Commit-Position: refs/branch-heads/3029@{#846} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/2c693969ab57ca41b67582265c08c1094a71a0bc/third_party/WebKit/Source/core/paint/PaintLayer.cpp [modify] https://crrev.com/2c693969ab57ca41b67582265c08c1094a71a0bc/third_party/WebKit/Source/core/paint/PaintLayer.h [modify] https://crrev.com/2c693969ab57ca41b67582265c08c1094a71a0bc/third_party/WebKit/Source/core/paint/PaintLayerTest.cpp [modify] https://crrev.com/2c693969ab57ca41b67582265c08c1094a71a0bc/third_party/WebKit/Source/core/style/ComputedStyle.cpp [modify] https://crrev.com/2c693969ab57ca41b67582265c08c1094a71a0bc/third_party/WebKit/Source/core/style/ComputedStyle.h [modify] https://crrev.com/2c693969ab57ca41b67582265c08c1094a71a0bc/third_party/WebKit/Source/core/style/ComputedStyleTest.cpp |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by dstockwell@chromium.org
, Feb 23 2017