[anchor] Main thread animation if first frame doesn't change anything. [chromium/src : main]
Morten Stenshorne (Gerrit)
Morten Stenshorne added 2 comments![]( "Open in Gerrit")
[anchor] Request main frame animation for delayed animation start.Morten Stenshorne"start" and "delayed" have very specific meanings in the animation space. This is about running animations where the applied effect does not change the style of the element, right?
Rune LillesveenThat's right. If there are no style changes at the very beginning of the animation, layout would fail to respond on any style changes that would happen later on during the animation.
Morten StenshorneI don't understand why.
We have discussed on the outside of this review already, and I've also updated the CL's description. Hopefully this is clearer now.
needed.Morten StenshorneWhy don't we get a re-layout when the applied translateX() animation starts applying translations different from 0 on the main thread without the change in this CL?
Rune LillesveenBecause it all happens on the compositor thread otherwise, I suppose.
Morten StenshorneThe animation has to apply its effects on the main thread too. Otherwise you get the wrong results from getComputedStyle().
getComputedStyle() itself will run on the main thread, and therefore ensure that the styles are updated before it returns. However, if the animation is running on the compositor thread, and nothing plucks the main thread in the meantime (getComputedStyle(), wiggling the mouse, etc.) computed style won't be updated for the entire duration of the animation.
Related details
- Rune Lillesveen
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager voted and added 1 comment![]( "Open in Gerrit")
- Morten Stenshorne
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
[anchor] Request main frame animation for delayed animation start.Morten Stenshorne"start" and "delayed" have very specific meanings in the animation space. This is about running animations where the applied effect does not change the style of the element, right?
Rune LillesveenThat's right. If there are no style changes at the very beginning of the animation, layout would fail to respond on any style changes that would happen later on during the animation.
Morten StenshorneI don't understand why.
We have discussed on the outside of this review already, and I've also updated the CL's description. Hopefully this is clearer now.
More discussion outside the review: we think the IsRunningTransformRelatedAnimationOnCompositor flag should not be custom_compare anymore, since it can affect invalidation.
Related details
- Morten Stenshorne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
[anchor] Request main frame animation for delayed animation start.Morten Stenshorne"start" and "delayed" have very specific meanings in the animation space. This is about running animations where the applied effect does not change the style of the element, right?
Rune LillesveenThat's right. If there are no style changes at the very beginning of the animation, layout would fail to respond on any style changes that would happen later on during the animation.
Morten StenshorneI don't understand why.
Rune LillesveenWe have discussed on the outside of this review already, and I've also updated the CL's description. Hopefully this is clearer now.
More discussion outside the review: we think the IsRunningTransformRelatedAnimationOnCompositor flag should not be custom_compare anymore, since it can affect invalidation.
I did this in the latest patch set, uploaded yesterday. However, this change will break the `ComputedStyleTest.AnimationFlags` unit test.
The test currently allows for two types of results. A flag change is expected to either trigger `!HasDifference()` and `ComputedStyle::Difference::kEqual`, or it should trigger `HasDifference()` and a specific expected `ComputedStyle::Difference` value.
Now we end up with `!HasDifference()` and `ComputedStyle::Difference::kNonInherited`.
Not sure if we should allow for that to happen.
Related details
- Rune Lillesveen
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
[anchor] Request main frame animation for delayed animation start.Morten Stenshorne"start" and "delayed" have very specific meanings in the animation space. This is about running animations where the applied effect does not change the style of the element, right?
Rune LillesveenThat's right. If there are no style changes at the very beginning of the animation, layout would fail to respond on any style changes that would happen later on during the animation.
Morten StenshorneI don't understand why.
Rune LillesveenWe have discussed on the outside of this review already, and I've also updated the CL's description. Hopefully this is clearer now.
Morten StenshorneMore discussion outside the review: we think the IsRunningTransformRelatedAnimationOnCompositor flag should not be custom_compare anymore, since it can affect invalidation.
I did this in the latest patch set, uploaded yesterday. However, this change will break the `ComputedStyleTest.AnimationFlags` unit test.
The test currently allows for two types of results. A flag change is expected to either trigger `!HasDifference()` and `ComputedStyle::Difference::kEqual`, or it should trigger `HasDifference()` and a specific expected `ComputedStyle::Difference` value.
Now we end up with `!HasDifference()` and `ComputedStyle::Difference::kNonInherited`.
Not sure if we should allow for that to happen.
I think that's expected if you don't have an `invalidation` entry for your property/flag in the relevant json file.
Related details
- Morten Stenshorne
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
[anchor] Request main frame animation for delayed animation start.Morten Stenshorne"start" and "delayed" have very specific meanings in the animation space. This is about running animations where the applied effect does not change the style of the element, right?
Rune LillesveenThat's right. If there are no style changes at the very beginning of the animation, layout would fail to respond on any style changes that would happen later on during the animation.
Morten StenshorneI don't understand why.
Rune LillesveenWe have discussed on the outside of this review already, and I've also updated the CL's description. Hopefully this is clearer now.
Morten StenshorneMore discussion outside the review: we think the IsRunningTransformRelatedAnimationOnCompositor flag should not be custom_compare anymore, since it can affect invalidation.
Rune LillesveenI did this in the latest patch set, uploaded yesterday. However, this change will break the `ComputedStyleTest.AnimationFlags` unit test.
The test currently allows for two types of results. A flag change is expected to either trigger `!HasDifference()` and `ComputedStyle::Difference::kEqual`, or it should trigger `HasDifference()` and a specific expected `ComputedStyle::Difference` value.
Now we end up with `!HasDifference()` and `ComputedStyle::Difference::kNonInherited`.
Not sure if we should allow for that to happen.
I think that's expected if you don't have an `invalidation` entry for your property/flag in the relevant json file.
- Rune Lillesveen
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 3 comments![]( "Open in Gerrit")
needed.Morten StenshorneWhy don't we get a re-layout when the applied translateX() animation starts applying translations different from 0 on the main thread without the change in this CL?
Rune LillesveenBecause it all happens on the compositor thread otherwise, I suppose.
Morten StenshorneThe animation has to apply its effects on the main thread too. Otherwise you get the wrong results from getComputedStyle().
getComputedStyle() itself will run on the main thread, and therefore ensure that the styles are updated before it returns. However, if the animation is running on the compositor thread, and nothing plucks the main thread in the meantime (getComputedStyle(), wiggling the mouse, etc.) computed style won't be updated for the entire duration of the animation.
Acknowledged
<title>Animated anchor transform, delayed animation start</title>Morten StenshorneI don't see any delayed start of the animation here? You mean the animation stays at translateX(0) for half the animation?
Rune LillesveenYes, that's it.
"animation effect not changing computed value" or something, then?
try { await done.promise; } catch(e) {}Morten StenshorneWhy try/catch this one?
Rune LillesveenI think I got the test runner to freeze or crash otherwise, at least if the test fails.
If this throws an exception while got_halfway is set to true, we will silently pass, right?
We should make catching an exception here make the test fail, right?
Related details
- Morten Stenshorne
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 2 comments![]( "Open in Gerrit")
<title>Animated anchor transform, delayed animation start</title>Morten StenshorneI don't see any delayed start of the animation here? You mean the animation stays at translateX(0) for half the animation?
Rune LillesveenYes, that's it.
"animation effect not changing computed value" or something, then?
Done
try { await done.promise; } catch(e) {}Morten StenshorneWhy try/catch this one?
Rune LillesveenI think I got the test runner to freeze or crash otherwise, at least if the test fails.
If this throws an exception while got_halfway is set to true, we will silently pass, right?
We should make catching an exception here make the test fail, right?
Since I don't quite remember why I added this, and since it passes without it, I'll just remove the try-catch.
Related details
- Rune Lillesveen
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rune Lillesveen
| Code-Review | +1 |
1 comment
try { await done.promise; } catch(e) {}Morten StenshorneWhy try/catch this one?
Rune LillesveenI think I got the test runner to freeze or crash otherwise, at least if the test fails.
Morten StenshorneIf this throws an exception while got_halfway is set to true, we will silently pass, right?
We should make catching an exception here make the test fail, right?
Since I don't quite remember why I added this, and since it passes without it, I'll just remove the try-catch.
- Morten Stenshorne
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
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/57230.
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
Related details
- Morten Stenshorne
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne voted Commit-Queue+2![]( "Open in Gerrit")
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
13 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[anchor] Main thread animation if first frame doesn't change anything.
This is a follow-up to https://crrev.com/c/7123703 , but it was blocked
on other problems, which got fixed by https://crrev.com/c/7239030 . We
can now get anchor animations that don't change any styles right away
(i.e. no visual updates until further into the animation) to trigger
relayout of anchor-positioned elements when needed.
If the very first animation frame actually has style changes that affect
layout (e.g. transforms that affect an anchor), we still haven't moved
the work off the main thread, and we'll therefore lay out, and by doing
that, HasRunningAnchorTransformAnimation() in PhysicalFragment will
return true, which in turn will make sure that the animation keeps
running on the main thread. This was already working.
On the other hand, if the first frame of an animation *doesn't* change
style that affects layout, the animation would be moved to the
compositor thread, and we'd never hear from it again, not even when any
style changes actually occur later on. To fix this, detect that a
transform-related animation wants to run on the compositor, mark for
layout, so that the physical fragments get updated for
HasRunningAnchorTransformAnimation(), so that we'll be able to keep the
animation on the main thread
Note: The test included would only fail with run_wpt_tests, not
run_web_tests.
- M third_party/blink/renderer/core/layout/layout_object.cc
- M third_party/blink/renderer/core/style/computed_style.h
- M third_party/blink/renderer/core/style/computed_style_extra_fields.json5
- M third_party/blink/renderer/core/style/computed_style_test.cc
- A third_party/blink/web_tests/external/wpt/css/css-anchor-position/transform-016.html
Code-Review: +1 by Rune Lillesveen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57230
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |