[animation-trigger] Parse trigger() function into computed style [chromium/src : main]
0 views
Skip to first unread message
AI Code Reviewer (Gerrit)
Sep 4, 2025, 2:54:00 PM (5 days ago) Sep 4
to David Awogbemila, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from David Awogbemila
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
File third_party/blink/renderer/core/css/properties/css_parsing_utils.h
Line 698, Patchset 1 (Latest):
CSSParserTokenStream& stream,AI Code Reviewer . unresolved
Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. The parameter names `stream` and `context` are clear from their types and can be omitted from the function declaration in the header file.
***
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason \
\
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent). \
AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve. \
[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
File third_party/blink/renderer/core/style/style_trigger_attachment.h
Line 42, Patchset 1 (Latest):
HeapVector<std::pair<AtomicString, AtomicString>> action_behavior_pairs_;AI Code Reviewer . unresolved
Blink Style Guide: Prefer blink:: types over STL and base types. Please use `WTF::KeyValuePair` instead of `std::pair` for the member variable `action_behavior_pairs_`.
***
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason \
\
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent). \
AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve. \
[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
Attention is currently required from:
- David Awogbemila
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Sep 5, 2025, 5:33:51 AM (5 days ago) Sep 5
to David Awogbemila, AI Code Reviewer, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from David Awogbemila
Anders Hartvoll Ruud added 10 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 3 (Latest):
Anders Hartvoll Ruud . unresolved
Looks good as long as we get the testing situation sorted. :-)
Commit Message
Line 24, Patchset 3 (Latest):
keep things simple. It shuold be easy enough to restrict this if such aAnders Hartvoll Ruud . unresolved
nit
File third_party/blink/renderer/core/animation/css/css_animations_test.cc
Line 1207, Patchset 3 (Latest):
// TODO(crbug.com/441408561): Enable when the actual attaching uses the
// updated parsingAnders Hartvoll Ruud . unresolved
I'd make the update here instead of disabling the tests.
File third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
Line 4427, Patchset 3 (Latest):
ConsumeCommaIncludingWhitespace(stream);Anders Hartvoll Ruud . unresolved
We should check the return value of this, right?
Line 4444, Patchset 3 (Latest):
// A ')' after an action-behavior pair indicates the end of the trigger()
// function.
if (stream.Peek().GetType() == kRightParenthesisToken) {
break;
}Anders Hartvoll Ruud . unresolved
Not needed; this token is treated as `EOF` inside the block (`stream.AtEnd()` returns true at this token.)
Line 4453, Patchset 3 (Latest):
if (trigger_name && !action_behavior_pairs.empty()) {
trigger_attachment_value =
MakeGarbageCollected<cssvalue::CSSTriggerAttachmentValue>(
trigger_name, action_behavior_pairs);
}
return trigger_attachment_value;Anders Hartvoll Ruud . unresolved
I don't think `trigger_attachment_value` is needed?
```suggestion
if (trigger_name && !action_behavior_pairs.empty()) {
return MakeGarbageCollected<cssvalue::CSSTriggerAttachmentValue>(
trigger_name, action_behavior_pairs);
}
return nullptr;
```
(Run `git cl format` on that, though.)
File third_party/blink/renderer/core/css/resolver/css_to_style_map.cc
Line 907, Patchset 3 (Latest):
return nullptr;Anders Hartvoll Ruud . unresolved
This can not happen. I'd use `const auto& attachment_value = To<cssvalue::CSSTriggerAttachmentValue>(value)` at the top instead.
Line 920, Patchset 3 (Latest):
return nullptr;Anders Hartvoll Ruud . unresolved
Can this happen? Isn't it either `none` or a list? If it can, we should cover this with a test. Otherwise, just confidently use `To<CSSValueList>(value)`.
File third_party/blink/web_tests/TestExpectations
Line 9574, Patchset 3 (Latest):
crbug.com/429392773 external/wpt/scroll-animations/animation-trigger/* [ Failure Timeout ]Anders Hartvoll Ruud . unresolved
This seems unwise. I would _recommend_ updating all the tests in the same CL to maintain unbroken coverage of the feature.
File third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/parsing/animation-trigger-parsing.tentative.html
Line 130, Patchset 3 (Latest):
test_invalid_value('animation-trigger', 'trigger(--abc, play)');Anders Hartvoll Ruud . unresolved
I suggest adding:
```
test_invalid_value('animation-trigger', 'trigger(--abc click play)');
test_invalid_value('animation-trigger', 'trigger(--abc, click play click play)');
```
Related details
Attention is currently required from:
- David Awogbemila
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
Sep 5, 2025, 3:48:55 PM (4 days ago) Sep 5
to Anders Hartvoll Ruud, AI Code Reviewer, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Anders Hartvoll Ruud
David Awogbemila added 12 comments![]( "Open in Gerrit")
Patchset-level comments
Looks good as long as we get the testing situation sorted. :-)
David Awogbemila
Acknowledged
Commit Message
Line 24, Patchset 3:
keep things simple. It shuold be easy enough to restrict this if such aAnders Hartvoll Ruud . resolved
David Awogbemilanit
Done
File third_party/blink/renderer/core/animation/css/css_animations_test.cc
Line 1207, Patchset 3:
// TODO(crbug.com/441408561): Enable when the actual attaching uses the
// updated parsingAnders Hartvoll Ruud . resolved
I'd make the update here instead of disabling the tests.
David Awogbemila
Sounds good.
File third_party/blink/renderer/core/css/properties/css_parsing_utils.h
Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. The parameter names `stream` and `context` are clear from their types and can be omitted from the function declaration in the header file.
***_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason \
\
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent). \
AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve. \
[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
David Awogbemila
Acknowledged
File third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
We should check the return value of this, right?
David Awogbemila
Yes, good catch. I've added the tests you suggested below which would have caught this.
Line 4444, Patchset 3:
// A ')' after an action-behavior pair indicates the end of the trigger()
// function.
if (stream.Peek().GetType() == kRightParenthesisToken) {
break;
}Anders Hartvoll Ruud . resolved
Not needed; this token is treated as `EOF` inside the block (`stream.AtEnd()` returns true at this token.)
David Awogbemila
Done.
Line 4453, Patchset 3:
if (trigger_name && !action_behavior_pairs.empty()) {
trigger_attachment_value =
MakeGarbageCollected<cssvalue::CSSTriggerAttachmentValue>(
trigger_name, action_behavior_pairs);
}
return trigger_attachment_value;Anders Hartvoll Ruud . resolved
I don't think `trigger_attachment_value` is needed?
```suggestion
if (trigger_name && !action_behavior_pairs.empty()) {
return MakeGarbageCollected<cssvalue::CSSTriggerAttachmentValue>(
trigger_name, action_behavior_pairs);
}return nullptr;
```(Run `git cl format` on that, though.)
David Awogbemila
Done.
File third_party/blink/renderer/core/css/resolver/css_to_style_map.cc
This can not happen. I'd use `const auto& attachment_value = To<cssvalue::CSSTriggerAttachmentValue>(value)` at the top instead.
David Awogbemila
Done.
Can this happen? Isn't it either `none` or a list? If it can, we should cover this with a test. Otherwise, just confidently use `To<CSSValueList>(value)`.
David Awogbemila
Done.
File third_party/blink/renderer/core/style/style_trigger_attachment.h
Line 42, Patchset 1:
HeapVector<std::pair<AtomicString, AtomicString>> action_behavior_pairs_;AI Code Reviewer . resolved
Blink Style Guide: Prefer blink:: types over STL and base types. Please use `WTF::KeyValuePair` instead of `std::pair` for the member variable `action_behavior_pairs_`.
***_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options: \
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason \
\
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent). \
AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve. \
[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
David Awogbemila
Won't fix: "error: static assertion failed due to requirement 'internal::IsTraceableV<blink::KeyValuePair"
File third_party/blink/web_tests/TestExpectations
Line 9574, Patchset 3:
crbug.com/429392773 external/wpt/scroll-animations/animation-trigger/* [ Failure Timeout ]Anders Hartvoll Ruud . resolved
This seems unwise. I would _recommend_ updating all the tests in the same CL to maintain unbroken coverage of the feature.
David Awogbemila
Sounds good. I've updated the tests. No need for the TestExpectations entry now.
File third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/parsing/animation-trigger-parsing.tentative.html
Line 130, Patchset 3:
test_invalid_value('animation-trigger', 'trigger(--abc, play)');Anders Hartvoll Ruud . resolved
I suggest adding:
```
test_invalid_value('animation-trigger', 'trigger(--abc click play)');
test_invalid_value('animation-trigger', 'trigger(--abc, click play click play)');
```
Attention is currently required from:
- Anders Hartvoll Ruud
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Sep 6, 2025, 3:36:04 AM (4 days ago) Sep 6
to David Awogbemila, AI Code Reviewer, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from David Awogbemila
Anders Hartvoll Ruud voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- David Awogbemila
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
Sep 6, 2025, 4:35:35 AM (4 days ago) Sep 6
to Anders Hartvoll Ruud, AI Code Reviewer, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
David Awogbemila voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Sep 6, 2025, 4:53:39 AM (4 days ago) Sep 6
to David Awogbemila, Anders Hartvoll Ruud, AI Code Reviewer, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[animation-trigger] Parse trigger() function into computed style
`animation-trigger` previously specified a comma-separated list of
space-separated lists of dashed idents which were names of triggers,
i.e:
animation-trigger: [ [ <dashed-ident>]+ ]#
Now, per [1], `animation-trigger` is parsed as:
/*action*/ /*behavior*/
<action-behavior>: <ident> <ident>
<single-animation-trigger>: trigger(<dashed-ident>, <action-behavior> [, <action-behavior>]+)
animation-trigger: [ [<single-animation-trigger>]+ ]#
The CSSWG might restrict the idents that can be specified as actions
and/or behaviors but for now I've kept them as CSSCustomIdentValue to
keep things simple. It should be easy enough to restrict this if such a
resolution is passed.
[1] https://github.com/w3c/csswg-drafts/issues/12652
R=and...@chromium.org
Bug: 441408561
Change-Id: Ibbd7dd0f349b268fd690e1d1180a518ae20706ae
Commit-Queue: David Awogbemila <awogb...@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <and...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1512009}
Files:
- M third_party/blink/renderer/build/scripts/core/style/make_computed_style_base.py
- M third_party/blink/renderer/core/animation/animation_timeline.cc
- M third_party/blink/renderer/core/animation/css/css_animation.cc
- M third_party/blink/renderer/core/animation/css/css_animation.h
- M third_party/blink/renderer/core/animation/css/css_animation_data.cc
- M third_party/blink/renderer/core/animation/css/css_animation_data.h
- M third_party/blink/renderer/core/animation/css/css_animation_update.h
- 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/css/build.gni
- M third_party/blink/renderer/core/css/css_properties.json5
- A third_party/blink/renderer/core/css/css_trigger_attachment_value.cc
- A third_party/blink/renderer/core/css/css_trigger_attachment_value.h
- M third_party/blink/renderer/core/css/css_value.cc
- M third_party/blink/renderer/core/css/css_value.h
- M third_party/blink/renderer/core/css/css_value_keywords.json5
- M third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
- M third_party/blink/renderer/core/css/properties/css_parsing_utils.h
- M third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
- M third_party/blink/renderer/core/css/resolver/css_to_style_map.cc
- M third_party/blink/renderer/core/css/resolver/css_to_style_map.h
- M third_party/blink/renderer/core/style/build.gni
- M third_party/blink/renderer/core/style/computed_style.cc
- A third_party/blink/renderer/core/style/style_trigger_attachment.h
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-alternate.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-fill-mode-both.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-fill-mode-none.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-late-attached-timeline.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-once-play-state.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-once.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-repeat.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-state.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/parsing/animation-trigger-parsing.tentative.html
Change size: L
Delta: 33 files changed, 623 insertions(+), 162 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Anders Hartvoll Ruud
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Sep 6, 2025, 5:39:31 AM (4 days ago) Sep 6
to Chromium LUCI CQ, David Awogbemila, Anders Hartvoll Ruud, AI Code Reviewer, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
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/54730
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages