Bug 230008

Summary: Accelerated animations on ::backdrop shouldn't affect <dialog> (backdrop-animate-002.html fails)
Product: WebKit Reporter: Tim Nguyen (:ntim) <ntim>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, graouts, graouts, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 231906, 232185    
Bug Blocks: 84635, 229042    
Attachments:
Description Flags
Patch
simon.fraser: review+
Patch for landing
ews-feeder: commit-queue-
Patch for landing ews-feeder: commit-queue-

Description Tim Nguyen (:ntim) 2021-09-07 09:56:32 PDT
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.
Comment 1 Radar WebKit Bug Importer 2021-09-10 09:29:31 PDT
<rdar://problem/82975766>
Comment 2 Tim Nguyen (:ntim) 2021-10-14 07:28:01 PDT
hmm transform animations also affect the <dialog> itself
Comment 3 Antoine Quint 2021-10-14 12:37:31 PDT
Created attachment 441256 [details]
Patch
Comment 4 Tim Nguyen (:ntim) 2021-10-14 13:00:22 PDT
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
Comment 5 Tim Nguyen (:ntim) 2021-10-14 13:02:31 PDT
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*
Comment 6 Tim Nguyen (:ntim) 2021-10-14 13:09:34 PDT
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);
```
Comment 7 Simon Fraser (smfr) 2021-10-14 13:14:59 PDT
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
Comment 8 Tim Nguyen (:ntim) 2021-10-14 13:16:35 PDT
(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 :)
Comment 9 Antoine Quint 2021-10-15 04:05:13 PDT
Created attachment 441362 [details]
Patch for landing
Comment 10 Antoine Quint 2021-10-15 06:47:25 PDT
(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.
Comment 11 Antoine Quint 2021-10-15 06:52:34 PDT
Created attachment 441374 [details]
Patch for landing
Comment 12 Tim Nguyen (:ntim) 2021-10-15 07:24:06 PDT
(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.
Comment 13 Antoine Quint 2021-10-16 03:25:27 PDT
Landed in r284313.