Bug 183781 - [Web Animations] Dispatch DOM events for CSS Transitions and CSS Animations implemented as Web Animations
Summary: [Web Animations] Dispatch DOM events for CSS Transitions and CSS Animations i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-20 09:44 PDT by Antoine Quint
Modified: 2018-03-21 12:42 PDT (History)
9 users (show)

See Also:


Attachments
Patch (90.54 KB, patch)
2018-03-20 11:02 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (27.31 KB, patch)
2018-03-21 11:05 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (29.00 KB, patch)
2018-03-21 12:00 PDT, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-03-20 09:44:01 PDT
Now that we've implemented CSS Animations and CSS Transitions as Web Animations (webkit.org/b/183504) we need to dispatch DOM events for them as well, as specified by https://drafts.csswg.org/css-animations-2/#events and https://drafts.csswg.org/css-transitions-2/#transition-events.
Comment 1 Antoine Quint 2018-03-20 11:02:09 PDT
Created attachment 336135 [details]
Patch
Comment 2 Dean Jackson 2018-03-20 11:34:07 PDT
Comment on attachment 336135 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336135&action=review

This is all good. It just needs to be split into smaller patches because it is doing multiple things.

> Source/WebCore/animation/AnimationTimeline.cpp:148
> +    // In case this element is newly getting a "display: none" we need to cancel all of its animations and disregard new ones.
> +    if (oldStyle && oldStyle->hasAnimations() && oldStyle->display() != NONE && newStyle.display() == NONE) {
> +        if (m_elementToCSSAnimationByName.contains(&element)) {
> +            for (const auto& cssAnimationsByNameMapItem : m_elementToCSSAnimationByName.take(&element))
> +                cancelOrRemoveDeclarativeAnimation(cssAnimationsByNameMapItem.value);
> +        }
> +        return;
> +    }
> +
> +    if (oldStyle && oldStyle->hasAnimations() && newStyle.hasAnimations() && *(oldStyle->animations()) == *(newStyle.animations()))
> +        return;

This display:none work should be a separate patch.

> Source/WebCore/animation/DeclarativeAnimation.cpp:72
> +    else
> +        pause();

So should this.

> Source/WebCore/animation/DeclarativeAnimation.cpp:106
> +void DeclarativeAnimation::cancel()
> +{
> +    auto cancelationTime = 0_s;
> +    if (auto animationEffect = effect())
> +        cancelationTime = animationEffect->activeTime().value_or(0_s);
> +
> +    WebAnimation::cancel();
> +
> +    invalidateDOMEvents(cancelationTime);
> +}

And maybe cancel() on declarative animations should be a separate patch too.
Comment 3 Antoine Quint 2018-03-21 07:48:47 PDT
I will land this as smaller patches.
Comment 4 Antoine Quint 2018-03-21 11:01:06 PDT
Actually, we can reuse this bug.
Comment 5 Antoine Quint 2018-03-21 11:05:46 PDT
Created attachment 336211 [details]
Patch
Comment 6 Antoine Quint 2018-03-21 12:00:48 PDT
Created attachment 336216 [details]
Patch
Comment 7 Antoine Quint 2018-03-21 12:41:33 PDT
Committed r229818: <https://trac.webkit.org/changeset/229818>
Comment 8 Radar WebKit Bug Importer 2018-03-21 12:42:28 PDT
<rdar://problem/38718784>