New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 695125 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Mar 2017
Cc:
Components:
EstimatedDays: 5
NextAction: ----
OS: All
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Sibling of animated element incorrectly animates under certain conditions

Project Member Reported by keanulee@chromium.org, Feb 22 2017

Issue description

Chrome 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.

 
Labels: Needs-Bisect
Reproduced on cros 55.0.2883.105
Components: -Blink>Animation Internals>Compositing>Animation
Labels: -OS-Mac OS-All
Status: Available (was: Untriaged)
Able to repro on Linux as well.

Comment 3 by ajha@chromium.org, Feb 23 2017

Labels: Needs-Triage-M56
Labels: -Type-Bug -Pri-3 -Needs-Bisect -Needs-Triage-M56 has-bisect-per-revision M-58 Pri-1 Type-Bug-Regression
Owner: alancutter@chromium.org
Status: Assigned (was: Available)
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,
Taking a look. Thanks for the minimal test case.

Out of curiosity how does this issue affect Polymer?
Doesn't affect the Polymer library, but affects one of our elements (paper-spinner, ref https://github.com/PolymerElements/paper-spinner/issues/65).
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.
Labels: Regressed-54
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.
Cc: trchen@chromium.org
Components: -Internals>Compositing>Animation
+trchen who brought up the issue regarding ComputedStyle::isStackingContext() and transform animations originally.
Components: Blink>Compositing
Owner: ----
Status: Available (was: Assigned)
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.
EstimatedDays: 5
Labels: RegressionFound-56
Owner: wkorman@chromium.org
Status: Assigned (was: Available)
Need an owner for this regression. Even though it was introduced elsewhere, fixing it requires changes to our animation code.
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.
695125.html
805 bytes View Download
Cc: alancutter@chromium.org
Hi, what's the status of this bug? It's marked as p1.
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.
Thanks I will take a look at this in the morning.
Cc: chrishtr@chromium.org
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.
Repro from trchen@ without a button or alert: http://output.jsbin.com/voporukeve
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'.
695125-4.html
1.0 KB View Download
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).
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.
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.

http://crrev.com/2767783003 in review.
Attaching final layout test used during development to repro situation. Not yet automatable or de-flaky.
transform-animation-avoid-squashing.html
1.1 KB View Download
Status: Fixed (was: Assigned)
Project Member

Comment 28 by bugdroid1@chromium.org, 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

Project Member

Comment 29 by bugdroid1@chromium.org, 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

Project Member

Comment 30 by bugdroid1@chromium.org, May 16 2017

Labels: merge-merged-3029
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