Make Composited Animations Invalidate Paint Directly [chromium/src : main]

0 views
Skip to first unread message

Claire Chambers (Gerrit)

unread,
Mar 25, 2024, 1:23:01 PMMar 25
to Olga Gerchikov, Robert Flack, Kevin Ellis, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
Attention needed from Kevin Ellis, Olga Gerchikov and Robert Flack

Claire Chambers voted Commit-Queue+1

Commit-Queue+1
Open in Gerrit

Related details

Attention is currently required from:
  • Kevin Ellis
  • Olga Gerchikov
  • Robert Flack
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
Gerrit-Change-Number: 5384914
Gerrit-PatchSet: 3
Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: David Bokan <bo...@chromium.org>
Gerrit-CC: Xida Chen <xida...@chromium.org>
Gerrit-Attention: Robert Flack <fla...@chromium.org>
Gerrit-Attention: Olga Gerchikov <gerc...@microsoft.com>
Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
Gerrit-Comment-Date: Mon, 25 Mar 2024 17:22:47 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Kevin Ellis (Gerrit)

unread,
Mar 25, 2024, 4:10:26 PMMar 25
to Claire Chambers, Olga Gerchikov, Robert Flack, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
Attention needed from Claire Chambers, Olga Gerchikov and Robert Flack

Kevin Ellis added 4 comments

Patchset-level comments
File-level comment, Patchset 5 (Latest):
Kevin Ellis . resolved

Looks like your stress test just failed when run in the virtual context.
CONSOLE ERROR: Uncaught ReferenceError: promise_test is not defined
This could suggest that you might have to switch to relative paths to load the test harness scripts. You should be able to experiment with this locally.

File third_party/blink/renderer/core/animation/element_animations.cc
Line 43, Patchset 5 (Parent): CompositedPaintStatus::kNeedsRepaintOrNoAnimation)),
Kevin Ellis . unresolved

Why are we changing background color status? I don't think you should be touching background color at all.

Line 122, Patchset 5 (Latest):void ElementAnimations::InvalidatePaintForCompositedAnimationsIfNecessary(
Kevin Ellis . unresolved

Your change is triggering multiple crashes in CSS-transitions within this method.

Line 132, Patchset 5 (Latest): SetCompositedBackgroundColorStatus(CompositedPaintStatus::kNoAnimation);
Kevin Ellis . unresolved

Why is this patch changing behavior for background color?

Open in Gerrit

Related details

Attention is currently required from:
  • Claire Chambers
  • Olga Gerchikov
  • Robert Flack
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Gerrit-Change-Number: 5384914
    Gerrit-PatchSet: 5
    Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
    Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: David Bokan <bo...@chromium.org>
    Gerrit-CC: Xida Chen <xida...@chromium.org>
    Gerrit-Attention: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Attention: Robert Flack <fla...@chromium.org>
    Gerrit-Attention: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-Comment-Date: Mon, 25 Mar 2024 20:10:17 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Blink W3C Test Autoroller (Gerrit)

    unread,
    Mar 28, 2024, 9:19:29 AMMar 28
    to Claire Chambers, Olga Gerchikov, Robert Flack, Kevin Ellis, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Olga Gerchikov and Robert Flack

    Message from Blink W3C Test Autoroller

    Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/45409.

    When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.

    WPT Export docs:
    https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Olga Gerchikov
    • Robert Flack
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Gerrit-Change-Number: 5384914
    Gerrit-PatchSet: 8
    Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
    Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Bokan <bo...@chromium.org>
    Gerrit-CC: Xida Chen <xida...@chromium.org>
    Gerrit-Attention: Robert Flack <fla...@chromium.org>
    Gerrit-Attention: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-Comment-Date: Thu, 28 Mar 2024 13:19:17 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Claire Chambers (Gerrit)

    unread,
    Mar 28, 2024, 9:41:11 AMMar 28
    to Blink W3C Test Autoroller, Olga Gerchikov, Robert Flack, Kevin Ellis, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Kevin Ellis, Olga Gerchikov and Robert Flack

    Claire Chambers voted and added 3 comments

    Votes added by Claire Chambers

    Commit-Queue+1

    3 comments

    Patchset-level comments
    Kevin Ellis . resolved

    Looks like your stress test just failed when run in the virtual context.
    CONSOLE ERROR: Uncaught ReferenceError: promise_test is not defined
    This could suggest that you might have to switch to relative paths to load the test harness scripts. You should be able to experiment with this locally.

    Claire Chambers

    Thanks, I had tried this with a prior version of the test but it didn't seem to fail expectedly without the fix. I've implemented this.

    File third_party/blink/renderer/core/animation/element_animations.cc
    Line 43, Patchset 5 (Parent): CompositedPaintStatus::kNeedsRepaintOrNoAnimation)),
    Kevin Ellis . unresolved

    Why are we changing background color status? I don't think you should be touching background color at all.

    Claire Chambers

    Because bgcolor animations rely on CompositablePaintAnimationChanged, same as clip path animations. Since this change makes CompositablePaintAnimationChanged unnecessary, it seemed a good idea to do so for both kinds of animations. They both need paint invalidated for the same reasons.

    I'm curious why we'd want to keep CompositablePaintAnimationChanged for bgcolor but eliminate it for clip paths.

    Line 122, Patchset 5:void ElementAnimations::InvalidatePaintForCompositedAnimationsIfNecessary(
    Kevin Ellis . resolved

    Your change is triggering multiple crashes in CSS-transitions within this method.

    Claire Chambers

    Fixed.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Kevin Ellis
    • Olga Gerchikov
    • Robert Flack
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Gerrit-Change-Number: 5384914
    Gerrit-PatchSet: 10
    Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
    Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-Reviewer: Robert Flack <fla...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Bokan <bo...@chromium.org>
    Gerrit-CC: Xida Chen <xida...@chromium.org>
    Gerrit-Attention: Robert Flack <fla...@chromium.org>
    Gerrit-Attention: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
    Gerrit-Comment-Date: Thu, 28 Mar 2024 13:41:02 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Kevin Ellis <kev...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Kevin Ellis (Gerrit)

    unread,
    Apr 2, 2024, 4:04:21 PMApr 2
    to Claire Chambers, Blink W3C Test Autoroller, Olga Gerchikov, Robert Flack, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Claire Chambers, Olga Gerchikov and Robert Flack

    Kevin Ellis added 9 comments

    File third_party/blink/renderer/core/animation/css/css_animations_test.cc
    Line 573, Patchset 10 (Parent): ScopedCompositeBGColorAnimationForTest scoped_feature(true);
    Kevin Ellis . unresolved

    Rather than deleting the test, can you update it to reflect the changes to ElementAnimations.

    File third_party/blink/renderer/core/animation/element_animations.cc
    Line 113, Patchset 10 (Latest): KeyframeEffect* effect = DynamicTo<KeyframeEffect>(entry.key->effect());
    Kevin Ellis . unresolved
    ```
    if (entry.key->CalculateAnimationPlayState() == Animation::kIdle) {
    continue;
    }
    KeyframeEffect* effect = ...
    ```

    and drop effect->HasPlayingAnimation. Rationale, a paused animation can still be active, though it is cancelled on the compositor and falls back to main. We are effectively grouping the kNoAnimation and kNotComposited states in the Set...Status() calls below.

    Line 132, Patchset 5: SetCompositedBackgroundColorStatus(CompositedPaintStatus::kNoAnimation);
    Kevin Ellis . unresolved

    Why is this patch changing behavior for background color?

    Kevin Ellis

    The comment does not seem to fully align with the logic in HasAnimationForProperty. The comment suggests it is testing for the case of a single cancelled animation yet the method iterates through all animations on an element to see if any are running and affecting the relevant property. A cancelled animation has playState = idle, and is not updated. The HasPlayingAnimation method will return false if there is a paused animation. This might still need to be repainted. If we see that there is an animation, but it is not running, it should probably fall back to the not-composited state.

    A bit of background on the design. With composite after paint, we have set up paint before deciding if we can composite the animation. Both the paint code and the animation code have to agree on the decision. We had numerous painting bugs related to failing to paint the background color under various edge cases and want to ensure we don't regress. The kNeedsRepaintOrNoAnimation case was to ensure that paint updated the state correctly, as it was in charge of making the definitive compositing decision.

    It appears that the underlying issue is that a cancelled animation can remain in element animations longer than expected. The entry is pruned via a call to the KeyframeEffect::DetachTarget method. It seems risky to add a detach call when entering the idle state since the animation can be reactivated and would need to be reattached. For now, it is OK to simply skip over the idle animations as suggested in a previous comment. Perhaps with a TODO to investigate if cancelled animations should be synchronously pruned from the list.

    Line 144, Patchset 10 (Latest): // If the animation has been canceled, we need to revert back to
    Kevin Ellis . unresolved

    No need to repeat the same comment.

    File third_party/blink/renderer/core/css/post_style_update_scope.cc
    Line 84, Patchset 10 (Latest): element_animations->InvalidatePaintForCompositedAnimationsIfNecessary(
    Kevin Ellis . unresolved

    Not sure if this is the right place for this call. Deferring to andruud@.

    The recommended option in https://issues.chromium.org/issues/40789479 is:
    2. After the style/layout lifecycle, go through compositor-pending animations and invalidate paint directly (somewhere on the LocalFrameView-level).

    Strictly speaking we are still during style update at this stage.

    File third_party/blink/renderer/modules/csspaint/nativepaint/background_color_paint_definition_test.cc
    Line 434, Patchset 10 (Parent):// Test that style->CompositablePaintAnimationChanged() should be true in the
    Kevin Ellis . unresolved

    Rather than deleting these tests due to an obsolete method they could potentially be updated to test the new behavior (i.e. Paint invalidation got triggered).

    File third_party/blink/web_tests/css3/masking/clip-path-animation-stress.html
    Line 41, Patchset 10 (Latest): function swapAnimation() {
    Kevin Ellis . unresolved

    Simpler would be to toggle a class and add the class to the style section above.

    Line 56, Patchset 10 (Latest): await delay(100);
    Kevin Ellis . unresolved

    This test looks susceptible to Timeouts given that it has a total of 5s wait time and the Timeout is 6s.

    File third_party/blink/web_tests/external/wpt/css/css-masking/clip-path/animations/two-clip-path-animation-diff-length4.html
    Line 45, Patchset 10 (Latest): anim.ready.then(() => { return waitForAnimationTime(300); }).then(takeScreenshot);
    Kevin Ellis . unresolved

    Susceptible to test flakes. The waitForAnimationTime method spams requestAnimationFrame. Instead use a long duration and a negative start delay.
    You can then trigger the screenshot as soon as the animation is running and has been rendered.

    await raf();
    anim = ...;
    await anim.ready;
    await raf();
    takeScreenshot();

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Claire Chambers
    • Olga Gerchikov
    • Robert Flack
    Gerrit-Attention: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Attention: Robert Flack <fla...@chromium.org>
    Gerrit-Attention: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-Comment-Date: Tue, 02 Apr 2024 20:04:12 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Kevin Ellis <kev...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Robert Flack (Gerrit)

    unread,
    Apr 3, 2024, 11:50:54 AMApr 3
    to Claire Chambers, Blink W3C Test Autoroller, Olga Gerchikov, Kevin Ellis, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Claire Chambers and Olga Gerchikov

    Robert Flack added 1 comment

    Patchset-level comments
    File-level comment, Patchset 10 (Latest):
    Robert Flack . resolved

    Moving myself to cc, re-add if/when needed. @kev...@chromium.org has the context on this one.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Claire Chambers
    • Olga Gerchikov
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Gerrit-Change-Number: 5384914
    Gerrit-PatchSet: 10
    Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
    Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Bokan <bo...@chromium.org>
    Gerrit-CC: Robert Flack <fla...@chromium.org>
    Gerrit-CC: Xida Chen <xida...@chromium.org>
    Gerrit-Attention: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Attention: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-Comment-Date: Wed, 03 Apr 2024 15:50:37 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Claire Chambers (Gerrit)

    unread,
    Apr 8, 2024, 1:38:15 PMApr 8
    to Robert Flack, Blink W3C Test Autoroller, Olga Gerchikov, Kevin Ellis, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Kevin Ellis and Olga Gerchikov

    Claire Chambers added 9 comments

    File third_party/blink/renderer/core/animation/css/css_animations_test.cc
    Line 573, Patchset 10 (Parent): ScopedCompositeBGColorAnimationForTest scoped_feature(true);
    Kevin Ellis . resolved

    Rather than deleting the test, can you update it to reflect the changes to ElementAnimations.

    Claire Chambers

    Done

    File third_party/blink/renderer/core/animation/element_animations.cc
    Line 113, Patchset 10: KeyframeEffect* effect = DynamicTo<KeyframeEffect>(entry.key->effect());
    Kevin Ellis . resolved
    ```
    if (entry.key->CalculateAnimationPlayState() == Animation::kIdle) {
    continue;
    }
    KeyframeEffect* effect = ...
    ```

    and drop effect->HasPlayingAnimation. Rationale, a paused animation can still be active, though it is cancelled on the compositor and falls back to main. We are effectively grouping the kNoAnimation and kNotComposited states in the Set...Status() calls below.

    Claire Chambers

    I altered this to check for idleness rather than whether it is playing, in accordance with your other comment.

    Line 132, Patchset 5: SetCompositedBackgroundColorStatus(CompositedPaintStatus::kNoAnimation);
    Kevin Ellis . resolved

    Why is this patch changing behavior for background color?

    Kevin Ellis

    The comment does not seem to fully align with the logic in HasAnimationForProperty. The comment suggests it is testing for the case of a single cancelled animation yet the method iterates through all animations on an element to see if any are running and affecting the relevant property. A cancelled animation has playState = idle, and is not updated. The HasPlayingAnimation method will return false if there is a paused animation. This might still need to be repainted. If we see that there is an animation, but it is not running, it should probably fall back to the not-composited state.

    A bit of background on the design. With composite after paint, we have set up paint before deciding if we can composite the animation. Both the paint code and the animation code have to agree on the decision. We had numerous painting bugs related to failing to paint the background color under various edge cases and want to ensure we don't regress. The kNeedsRepaintOrNoAnimation case was to ensure that paint updated the state correctly, as it was in charge of making the definitive compositing decision.

    It appears that the underlying issue is that a cancelled animation can remain in element animations longer than expected. The entry is pruned via a call to the KeyframeEffect::DetachTarget method. It seems risky to add a detach call when entering the idle state since the animation can be reactivated and would need to be reattached. For now, it is OK to simply skip over the idle animations as suggested in a previous comment. Perhaps with a TODO to investigate if cancelled animations should be synchronously pruned from the list.

    Claire Chambers

    Added TODO as per suggestion.

    Line 144, Patchset 10: // If the animation has been canceled, we need to revert back to
    Kevin Ellis . resolved

    No need to repeat the same comment.

    Claire Chambers

    Done

    File third_party/blink/renderer/core/css/post_style_update_scope.cc
    Line 84, Patchset 10: element_animations->InvalidatePaintForCompositedAnimationsIfNecessary(
    Kevin Ellis . unresolved

    Not sure if this is the right place for this call. Deferring to andruud@.

    The recommended option in https://issues.chromium.org/issues/40789479 is:
    2. After the style/layout lifecycle, go through compositor-pending animations and invalidate paint directly (somewhere on the LocalFrameView-level).

    Strictly speaking we are still during style update at this stage.

    Claire Chambers

    I'm absolutely open to moving it elsewhere - anywhere after applying the pending animation update should work.

    I do think putting it here is very convenient as we get to piggyback off of the list of animations with pending updates, which is cleared during this function.

    File third_party/blink/renderer/modules/csspaint/nativepaint/background_color_paint_definition_test.cc
    Line 434, Patchset 10 (Parent):// Test that style->CompositablePaintAnimationChanged() should be true in the
    Kevin Ellis . resolved

    Rather than deleting these tests due to an obsolete method they could potentially be updated to test the new behavior (i.e. Paint invalidation got triggered).

    Claire Chambers

    Done, though certain test had to be further amended. The test that changes the keyframes now uses SetKeyframes(), as is used by the Web Animations API, that way the animation is properly set pending. The test that changes the start time has had its second half removed, as adding the start time the first time cancels the animation effect as it is no longer current/in effect. Changing it the second time does not change its state, and thus does not cause a style recalc and paint invalidation. I believe the only scenario in which this is relevant is animations with positive delays, which are known not to be supported at the moment, so I don't believe this issue needs resolving now.

    File third_party/blink/web_tests/css3/masking/clip-path-animation-stress.html
    Line 41, Patchset 10: function swapAnimation() {
    Kevin Ellis . resolved

    Simpler would be to toggle a class and add the class to the style section above.

    Claire Chambers

    Done

    Line 56, Patchset 10: await delay(100);
    Kevin Ellis . resolved

    This test looks susceptible to Timeouts given that it has a total of 5s wait time and the Timeout is 6s.

    Claire Chambers

    I've cut it down to 20 loops, and this seems to catch the crash just as well. This should avoid running so close to the timeout.

    File third_party/blink/web_tests/external/wpt/css/css-masking/clip-path/animations/two-clip-path-animation-diff-length4.html
    Line 45, Patchset 10: anim.ready.then(() => { return waitForAnimationTime(300); }).then(takeScreenshot);
    Kevin Ellis . unresolved

    Susceptible to test flakes. The waitForAnimationTime method spams requestAnimationFrame. Instead use a long duration and a negative start delay.
    You can then trigger the screenshot as soon as the animation is running and has been rendered.

    await raf();
    anim = ...;
    await anim.ready;
    await raf();
    takeScreenshot();

    Claire Chambers

    I think my comment may not have clearly explained what this test is testing for. Specifically, the case where there is a running, valid cc animation with a curve such that [0,t]-> static frame, (t, animation end) -> changing frame. Without the presence of this animation, the clip path will begin changing at time t. However, if you add a second animation *before* time t, such that the animation has a curve [0,t+n] -> static frame, then this animation should prevent the clip path from changing at time t as this new animation's curve takes precedence.

    The reason for the static frame is such that any change in the clip path will cause paint invalidation independent of any animation change. As a result, having the test set up this way ensures that *just* a new animation with the web animations API will cause the running cc animation to be invalidated. In earlier prototypes of this change, this was not the case. This test would fail as the old animation would still be running despite the addition of a new animation.

    I don't believe your test will achieve the same effect, as adding a second animation *after* time t will alter the clip path property which causes paint invalidation irrespective of any animation due to the change in the clip path property.

    I completely agree that this test methodology is a little janky, and I'm interested in improving it, but I don't want to sacrifice the essential discriminatory element of this test, which is that it should fail when a change to a composited animation does not invalidate paint independently to changes in style.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Kevin Ellis
    • Olga Gerchikov
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Gerrit-Change-Number: 5384914
    Gerrit-PatchSet: 11
    Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
    Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Bokan <bo...@chromium.org>
    Gerrit-CC: Robert Flack <fla...@chromium.org>
    Gerrit-CC: Xida Chen <xida...@chromium.org>
    Gerrit-Attention: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
    Gerrit-Comment-Date: Mon, 08 Apr 2024 17:38:01 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Kevin Ellis <kev...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Kevin Ellis (Gerrit)

    unread,
    Apr 11, 2024, 10:18:54 AMApr 11
    to Claire Chambers, Robert Flack, Blink W3C Test Autoroller, Olga Gerchikov, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Claire Chambers and Olga Gerchikov

    Kevin Ellis added 2 comments

    Patchset-level comments
    File-level comment, Patchset 11 (Latest):
    Kevin Ellis . resolved

    Getting very close. Just one comment left on the test.

    File third_party/blink/web_tests/external/wpt/css/css-masking/clip-path/animations/two-clip-path-animation-diff-length4.html
    Line 45, Patchset 10: anim.ready.then(() => { return waitForAnimationTime(300); }).then(takeScreenshot);
    Kevin Ellis . unresolved

    Susceptible to test flakes. The waitForAnimationTime method spams requestAnimationFrame. Instead use a long duration and a negative start delay.
    You can then trigger the screenshot as soon as the animation is running and has been rendered.

    await raf();
    anim = ...;
    await anim.ready;
    await raf();
    takeScreenshot();

    Claire Chambers

    I think my comment may not have clearly explained what this test is testing for. Specifically, the case where there is a running, valid cc animation with a curve such that [0,t]-> static frame, (t, animation end) -> changing frame. Without the presence of this animation, the clip path will begin changing at time t. However, if you add a second animation *before* time t, such that the animation has a curve [0,t+n] -> static frame, then this animation should prevent the clip path from changing at time t as this new animation's curve takes precedence.

    The reason for the static frame is such that any change in the clip path will cause paint invalidation independent of any animation change. As a result, having the test set up this way ensures that *just* a new animation with the web animations API will cause the running cc animation to be invalidated. In earlier prototypes of this change, this was not the case. This test would fail as the old animation would still be running despite the addition of a new animation.

    I don't believe your test will achieve the same effect, as adding a second animation *after* time t will alter the clip path property which causes paint invalidation irrespective of any animation due to the change in the clip path property.

    I completely agree that this test methodology is a little janky, and I'm interested in improving it, but I don't want to sacrifice the essential discriminatory element of this test, which is that it should fail when a change to a composited animation does not invalidate paint independently to changes in style.

    Kevin Ellis

    My only concern here is that if the test is flaky it gets disabled and we lose test coverage. If you run the test without the rest of your patch, does it reliably fail? Can you get the same failure with a double raf in place of a rafs for 300ms?
    If you run the test 1000 times with your patch (--repeat-each=1000) does it ever TIMEOUT?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Claire Chambers
    • Olga Gerchikov
    Gerrit-Attention: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Attention: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-Comment-Date: Thu, 11 Apr 2024 14:18:42 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Claire Chambers <clcha...@microsoft.com>
    Comment-In-Reply-To: Kevin Ellis <kev...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Kevin Ellis (Gerrit)

    unread,
    Apr 11, 2024, 12:24:38 PMApr 11
    to Claire Chambers, Robert Flack, Blink W3C Test Autoroller, Olga Gerchikov, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Claire Chambers and Olga Gerchikov

    Kevin Ellis added 2 comments

    File third_party/blink/renderer/core/animation/element_animations.cc
    Line 43, Patchset 5 (Parent): CompositedPaintStatus::kNeedsRepaintOrNoAnimation)),
    Kevin Ellis . resolved

    Why are we changing background color status? I don't think you should be touching background color at all.

    Claire Chambers

    Because bgcolor animations rely on CompositablePaintAnimationChanged, same as clip path animations. Since this change makes CompositablePaintAnimationChanged unnecessary, it seemed a good idea to do so for both kinds of animations. They both need paint invalidated for the same reasons.

    I'm curious why we'd want to keep CompositablePaintAnimationChanged for bgcolor but eliminate it for clip paths.

    Kevin Ellis

    Acknowledged

    File third_party/blink/renderer/core/css/post_style_update_scope.cc
    Line 84, Patchset 10: element_animations->InvalidatePaintForCompositedAnimationsIfNecessary(
    Kevin Ellis . resolved

    Not sure if this is the right place for this call. Deferring to andruud@.

    The recommended option in https://issues.chromium.org/issues/40789479 is:
    2. After the style/layout lifecycle, go through compositor-pending animations and invalidate paint directly (somewhere on the LocalFrameView-level).

    Strictly speaking we are still during style update at this stage.

    Claire Chambers

    I'm absolutely open to moving it elsewhere - anywhere after applying the pending animation update should work.

    I do think putting it here is very convenient as we get to piggyback off of the list of animations with pending updates, which is cleared during this function.

    Kevin Ellis

    Acknowledged

    Gerrit-Comment-Date: Thu, 11 Apr 2024 16:24:28 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Claire Chambers (Gerrit)

    unread,
    Apr 16, 2024, 1:39:33 PMApr 16
    to Robert Flack, Blink W3C Test Autoroller, Olga Gerchikov, Kevin Ellis, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Kevin Ellis

    Claire Chambers added 1 comment

    File third_party/blink/web_tests/external/wpt/css/css-masking/clip-path/animations/two-clip-path-animation-diff-length4.html
    Line 45, Patchset 10: anim.ready.then(() => { return waitForAnimationTime(300); }).then(takeScreenshot);
    Kevin Ellis . resolved

    Susceptible to test flakes. The waitForAnimationTime method spams requestAnimationFrame. Instead use a long duration and a negative start delay.
    You can then trigger the screenshot as soon as the animation is running and has been rendered.

    await raf();
    anim = ...;
    await anim.ready;
    await raf();
    takeScreenshot();

    Claire Chambers

    I think my comment may not have clearly explained what this test is testing for. Specifically, the case where there is a running, valid cc animation with a curve such that [0,t]-> static frame, (t, animation end) -> changing frame. Without the presence of this animation, the clip path will begin changing at time t. However, if you add a second animation *before* time t, such that the animation has a curve [0,t+n] -> static frame, then this animation should prevent the clip path from changing at time t as this new animation's curve takes precedence.

    The reason for the static frame is such that any change in the clip path will cause paint invalidation independent of any animation change. As a result, having the test set up this way ensures that *just* a new animation with the web animations API will cause the running cc animation to be invalidated. In earlier prototypes of this change, this was not the case. This test would fail as the old animation would still be running despite the addition of a new animation.

    I don't believe your test will achieve the same effect, as adding a second animation *after* time t will alter the clip path property which causes paint invalidation irrespective of any animation due to the change in the clip path property.

    I completely agree that this test methodology is a little janky, and I'm interested in improving it, but I don't want to sacrifice the essential discriminatory element of this test, which is that it should fail when a change to a composited animation does not invalidate paint independently to changes in style.

    Kevin Ellis

    My only concern here is that if the test is flaky it gets disabled and we lose test coverage. If you run the test without the rest of your patch, does it reliably fail? Can you get the same failure with a double raf in place of a rafs for 300ms?
    If you run the test 1000 times with your patch (--repeat-each=1000) does it ever TIMEOUT?

    Claire Chambers

    It worked fine to 2000x iters before, however I've edited per request.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Kevin Ellis
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Gerrit-Change-Number: 5384914
    Gerrit-PatchSet: 11
    Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
    Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Bokan <bo...@chromium.org>
    Gerrit-CC: Robert Flack <fla...@chromium.org>
    Gerrit-CC: Xida Chen <xida...@chromium.org>
    Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
    Gerrit-Comment-Date: Tue, 16 Apr 2024 17:39:21 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Claire Chambers (Gerrit)

    unread,
    Apr 19, 2024, 9:01:41 AMApr 19
    to Anders Hartvoll Ruud, Robert Flack, Blink W3C Test Autoroller, Olga Gerchikov, Kevin Ellis, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, dpr...@google.com, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Anders Hartvoll Ruud and Kevin Ellis

    Claire Chambers added 1 comment

    File third_party/blink/renderer/core/css/post_style_update_scope.cc
    Line 84, Patchset 10: element_animations->InvalidatePaintForCompositedAnimationsIfNecessary(
    Kevin Ellis . resolved

    Not sure if this is the right place for this call. Deferring to andruud@.

    The recommended option in https://issues.chromium.org/issues/40789479 is:
    2. After the style/layout lifecycle, go through compositor-pending animations and invalidate paint directly (somewhere on the LocalFrameView-level).

    Strictly speaking we are still during style update at this stage.

    Claire Chambers

    I'm absolutely open to moving it elsewhere - anywhere after applying the pending animation update should work.

    I do think putting it here is very convenient as we get to piggyback off of the list of animations with pending updates, which is cleared during this function.

    Kevin Ellis

    Acknowledged

    Claire Chambers

    Adding them as reviewer.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • Kevin Ellis
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Gerrit-Change-Number: 5384914
    Gerrit-PatchSet: 13
    Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
    Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Bokan <bo...@chromium.org>
    Gerrit-CC: Robert Flack <fla...@chromium.org>
    Gerrit-CC: Xida Chen <xida...@chromium.org>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Attention: Kevin Ellis <kev...@chromium.org>
    Gerrit-Comment-Date: Fri, 19 Apr 2024 13:01:28 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Dominique Fauteux-Chapleau (Gerrit)

    unread,
    Apr 19, 2024, 9:11:10 AMApr 19
    to Claire Chambers, dpr...@google.com, Anders Hartvoll Ruud, Robert Flack, Blink W3C Test Autoroller, Olga Gerchikov, Kevin Ellis, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Anders Hartvoll Ruud and Kevin Ellis

    Dominique Fauteux-Chapleau removed dpr...@google.com from this change

    Deleted Reviewers:
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • Kevin Ellis
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: deleteReviewer
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Kevin Ellis (Gerrit)

    unread,
    Apr 22, 2024, 9:53:22 AMApr 22
    to Claire Chambers, Anders Hartvoll Ruud, Robert Flack, Blink W3C Test Autoroller, Olga Gerchikov, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Anders Hartvoll Ruud and Claire Chambers

    Kevin Ellis voted and added 2 comments

    Votes added by Kevin Ellis

    Code-Review+1

    2 comments

    Patchset-level comments
    File-level comment, Patchset 13 (Latest):
    Kevin Ellis . resolved

    LGTM % nit

    File third_party/blink/renderer/core/animation/css/css_animations_test.cc
    Line 652, Patchset 13 (Latest): // TODO(crbug.com/1245806): We could invalidate the keyframes by using e.g.
    Kevin Ellis . unresolved

    Nit: This issue is marked as fixed and might no longer be relevant. OK with artificially invalidating, but we should consider removing the comment if misleading.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Anders Hartvoll Ruud
    • Claire Chambers
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Gerrit-Change-Number: 5384914
    Gerrit-PatchSet: 13
    Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
    Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Bokan <bo...@chromium.org>
    Gerrit-CC: Robert Flack <fla...@chromium.org>
    Gerrit-CC: Xida Chen <xida...@chromium.org>
    Gerrit-Attention: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Attention: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Comment-Date: Mon, 22 Apr 2024 13:53:04 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Anders Hartvoll Ruud (Gerrit)

    unread,
    Apr 23, 2024, 5:33:19 AMApr 23
    to Claire Chambers, Kevin Ellis, Robert Flack, Blink W3C Test Autoroller, Olga Gerchikov, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org
    Attention needed from Claire Chambers

    Anders Hartvoll Ruud voted and added 2 comments

    Votes added by Anders Hartvoll Ruud

    Code-Review+1

    2 comments

    Patchset-level comments
    Anders Hartvoll Ruud . resolved

    lgtm for VirtualTestSuites, defer to kevers@ for everything else.

    File third_party/blink/renderer/core/css/post_style_update_scope.cc
    Line 84, Patchset 10: element_animations->InvalidatePaintForCompositedAnimationsIfNecessary(
    Kevin Ellis . resolved

    Not sure if this is the right place for this call. Deferring to andruud@.

    The recommended option in https://issues.chromium.org/issues/40789479 is:
    2. After the style/layout lifecycle, go through compositor-pending animations and invalidate paint directly (somewhere on the LocalFrameView-level).

    Strictly speaking we are still during style update at this stage.

    Claire Chambers

    I'm absolutely open to moving it elsewhere - anywhere after applying the pending animation update should work.

    I do think putting it here is very convenient as we get to piggyback off of the list of animations with pending updates, which is cleared during this function.

    Kevin Ellis

    Acknowledged

    Claire Chambers

    Adding them as reviewer.

    Anders Hartvoll Ruud

    Sorry for the delay. This call site seems fine, thanks for tackling this issue! :-)

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Claire Chambers
    Gerrit-Comment-Date: Tue, 23 Apr 2024 09:33:06 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Claire Chambers (Gerrit)

    unread,
    Apr 24, 2024, 12:17:44 PMApr 24
    to Anders Hartvoll Ruud, Kevin Ellis, Robert Flack, Blink W3C Test Autoroller, Olga Gerchikov, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org

    Claire Chambers added 1 comment

    File third_party/blink/renderer/core/animation/css/css_animations_test.cc
    Line 652, Patchset 13: // TODO(crbug.com/1245806): We could invalidate the keyframes by using e.g.
    Kevin Ellis . resolved

    Nit: This issue is marked as fixed and might no longer be relevant. OK with artificially invalidating, but we should consider removing the comment if misleading.

    Claire Chambers

    Done

    Open in Gerrit

    Related details

    Attention set is empty
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Gerrit-Change-Number: 5384914
    Gerrit-PatchSet: 13
    Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
    Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Bokan <bo...@chromium.org>
    Gerrit-CC: Robert Flack <fla...@chromium.org>
    Gerrit-CC: Xida Chen <xida...@chromium.org>
    Gerrit-Comment-Date: Wed, 24 Apr 2024 16:17:31 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Kevin Ellis <kev...@chromium.org>
    satisfied_requirement
    open
    diffy

    Claire Chambers (Gerrit)

    unread,
    Apr 24, 2024, 12:17:54 PMApr 24
    to Anders Hartvoll Ruud, Kevin Ellis, Robert Flack, Blink W3C Test Autoroller, Olga Gerchikov, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, Chromium LUCI CQ, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org

    Claire Chambers voted Commit-Queue+2

    Commit-Queue+2
    Open in Gerrit

    Related details

    Attention set is empty
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Gerrit-Change-Number: 5384914
    Gerrit-PatchSet: 14
    Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
    Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>
    Gerrit-CC: David Bokan <bo...@chromium.org>
    Gerrit-CC: Robert Flack <fla...@chromium.org>
    Gerrit-CC: Xida Chen <xida...@chromium.org>
    Gerrit-Comment-Date: Wed, 24 Apr 2024 16:17:41 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    open
    diffy

    Chromium LUCI CQ (Gerrit)

    unread,
    Apr 24, 2024, 1:25:19 PMApr 24
    to Claire Chambers, Anders Hartvoll Ruud, Kevin Ellis, Robert Flack, Blink W3C Test Autoroller, Olga Gerchikov, AyeAye, Tricium, Alexis Menard, David Bokan, chromium...@chromium.org, Xida Chen, blink-revie...@chromium.org, blink-rev...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, zol...@webkit.org

    Chromium LUCI CQ submitted the change with unreviewed changes

    Unreviewed changes

    13 is the latest approved patch-set.
    The change was submitted with unreviewed changes in the following files:

    ```
    The name of the file: third_party/blink/renderer/core/animation/css/css_animations_test.cc
    Insertions: 0, Deletions: 5.

    @@ -648,11 +648,6 @@
    EXPECT_TRUE(element->ComputedStyleRef().HasCurrentBackgroundColorAnimation());

    // Change compositor snapshot values:
    - //
    - // TODO(crbug.com/1245806): We could invalidate the keyframes by using e.g.
    - // var()-references, and changing them in the base style, but it does
    - // currently not work for composited animations, hence the snapshot is
    - // invalidated artificially.
    InvalidateCompositorKeyframesSnapshot(animation);
    // Also do an "unrelated" change, to avoid IsAnimationStyleChange()==true.
    element->classList().toggle(AtomicString("unrelated"), ASSERT_NO_EXCEPTION);
    ```

    Change information

    Commit message:
    Make Composited Animations Invalidate Paint Directly

    The previous system of relying on synthetic style diffs to produce paint invalidation proved too unwieldly, and created issues where animation changes in the pending animation update would cause crashes in certain situations.

    This CL completely removes CompositablePaintAnimationChanged and replaces it with an extra hook in post style update scope. Any animations that are compositor pending or have been canceled have their corresponding layout objects invalidated.

    Bug: 1246826
    Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Reviewed-by: Anders Hartvoll Ruud <and...@chromium.org>
    Commit-Queue: Claire Chambers <clcha...@microsoft.com>
    Reviewed-by: Kevin Ellis <kev...@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#1291962}
    Files:
    • M third_party/blink/renderer/core/animation/animation.cc
    • M third_party/blink/renderer/core/animation/css/css_animations.cc
    • M third_party/blink/renderer/core/animation/css/css_animations_test.cc
    • M third_party/blink/renderer/core/animation/element_animations.cc
    • M third_party/blink/renderer/core/animation/element_animations.h
    • M third_party/blink/renderer/core/css/post_style_update_scope.cc
    • M third_party/blink/renderer/core/dom/element.cc
    • M third_party/blink/renderer/core/layout/layout_object.cc
    • M third_party/blink/renderer/core/paint/box_painter_base.cc
    • M third_party/blink/renderer/core/style/computed_style.cc
    • M third_party/blink/renderer/core/style/computed_style_extra_fields.json5
    • M third_party/blink/renderer/modules/csspaint/nativepaint/background_color_paint_definition_test.cc
    • M third_party/blink/renderer/modules/csspaint/nativepaint/clip_path_paint_definition_test.cc
    • M third_party/blink/web_tests/VirtualTestSuites
    • A third_party/blink/web_tests/css3/masking/clip-path-animation-stress.html
    • A third_party/blink/web_tests/external/wpt/css/css-masking/clip-path/animations/two-clip-path-animation-diff-length4.html
    Change size: L
    Delta: 16 files changed, 307 insertions(+), 207 deletions(-)
    Branch: refs/heads/main
    Submit Requirements:
    • requirement satisfiedCode-Review: +1 by Kevin Ellis, +1 by Anders Hartvoll Ruud
    Open in Gerrit
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: merged
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Ib071d09b337ec786188aa5323dd4e6a95cfb962f
    Gerrit-Change-Number: 5384914
    Gerrit-PatchSet: 15
    Gerrit-Owner: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Anders Hartvoll Ruud <and...@chromium.org>
    Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
    Gerrit-Reviewer: Claire Chambers <clcha...@microsoft.com>
    Gerrit-Reviewer: Kevin Ellis <kev...@chromium.org>
    Gerrit-Reviewer: Olga Gerchikov <gerc...@microsoft.com>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Blink W3C Test Autoroller <blink-w3c-te...@chromium.org>