[animation-trigger] Implement trigger-scope [chromium/src : main]
David Awogbemila (Gerrit)
David Awogbemila added 1 comment![]( "Open in Gerrit")
Hi Anders, ptal at this trigger-scope implementation, thanks.
I've tried to unify the parts that are shared with anchor-scope.
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 8 comments![]( "Open in Gerrit")
Generally looks reasonable. I just have some comments on naming/readability, etc.
typedef GCedHeapHashMap<Member<const ScopedTrigger>, Member<AnimationTrigger>>I think we prefer `using`?
using ScopedTrigger = NamingScope;`TriggerScopedName`?
It's tricky, because these terms are overloaded in multiple ways:
"Scope triggers" is already something you can add to an element in relation to something else (`@scope`): https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/resolver/scoped_style_resolver.cc;bpv=1;bpt=1;l=490?q=StyleResolver::AddImplicitScopeTrigger . Although it's not a widely-known concept, I'd like to avoid confusing these as much as possible.
"Scoped" already means "scoped as in TreeScope/ShadowDOM" in many cases. We've since gained other kinds of scoping: anchor-scope-scoping, trigger-scope-scoping. `TriggerScopedName` _hopefully_ indicates that this is scoped per `trigger-scope`, and not some other scoping mechanism.
Suggestion:
```
// A name scoped according to the 'trigger-scope' property.
//
// https://link-to-property-in-spec
using TriggerScopedName = NamingScope;
```
And similarly for `AnchorScopedName`.
#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_DOM_TRIGGER_SCOPE_H_I think we should call this `trigger_scoped_name.h/cc`.
(`anchor_scope.h` should probably also have been `anchor_scoped_name.h/cc`)
PropagateScopedTriggers(child);As mentioned elsewhere, I'd avoid "ScopedTrigger". I think we can probably just keep "NamedTrigger" for the function names, but now deal with objects of type `TriggerScopedName`s?
static const Element* FindScopeElement(
const ScopedCSSName& name,
const Element& start_element,
base::FunctionRef<StyleNameScope(const ComputedStyle& style)> get_scope);Let's explain a bit here:
```suggestion
// This function finds an element in the inclusive-ancestor chain
// of `start_element` that scopes `name` to that element via some property,
// e.g. 'anchor-scope'.
static const Element* FindScopeElement(
const ScopedCSSName& name,
const Element& start_element,
base::FunctionRef<StyleNameScope(const ComputedStyle& style)> get_scope);
```
class CORE_EXPORT NamingScope : public GarbageCollected<NamingScope> {Especially now as a more general util, this could really use some documentation explaining what it's for.
static bool IsWithinScope(const ScopedCSSName& lookup_name,
const StyleNameScope& scope);
static const Element* FindScopeElement(
const ScopedCSSName& name,
const Element& start_element,
base::FunctionRef<StyleNameScope(const ComputedStyle& style)> get_scope);I guess these ended up not being defined in `StyleNameScope`?
Related details
- David Awogbemila
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. |
David Awogbemila (Gerrit)
David Awogbemila added 7 comments![]( "Open in Gerrit")
typedef GCedHeapHashMap<Member<const ScopedTrigger>, Member<AnimationTrigger>>I think we prefer `using`?
Done
using ScopedTrigger = NamingScope;`TriggerScopedName`?
It's tricky, because these terms are overloaded in multiple ways:"Scope triggers" is already something you can add to an element in relation to something else (`@scope`): https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/resolver/scoped_style_resolver.cc;bpv=1;bpt=1;l=490?q=StyleResolver::AddImplicitScopeTrigger . Although it's not a widely-known concept, I'd like to avoid confusing these as much as possible.
"Scoped" already means "scoped as in TreeScope/ShadowDOM" in many cases. We've since gained other kinds of scoping: anchor-scope-scoping, trigger-scope-scoping. `TriggerScopedName` _hopefully_ indicates that this is scoped per `trigger-scope`, and not some other scoping mechanism.
Suggestion:
```
// A name scoped according to the 'trigger-scope' property.
//
// https://link-to-property-in-spec
using TriggerScopedName = NamingScope;
```And similarly for `AnchorScopedName`.
`TriggerScopedName` sounds good.
#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_DOM_TRIGGER_SCOPE_H_I think we should call this `trigger_scoped_name.h/cc`.
(`anchor_scope.h` should probably also have been `anchor_scoped_name.h/cc`)
Sounds good. Done. (Will send a separate refactor to rename anchor_scope.h/cc)
As mentioned elsewhere, I'd avoid "ScopedTrigger". I think we can probably just keep "NamedTrigger" for the function names, but now deal with objects of type `TriggerScopedName`s?
Sounds good. Done.
static const Element* FindScopeElement(
const ScopedCSSName& name,
const Element& start_element,
base::FunctionRef<StyleNameScope(const ComputedStyle& style)> get_scope);Let's explain a bit here:
```suggestion
// This function finds an element in the inclusive-ancestor chain
// of `start_element` that scopes `name` to that element via some property,
// e.g. 'anchor-scope'.
static const Element* FindScopeElement(
const ScopedCSSName& name,
const Element& start_element,
base::FunctionRef<StyleNameScope(const ComputedStyle& style)> get_scope);
```
Done
class CORE_EXPORT NamingScope : public GarbageCollected<NamingScope> {Especially now as a more general util, this could really use some documentation explaining what it's for.
Done
static bool IsWithinScope(const ScopedCSSName& lookup_name,
const StyleNameScope& scope);
static const Element* FindScopeElement(
const ScopedCSSName& name,
const Element& start_element,
base::FunctionRef<StyleNameScope(const ComputedStyle& style)> get_scope);I guess these ended up not being defined in `StyleNameScope`?
- Anders Hartvoll Ruud
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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 8 comments![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
8 comments
Member<Element> owning_element_;What retains `AnimationTrigger`? Could this lead to elements being held on to for too long?
(I don't know the answer, I just want to make sure you thought about it.)
ancestors_named_triggers->Set(entry.key, entry.value);So I guess it's possible for multiple elements in an ancestor chain (within the same trigger-scope) to define the same trigger name. Do we indeed want outer names to overwrite inner names?
I'd add a test similar to trigger-scope-scoped-tree-order.tentative.html, but with competing definitions in the ancestor chain.
element = element->parentElement();Not changed in this CL, but should this indeed be the parent in the light tree?
static const Element* FindScopeElement(
const ScopedCSSName& name,
const Element& start_element,
base::FunctionRef<StyleNameScope(const ComputedStyle& style)> get_scope);David AwogbemilaLet's explain a bit here:
```suggestion
// This function finds an element in the inclusive-ancestor chain
// of `start_element` that scopes `name` to that element via some property,
// e.g. 'anchor-scope'.
static const Element* FindScopeElement(
const ScopedCSSName& name,
const Element& start_element,
base::FunctionRef<StyleNameScope(const ComputedStyle& style)> get_scope);
```
Done
👍
class CORE_EXPORT NamingScope : public GarbageCollected<NamingScope> {David AwogbemilaEspecially now as a more general util, this could really use some documentation explaining what it's for.
Done
👍
animation-trigger: --trigger play-forwards play-backwards;Enlighten me: what is the effect of playing both forwards and backwards?
// The in-scope target should be attached to the trigger and paused atSo it's automatically paused on creation because we specified an `animation-trigger`?
// are yet to be paused by the trigger.- David Awogbemila
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. |
David Awogbemila (Gerrit)
David Awogbemila added 7 comments![]( "Open in Gerrit")
What retains `AnimationTrigger`? Could this lead to elements being held on to for too long?
(I don't know the answer, I just want to make sure you thought about it.)
Ah right, I think this should be a WeakMember. If the element goes away the trigger should also go away.
What retains `AnimationTrigger`?
In the case of TimelineTrigger the [timeline](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/animation_timeline.h;l=177?q=animation_timeline.h&ss=chromium%2Fchromium%2Fsrc). If the trigger has no animations, we [remove](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/timeline_trigger.cc;l=412?q=timelinetrigger::didremoveanimation&ss=chromium%2Fchromium%2Fsrc) it form the timeline's list.
In the case of an EventTrigger, the [EventListener](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/event_trigger.cc;l=28). If there are no animations attached, the listener is [removed](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/event_trigger.cc;l=82)
ancestors_named_triggers->Set(entry.key, entry.value);So I guess it's possible for multiple elements in an ancestor chain (within the same trigger-scope) to define the same trigger name. Do we indeed want outer names to overwrite inner names?
I'd add a test similar to trigger-scope-scoped-tree-order.tentative.html, but with competing definitions in the ancestor chain.
This won't cause outer names to overwrite inner names. In the fragment builder propagation code, the ancestor should see the descendant with the same name and do the last-in-tree-order comparison and pick the descendant.
I've tested that this works as expected (descendant is picked). Will add a competing ancestor chain test in follow-up patch (just so I can keep the current +1 and check this in asap :) ).
element = element->parentElement();Not changed in this CL, but should this indeed be the parent in the light tree?
Will look into this. I filed crbug.com/467317879 to track it.
using ScopedTrigger = NamingScope;`TriggerScopedName`?
It's tricky, because these terms are overloaded in multiple ways:"Scope triggers" is already something you can add to an element in relation to something else (`@scope`): https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/resolver/scoped_style_resolver.cc;bpv=1;bpt=1;l=490?q=StyleResolver::AddImplicitScopeTrigger . Although it's not a widely-known concept, I'd like to avoid confusing these as much as possible.
"Scoped" already means "scoped as in TreeScope/ShadowDOM" in many cases. We've since gained other kinds of scoping: anchor-scope-scoping, trigger-scope-scoping. `TriggerScopedName` _hopefully_ indicates that this is scoped per `trigger-scope`, and not some other scoping mechanism.
Suggestion:
```
// A name scoped according to the 'trigger-scope' property.
//
// https://link-to-property-in-spec
using TriggerScopedName = NamingScope;
```And similarly for `AnchorScopedName`.
`TriggerScopedName` sounds good.
Done
animation-trigger: --trigger play-forwards play-backwards;Enlighten me: what is the effect of playing both forwards and backwards?
This, combined with the timeline-trigger definition, is effectively saying "play forwards when scrolled into view, play backwards when scrolled out of view", not "play forwards and backwards."
Triggers work with a notion of activate/deactivate "events." The first value in `animation-trigger` specifies the action to take on "activate", and the second value is the action to take on "deactivate."
// The in-scope target should be attached to the trigger and paused atSo it's automatically paused on creation because we specified an `animation-trigger`?
Yes, this will be stated more plainly when the web animations spec gets updated. It was one of the outcomes of the discussion in [#12119](https://github.com/w3c/csswg-drafts/issues/12119#issuecomment-2922467450) about how to keep an animation that is waiting for its trigger condition displaying its first keyframe (fill-mode-dependent).
This doesn't make sense given the text that comes before it.
Right, left over from prior iterations. Updated.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[animation-trigger] Implement trigger-scope
- M third_party/blink/renderer/core/animation/animation_trigger.cc
- M third_party/blink/renderer/core/animation/animation_trigger.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/animation/document_animations.cc
- M third_party/blink/renderer/core/animation/timeline_trigger.cc
- M third_party/blink/renderer/core/animation/timeline_trigger.h
- M third_party/blink/renderer/core/css/css_trigger_attachment_value.cc
- M 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/dom/build.gni
- A third_party/blink/renderer/core/dom/trigger_scoped_name.cc
- A third_party/blink/renderer/core/dom/trigger_scoped_name.h
- M third_party/blink/renderer/core/layout/anchor_scope.cc
- M third_party/blink/renderer/core/layout/anchor_scope.h
- M third_party/blink/renderer/core/layout/build.gni
- M third_party/blink/renderer/core/layout/fragment_builder.cc
- M third_party/blink/renderer/core/layout/fragment_builder.h
- A third_party/blink/renderer/core/layout/naming_scope.cc
- A third_party/blink/renderer/core/layout/naming_scope.h
- M third_party/blink/renderer/core/layout/physical_fragment.h
- A third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/support/trigger-scope-support.js
- A third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/trigger-scope-in-scope-source.tentative.html
- A third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/trigger-scope-named.tentative.html
- A third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/trigger-scope-oof.tentative.html
- A third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/trigger-scope-out-of-scope-source.tentative.html
- A third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/trigger-scope-scoped-tree-order.tentative.html
- A third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/trigger-scope-unscoped-tree-order.tentative.html
Code-Review: +1 by Anders Hartvoll Ruud, +1 by David Awogbemila
| 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/56601
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |