Dialog element only animates once

[Darin Senneff]

Created attachment 451176 [details]
Screen capture of dialog animation bug

When using a CSS animation to display a <dialog> element, the animation plays correctly the first time the <dialog> is displayed but not subsequent times.

Noticed this on the Webkit blog post about the <dialog> element (https://webkit.org/blog/12209/introducing-the-dialog-element/). The demo that is animated is embedded under the "Styling" heading. 

I also built my own demo to confirm and am experiencing the issue there, too. Also attaching a screen capture video for reference.


To replicate:

1) Open the following demo in Safari Tech Preview:
https://codepen.io/dsenneff/pen/qBVRPNr/ed7774c741125ea6f486e4da13d65cf0?editors=1111

2) Click on the 'Open Dialog' button. The <dialog> transitions into view.

3) Close the <dialog>.

4) Click on the 'Open Dialog' button again. The <dialog> does not transition.

[Radar WebKit Bug Importer]

<rdar://problem/88635487>

[Antoine Quint]

If I interrupt the animation by clicking on Cancel or Confirm while the dialog is animating, then I can re-run the animation just fine.

[Antoine Quint]

Created attachment 451379 [details]
Patch

[Antoine Quint]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/32773

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Tim Nguyen (:ntim)]

Comment on attachment 451379 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html:12
> +dialog[open] {
> +    animation: dialog-open-animation 1ms;
> +}

Maybe `dialog` is better selector to target since it always matches. not sure?

> LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html:20
> +<script type="text/javascript">

nit: drop type="text/javascript"

> LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html:22
> +'use strict';

nit: double quotes for consistency with the other strings in the test

> LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-animation.html:41
> +  dialog.open = true;
> +  const animations = dialog.getAnimations();
> +  assert_equals(animations.length, 1, "As the <dialog> is shown intially an animation is started");
> +
> +  await animations[0].finished;
> +
> +  await waitForNextFrame();
> +
> +  dialog.open = false;
> +  assert_equals(dialog.getAnimations().length, 0, "As the <dialog> is closed the animation is removed");
> +
> +  await waitForNextFrame();
> +
> +  dialog.open = true;

dialog.show()/dialog.close() is usually preferable, but I guess it doesn't matter for this test

> LayoutTests/imported/w3c/web-platform-tests/css/css-animations/dialog-backdrop-animation.html:20
> +<script type="text/javascript">

ditto

[Tim Nguyen (:ntim)]

Comment on attachment 451379 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:3451
> +    if (auto* renderer = this->renderer()) {
> +        if (auto backdrop = renderer->backdropRenderer()) {
> +            if (auto styleable = Styleable::fromRenderer(*backdrop))
> +                styleable->cancelDeclarativeAnimations();
> +        }
> +    }
> +

Note for other reviewers: this is done here, because after backdrop renderers get destroyed after Element::removeFromTopLayer()

and once they get destroyed, Styleable::fromRenderer(backdropRenderer) can't find the associated element anymore, since fromRenderer uses Document::topLayerElements() to find element renderers associated to backdrops.

Another way to fix this bug would be making  Styleable::fromRenderer independent from topLayerElements(), but it's not entirely trivial to do atm.

[Dean Jackson]

Comment on attachment 451379 [details]
Patch

r=me based on Tim's review

[Simon Fraser (smfr)]

Comment on attachment 451379 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:3451
>> +
> 
> Note for other reviewers: this is done here, because after backdrop renderers get destroyed after Element::removeFromTopLayer()
> 
> and once they get destroyed, Styleable::fromRenderer(backdropRenderer) can't find the associated element anymore, since fromRenderer uses Document::topLayerElements() to find element renderers associated to backdrops.
> 
> Another way to fix this bug would be making  Styleable::fromRenderer independent from topLayerElements(), but it's not entirely trivial to do atm.

If you have to explain this to other reviewers, then maybe the code needs to explain it in the form of a comment.

[Antoine Quint]

Created attachment 451413 [details]
Patch for landing

[EWS]

Committed r289498 (247036@main): <https://commits.webkit.org/247036@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451413 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 236534

[Tim Nguyen (:ntim)]

Comment on attachment 451413 [details]
Patch for landing

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

> Source/WebCore/dom/Element.cpp:3446
> +    // We need to call Styleable::fromRenderer() while this document is still contained
> +    // in Document::topLayerElements().

This comment would be more accurate:

    // We need to call Styleable::fromRenderer() while this element is still contained in
    // Document::topLayerElements(), since Styleable::fromRenderer() relies on this to
    // find the backdrop's associated element.

[Antoine Quint]

Created attachment 454676 [details]
Patch

[Antoine Quint]

(In reply to Tim Nguyen (:ntim) from comment #13)
> Comment on attachment 451413 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451413&action=review
> 
> > Source/WebCore/dom/Element.cpp:3446
> > +    // We need to call Styleable::fromRenderer() while this document is still contained
> > +    // in Document::topLayerElements().
> 
> This comment would be more accurate:
> 
>     // We need to call Styleable::fromRenderer() while this element is still
> contained in
>     // Document::topLayerElements(), since Styleable::fromRenderer() relies
> on this to
>     // find the backdrop's associated element.

I used that comment in the new patch.

[Antoine Quint]

Created attachment 454685 [details]
Patch for landing

[EWS]

Committed r291282 (248421@main): <https://commits.webkit.org/248421@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454685 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 237924

[Antoine Quint]

I think I worked out the performance regression, the issue was that we'd added a call to `setAnimationsCreatedByMarkup({ })` under `Styleable::cancelDeclarativeAnimations()` and this would force the creation of ElementAnimationRareData for the wrapper element even if there were no pre-existing data stored for it. I will now upload a new patch which appears to address Speedometer performance concerns.

[Antoine Quint]

Created attachment 455117 [details]
Patch

[Antoine Quint]

Created attachment 455119 [details]
Patch for landing

[EWS]

Committed r291527 (248633@main): <https://commits.webkit.org/248633@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455119 [details].

[Antoine Quint]

This caused bug 239291.

[Brent Fulgham]

This fix shipped with Safari 15.5 (all platforms).