Bug 218792 – Support animations on more pseudo-elements

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

[Antoine Quint]

Support animations on more pseudo-elements

[Radar WebKit Bug Importer]

<rdar://problem/71274485>

[Antoine Quint]

Created attachment 413798 [details]
Patch

[Antoine Quint]

<rdar://problem/71274485>

[Antoine Quint]

Created attachment 413804 [details]
Patch

[Antoine Quint]

Created attachment 413942 [details]
Patch

[Antoine Quint]

To whomever ends up reviewing this: this patch makes http://wpt.live/css/css-pseudo/marker-font-variant-numeric-default.html regress since relying on ::before::marker and ::after::marker in the user-agent stylesheet seems to no longer apply `font-variant-numeric: tabular-nums` whereas it would when that property was hard-coded in RenderListItem. However, not making that change would cause some other WPT failures with `font-variant-numeric` not being able to be set to `reset` for `::marker` since it would not have a user-agent-defined value to revert to and it would use `normal`.

I think we mark this failure as expected and file a dedicated bug to deal with ::before::marker and ::after:marker.

[Antoine Quint]

Created attachment 414007 [details]
Patch

[Antoine Quint]

Created attachment 414041 [details]
Patch

[Antti Koivisto]

Comment on attachment 414041 [details]
Patch

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

> Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:302
> -void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::ElementUpdate& update)
> +void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::ElementUpdates* updates)
>  {
> +    ASSERT(updates);

Why is this now taking a pointer if it can't be null? Please switch back to a reference.

> Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:110
> +void RenderTreeUpdater::GeneratedContent::updatePseudoElement(Element& current, const Style::ElementUpdates* updates, PseudoId pseudoId)

Why is this taking a ElementUpdates pointer? The call sites null test so it should be a reference.

> Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:117
> +    auto* update = [updates, pseudoId]() -> const Style::ElementUpdate* {

could just use [&]

> Source/WebCore/style/StyleTreeResolver.cpp:251
> -    auto beforeUpdate = resolvePseudoStyle(element, update, PseudoId::Before);
> -    auto afterUpdate = resolvePseudoStyle(element, update, PseudoId::After);
> +    PseudoIdToElementUpdateMap pseudoUpdates;
> +    for (PseudoId pseudoId = PseudoId::FirstPublicPseudoId; pseudoId < PseudoId::FirstInternalPseudoId; pseudoId = static_cast<PseudoId>(static_cast<unsigned>(pseudoId) + 1)) {
> +        if (auto elementUpdate = resolvePseudoStyle(element, update, pseudoId))
> +            pseudoUpdates.set(pseudoId, WTFMove(*elementUpdate));
> +    }

At least::marker (anything else?) matches every element (since it is in the UA sheet) but only those with display:list-item care about it. It seems wasteful (and potential a perf regression) to resolve it eagerly in all cases. Maybe there should be a filter here that tests if a given pseudo element is relevant before resolving?

> Source/WebCore/style/StyleTreeResolver.cpp:269
> -        return { };
> +        return WTF::nullopt;

{ } would have still worked in these

[Antoine Quint]

(In reply to Antti Koivisto from comment #9)
> Comment on attachment 414041 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414041&action=review
> 
> > Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:302
> > -void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::ElementUpdate& update)
> > +void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::ElementUpdates* updates)
> >  {
> > +    ASSERT(updates);
> 
> Why is this now taking a pointer if it can't be null? Please switch back to
> a reference.

Will fix in new patch.

> > Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:110
> > +void RenderTreeUpdater::GeneratedContent::updatePseudoElement(Element& current, const Style::ElementUpdates* updates, PseudoId pseudoId)
> 
> Why is this taking a ElementUpdates pointer? The call sites null test so it
> should be a reference.

Will fix in new patch.

> > Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:117
> > +    auto* update = [updates, pseudoId]() -> const Style::ElementUpdate* {
> 
> could just use [&]

Indeed, will fix in new patch.

> > Source/WebCore/style/StyleTreeResolver.cpp:251
> > -    auto beforeUpdate = resolvePseudoStyle(element, update, PseudoId::Before);
> > -    auto afterUpdate = resolvePseudoStyle(element, update, PseudoId::After);
> > +    PseudoIdToElementUpdateMap pseudoUpdates;
> > +    for (PseudoId pseudoId = PseudoId::FirstPublicPseudoId; pseudoId < PseudoId::FirstInternalPseudoId; pseudoId = static_cast<PseudoId>(static_cast<unsigned>(pseudoId) + 1)) {
> > +        if (auto elementUpdate = resolvePseudoStyle(element, update, pseudoId))
> > +            pseudoUpdates.set(pseudoId, WTFMove(*elementUpdate));
> > +    }
> 
> At least::marker (anything else?) matches every element (since it is in the
> UA sheet) but only those with display:list-item care about it. It seems
> wasteful (and potential a perf regression) to resolve it eagerly in all
> cases. Maybe there should be a filter here that tests if a given pseudo
> element is relevant before resolving?

resolvePseudoStyle() bails if !elementUpdate.style->hasPseudoStyle(pseudoId) but we can move this up to the call site.

> > Source/WebCore/style/StyleTreeResolver.cpp:269
> > -        return { };
> > +        return WTF::nullopt;
> 
> { } would have still worked in these

Oh, cool! Will move back to { } to make a smaller diff.

[Antoine Quint]

Created attachment 414046 [details]
Patch

[Antti Koivisto]

> resolvePseudoStyle() bails if !elementUpdate.style->hasPseudoStyle(pseudoId)
> but we can move this up to the call site.

That won't help since hasPseudoStyle(marker) is true for every element.

[Antoine Quint]

Created attachment 414049 [details]
Patch

[Antoine Quint]

(In reply to Antti Koivisto from comment #12)
> > resolvePseudoStyle() bails if !elementUpdate.style->hasPseudoStyle(pseudoId)
> > but we can move this up to the call site.
> 
> That won't help since hasPseudoStyle(marker) is true for every element.

Right! I added a check for `::marker` to check that `display: list-item` is set for the element.

[Antti Koivisto]

Comment on attachment 414049 [details]
Patch

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

> Source/WebCore/style/StyleTreeResolver.cpp:273
> +    if (pseudoId == PseudoId::Marker && elementUpdate.style->display() != DisplayType::ListItem)
> +        return { };
>      if (elementUpdate.style->display() == DisplayType::None)
>          return { };
>      if (!elementUpdate.style->hasPseudoStyle(pseudoId))

this stuff could be factored into a function (shouldResolvePseudoElementStyle or similar).

[Antti Koivisto]

Comment on attachment 414049 [details]
Patch

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

> Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:117
> +    auto* update = [&]() -> const Style::ElementUpdate* {

This can probably be just [&] {

[Antoine Quint]

(In reply to Antti Koivisto from comment #16)
> Comment on attachment 414049 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414049&action=review
> 
> > Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:117
> > +    auto* update = [&]() -> const Style::ElementUpdate* {
> 
> This can probably be just [&] {

The compiler wants an explicit return type.

[Antoine Quint]

Created attachment 414070 [details]
Patch

[Antoine Quint]

Created attachment 414085 [details]
Patch

[Antoine Quint]

Committed r269813: <https://trac.webkit.org/changeset/269813>

[Antoine Quint]

This appears to have caused bug 220652.