Bug 230008 – Accelerated animations on ::backdrop shouldn't affect <dialog> (backdrop-animate-002.html fails)

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

[Tim Nguyen (:ntim)]

CADebug -s layers


backdrop:

(name "CALayer(0x138610950) GraphicsLayer(0x1006b4f00, 21) RenderBlock (positioned) 0x1008d7960")
(opacity 0.101961)


dialog:

(values (array
                               (vector 0.1000000015)
                               (vector 0.1000000015)))


For some reason the opacity animation on ::backdrop affects the dialog by adding an opacity on the <dialog>. Also happens when using @keyframes.

[Radar WebKit Bug Importer]

<rdar://problem/82975766>

[Tim Nguyen (:ntim)]

hmm transform animations also affect the <dialog> itself

[Antoine Quint]

Created attachment 441256 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 441256 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Accelerated animations on ::backdrop shouldn't affect <dialog> (backdrop-animate-002.html fails)

Not sure this is the best title for this change.

> Source/WebCore/style/Styleable.cpp:55
> +    // FIXME: handle ::marker.

I think you can implement marker as:

```
if (renderer.style().styleType() == PseudoId::Marker) {
    ASSERT(renderer.parent()->markerRenderer() == &renderer);
    return Styleable(renderer.parent()->markerRenderer(), PseudoId::Marker);
}
```

::marker is a child in the render tree of RenderListItem: https://webkit-search.igalia.com/webkit/source/Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp#88

[Tim Nguyen (:ntim)]

Comment on attachment 441256 [details]
Patch

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

>> Source/WebCore/style/Styleable.cpp:55
>> +    // FIXME: handle ::marker.
> 
> I think you can implement marker as:
> 
> ```
> if (renderer.style().styleType() == PseudoId::Marker) {
>     ASSERT(renderer.parent()->markerRenderer() == &renderer);
>     return Styleable(renderer.parent()->markerRenderer(), PseudoId::Marker);
> }
> ```
> 
> ::marker is a child in the render tree of RenderListItem: https://webkit-search.igalia.com/webkit/source/Source/WebCore/rendering/updating/RenderTreeBuilderList.cpp#88

well:

return Styleable(renderer.parent()->element(), PseudoId::Marker);

I *think*

[Tim Nguyen (:ntim)]

Comment on attachment 441256 [details]
Patch

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

> Source/WebCore/style/Styleable.cpp:54
> +{

One thing that would be nice is to `ASSERT` of expected PseudoId types in this function.

So future bugs like this can be caught easily in the future, since the test would just crash and assert at the right place:

```
auto pseudoId = renderer.style().styleType();
ASSERT(pseudoId == PseudoId::None || pseudoId == PseudoId::Before || pseudoId == PseudoId::After || [...], "PseudoId should be handled by Styleable);
```

[Simon Fraser (smfr)]

Comment on attachment 441256 [details]
Patch

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

> Source/WebCore/style/Styleable.cpp:86
> +    if (pseudoId == PseudoId::After) {
> +        if (auto* afterPseudoElement = element.afterPseudoElement())
> +            return afterPseudoElement->renderer();
> +    } else if (pseudoId == PseudoId::Backdrop) {
> +        if (auto* hostRenderer = element.renderer())
> +            return hostRenderer->backdropRenderer().get();
> +    } else if (pseudoId == PseudoId::Before) {
> +        if (auto* beforePseudoElement = element.beforePseudoElement())
> +            return beforePseudoElement->renderer();
> +    } else if (pseudoId == PseudoId::Marker) {
> +        if (is<RenderListItem>(element.renderer()))
> +            return downcast<RenderListItem>(*element.renderer()).markerRenderer();
> +    } else if (pseudoId == PseudoId::None)
> +        return element.renderer();

Switch

[Tim Nguyen (:ntim)]

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 441256 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441256&action=review
> 
> > Source/WebCore/style/Styleable.cpp:86
> > +    if (pseudoId == PseudoId::After) {
> > +        if (auto* afterPseudoElement = element.afterPseudoElement())
> > +            return afterPseudoElement->renderer();
> > +    } else if (pseudoId == PseudoId::Backdrop) {
> > +        if (auto* hostRenderer = element.renderer())
> > +            return hostRenderer->backdropRenderer().get();
> > +    } else if (pseudoId == PseudoId::Before) {
> > +        if (auto* beforePseudoElement = element.beforePseudoElement())
> > +            return beforePseudoElement->renderer();
> > +    } else if (pseudoId == PseudoId::Marker) {
> > +        if (is<RenderListItem>(element.renderer()))
> > +            return downcast<RenderListItem>(*element.renderer()).markerRenderer();
> > +    } else if (pseudoId == PseudoId::None)
> > +        return element.renderer();
> 
> Switch

Use ASSERT_NOT_REACHED(); in unhandled cases too :)

[Antoine Quint]

Created attachment 441362 [details]
Patch for landing

[Antoine Quint]

(In reply to Tim Nguyen (:ntim) from comment #6)
> Comment on attachment 441256 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441256&action=review
> 
> > Source/WebCore/style/Styleable.cpp:54
> > +{
> 
> One thing that would be nice is to `ASSERT` of expected PseudoId types in
> this function.
> 
> So future bugs like this can be caught easily in the future, since the test
> would just crash and assert at the right place:
> 
> ```
> auto pseudoId = renderer.style().styleType();
> ASSERT(pseudoId == PseudoId::None || pseudoId == PseudoId::Before ||
> pseudoId == PseudoId::After || [...], "PseudoId should be handled by
> Styleable);
> ```

In the case of Styleable::renderer(), it currently makes sense to have an ASSERT_NOT_REACHED() call for unhandled pseudo-elements.

However, in the case of Styleable::fromRenderer() it doesn't, because that function is called under RenderLayerCompositor::requiresCompositingForAnimation() which may be called for any potentially-composited element, which can be the case for :first-letter for instance.

[Antoine Quint]

Created attachment 441374 [details]
Patch for landing

[Tim Nguyen (:ntim)]

(In reply to Antoine Quint from comment #10)
> However, in the case of Styleable::fromRenderer() it doesn't, because that
> function is called under
> RenderLayerCompositor::requiresCompositingForAnimation() which may be called
> for any potentially-composited element, which can be the case for
> :first-letter for instance.

I think we should at least warn, it's fishy to end up in an unhandled case.

[Antoine Quint]

Landed in r284313.