Bug 207290 – [Web Animations] Add support for `pseudoElement` on `KeyframeEffect` and `KeyframeEffectOptions`

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

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.

[Radar WebKit Bug Importer]

<rdar://problem/59199003>

[Antoine Quint]

Created attachment 396007 [details]
Patch

[Antoine Quint]

Created attachment 396054 [details]
Patch

[Antti Koivisto]

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.

[Antoine Quint]

Created attachment 396078 [details]
Patch

[Antoine Quint]

Created attachment 396097 [details]
Patch

[Sam Weinig]

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?

[Antoine Quint]

(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().

[Antti Koivisto]

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).

[Antoine Quint]

(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.

[Antoine Quint]

Created attachment 396524 [details]
Patch

[Antti Koivisto]

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?

[Antoine Quint]

Created attachment 396527 [details]
Patch

[Antoine Quint]

Created attachment 396535 [details]
Patch

[Antoine Quint]

Committed r260139: <https://trac.webkit.org/changeset/260139>