Bug 207290 - [Web Animations] Add support for `pseudoElement` on `KeyframeEffect` and `KeyframeEffectOptions`
Summary: [Web Animations] Add support for `pseudoElement` on `KeyframeEffect` and `Key...
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: 207291
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-05 13:01 PST by Antoine Quint
Modified: 2020-04-15 10:40 PDT (History)
18 users (show)

See Also:


Attachments
Patch (38.84 KB, patch)
2020-04-09 14:08 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (38.88 KB, patch)
2020-04-10 00:05 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (38.86 KB, patch)
2020-04-10 06:45 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (42.18 KB, patch)
2020-04-10 10:15 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (44.60 KB, patch)
2020-04-15 06:26 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (44.70 KB, patch)
2020-04-15 06:58 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (45.15 KB, patch)
2020-04-15 08:04 PDT, Antoine Quint
koivisto: 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 2020-02-05 13:01:08 PST
There is now a `pseudoElement` property on `KeyframeEffect` and `KeyframeEffectOptions` to indicate that an effect targets a pseudo-element, but also to allow setting one as an effect's target. We need to add support for this.
Comment 1 Radar WebKit Bug Importer 2020-02-05 13:01:23 PST
<rdar://problem/59199003>
Comment 2 Antoine Quint 2020-04-09 14:08:20 PDT
Created attachment 396007 [details]
Patch
Comment 3 Antoine Quint 2020-04-10 00:05:21 PDT
Created attachment 396054 [details]
Patch
Comment 4 Antti Koivisto 2020-04-10 06:19:13 PDT
Comment on attachment 396054 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffect.cpp:1182
> +    if (!m_pseudoId) {
> +        if (is<PseudoElement>(m_target.get()))
> +            setTarget(downcast<PseudoElement>(*m_target).hostElement());
> +    } else {
> +        if (is<PseudoElement>(m_target.get()))
> +            setTarget(ensurePseudoElement(downcast<PseudoElement>(*m_target).hostElement(), *m_pseudoId));
> +        else if (auto* target = m_target.get())
> +            setTarget(ensurePseudoElement(target, *m_pseudoId));
> +    }

You should implement this code in terms for Element/pseudoId pair. m_target should always point to the element and the code shouldn't deal with PseudoElement at all. It certainly shouldn't construct any.
Comment 5 Antoine Quint 2020-04-10 06:45:44 PDT
Created attachment 396078 [details]
Patch
Comment 6 Antoine Quint 2020-04-10 10:15:54 PDT
Created attachment 396097 [details]
Patch
Comment 7 Sam Weinig 2020-04-11 16:27:52 PDT
Comment on attachment 396097 [details]
Patch

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

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:404
> +ENABLE_WEB_ANIMATIONS = ENABLE_WEB_ANIMATIONS;

Why is this change needed?
Comment 8 Antoine Quint 2020-04-12 01:51:43 PDT
(In reply to Sam Weinig from comment #7)
> Comment on attachment 396097 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396097&action=review
> 
> > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:404
> > +ENABLE_WEB_ANIMATIONS = ENABLE_WEB_ANIMATIONS;
> 
> Why is this change needed?

It's not and it wouldn't go into a possible commit!

This was just a hack to force EWS to rebuild all of WebCore since otherwise an incremental build would fail new test expectations, likely due to the changes in the IDL files. Conversion would yield a null `keyframeEffectOptions.pseudoElement` in KeyframeEffect::create().
Comment 9 Antti Koivisto 2020-04-14 04:28:11 PDT
Comment on attachment 396097 [details]
Patch

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

I think this patch is heading to a wrong direction with expanded use of PseudoElement. I feel the animation code should be refactored to minimise its use first. Nevertheless I'd r+ this with some cleanups since it is not very large and clearly adds useful functionality.

> Source/WebCore/animation/KeyframeEffect.cpp:1104
> +static Element* ensurePseudoElement(Element* host, PseudoId pseudoId)
> +{
> +    ASSERT(host);
> +
> +    if (pseudoId == PseudoId::Before) {
> +        if (auto* beforePseudoElement = host->beforePseudoElement())
> +            return beforePseudoElement;
> +        auto newBeforePseudoElement = PseudoElement::create(*host, pseudoId);
> +        host->setBeforePseudoElement(WTFMove(newBeforePseudoElement));
> +        return host->beforePseudoElement();
> +    }
> +
> +    if (pseudoId == PseudoId::After) {
> +        if (auto* afterPseudoElement = host->afterPseudoElement())
> +            return afterPseudoElement;
> +        auto newAfterPseudoElement = PseudoElement::create(*host, pseudoId);
> +        host->setAfterPseudoElement(WTFMove(newAfterPseudoElement));
> +        return host->afterPseudoElement();
> +    }
> +
> +    ASSERT_NOT_REACHED();
> +    return nullptr;

This is the 3rd instance of this code. At least it should go to Element where it can be shared.

> Source/WebCore/animation/KeyframeEffect.cpp:1113
> +void KeyframeEffect::setBindingsTarget(RefPtr<Element>&& target)
> +{
> +    if (m_pseudoId && target)
> +        setTarget(ensurePseudoElement(target.get(), *m_pseudoId));
> +    else
> +        setTarget(WTFMove(target));
> +}

So nasty.

It seems to me that the right factoring is to save the target Element (not PseudoElement) and m_pseudoId, then have a helper that returns either Element or PseudoElement based on those. Current m_target users would switch to that.

> Source/WebCore/animation/KeyframeEffect.cpp:1161
> +    // If the provided value is not null or a syntactically valid the user agent must throw a DOMException with error name TypeError and leave the
> +    // target pseudo-selector of this animation effect unchanged.

The code below throws on syntactically valid but unhandled selectors. That's not what this text says.

> Source/WebCore/animation/KeyframeEffect.cpp:1172
> +    if (!pseudoElement)
> +        m_pseudoId = WTF::nullopt;
> +    else if (pseudoElement == "::before" || pseudoElement == ":before")
> +        m_pseudoId = PseudoId::Before;
> +    else if (pseudoElement == "::after" || pseudoElement == ":after")
> +        m_pseudoId = PseudoId::After;
> +    else
> +        return Exception { TypeError };

Why is this done manually instead of using the parsing facilities?

> Source/WebCore/animation/KeyframeEffect.cpp:1182
> +    if (!m_pseudoId) {
> +        if (is<PseudoElement>(m_target.get()))
> +            setTarget(downcast<PseudoElement>(*m_target).hostElement());
> +    } else {
> +        if (is<PseudoElement>(m_target.get()))
> +            setTarget(ensurePseudoElement(downcast<PseudoElement>(*m_target).hostElement(), *m_pseudoId));
> +        else if (auto* target = m_target.get())
> +            setTarget(ensurePseudoElement(target, *m_pseudoId));
> +    }

:(

> Source/WebCore/animation/KeyframeEffect.cpp:1759
> +bool KeyframeEffect::requiresPseudoElement() const
> +{
> +    return m_blendingKeyframesSource == BlendingKeyframesSource::WebAnimation && is<PseudoElement>(m_target.get());
> +}

Only one of the m_pseudoId and is<PseudoElement>(m_target) should be authorative and you should stick to that (m_pseudoId in practice as we want to get rid of PseudoElement).
Comment 10 Antoine Quint 2020-04-14 06:45:17 PDT
(In reply to Antti Koivisto from comment #9)
> > Source/WebCore/animation/KeyframeEffect.cpp:1104
> > +static Element* ensurePseudoElement(Element* host, PseudoId pseudoId)
> > +{
> > +    ASSERT(host);
> > +
> > +    if (pseudoId == PseudoId::Before) {
> > +        if (auto* beforePseudoElement = host->beforePseudoElement())
> > +            return beforePseudoElement;
> > +        auto newBeforePseudoElement = PseudoElement::create(*host, pseudoId);
> > +        host->setBeforePseudoElement(WTFMove(newBeforePseudoElement));
> > +        return host->beforePseudoElement();
> > +    }
> > +
> > +    if (pseudoId == PseudoId::After) {
> > +        if (auto* afterPseudoElement = host->afterPseudoElement())
> > +            return afterPseudoElement;
> > +        auto newAfterPseudoElement = PseudoElement::create(*host, pseudoId);
> > +        host->setAfterPseudoElement(WTFMove(newAfterPseudoElement));
> > +        return host->afterPseudoElement();
> > +    }
> > +
> > +    ASSERT_NOT_REACHED();
> > +    return nullptr;
> 
> This is the 3rd instance of this code. At least it should go to Element
> where it can be shared.

Will do, probably as part of a dedicated patch.

> > Source/WebCore/animation/KeyframeEffect.cpp:1113
> > +void KeyframeEffect::setBindingsTarget(RefPtr<Element>&& target)
> > +{
> > +    if (m_pseudoId && target)
> > +        setTarget(ensurePseudoElement(target.get(), *m_pseudoId));
> > +    else
> > +        setTarget(WTFMove(target));
> > +}
> 
> So nasty.
> 
> It seems to me that the right factoring is to save the target Element (not
> PseudoElement) and m_pseudoId, then have a helper that returns either
> Element or PseudoElement based on those. Current m_target users would switch
> to that.

Agreed, this is being done in bug 210491.

> > Source/WebCore/animation/KeyframeEffect.cpp:1161
> > +    // If the provided value is not null or a syntactically valid the user agent must throw a DOMException with error name TypeError and leave the
> > +    // target pseudo-selector of this animation effect unchanged.
> 
> The code below throws on syntactically valid but unhandled selectors. That's
> not what this text says.

Will double check this!

> > Source/WebCore/animation/KeyframeEffect.cpp:1172
> > +    if (!pseudoElement)
> > +        m_pseudoId = WTF::nullopt;
> > +    else if (pseudoElement == "::before" || pseudoElement == ":before")
> > +        m_pseudoId = PseudoId::Before;
> > +    else if (pseudoElement == "::after" || pseudoElement == ":after")
> > +        m_pseudoId = PseudoId::After;
> > +    else
> > +        return Exception { TypeError };
> 
> Why is this done manually instead of using the parsing facilities?

For no good reason, will look into using existing code.

> > Source/WebCore/animation/KeyframeEffect.cpp:1759
> > +bool KeyframeEffect::requiresPseudoElement() const
> > +{
> > +    return m_blendingKeyframesSource == BlendingKeyframesSource::WebAnimation && is<PseudoElement>(m_target.get());
> > +}
> 
> Only one of the m_pseudoId and is<PseudoElement>(m_target) should be
> authorative and you should stick to that (m_pseudoId in practice as we want
> to get rid of PseudoElement).

This will be clear after bug 210491 lands.
Comment 11 Antoine Quint 2020-04-15 06:26:39 PDT
Created attachment 396524 [details]
Patch
Comment 12 Antti Koivisto 2020-04-15 06:52:19 PDT
Comment on attachment 396524 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffect.cpp:1139
> +        bool isLegacy = pseudoElement == ":before" || pseudoElement == ":after" || pseudoElement == ":first-letter" || pseudoElement == ":first-line";
> +        auto pseudoType = CSSSelector::parsePseudoElementType(pseudoElement.substring(isLegacy ? 1 : 2));
> +        if (pseudoType == CSSSelector::PseudoElementUnknown)

This parses xxbefore as a valid pseudo element.

> Source/WebCore/animation/KeyframeEffect.cpp:1154
> +void KeyframeEffect::invalidateTargetElementOrPseudoElement(Element* previousTargetElementOrPseudoElement)

Would this be more like didChangeTargetElementOrPseudoElement?
Comment 13 Antoine Quint 2020-04-15 06:58:35 PDT
Created attachment 396527 [details]
Patch
Comment 14 Antoine Quint 2020-04-15 08:04:38 PDT
Created attachment 396535 [details]
Patch
Comment 15 Antoine Quint 2020-04-15 10:40:07 PDT
Committed r260139: <https://trac.webkit.org/changeset/260139>