Bug 236274

Summary: Dialog element only animates once
Product: WebKit Reporter: Darin Senneff <darin.senneff>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, clopez, cmarcelo, commit-queue, dino, esprehn+autocc, ews-watchlist, graouts, graouts, kangil.han, koivisto, ntim, simon.fraser, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac (Intel)   
OS: macOS 12   
See Also: https://github.com/web-platform-tests/wpt/pull/32773
https://bugs.webkit.org/show_bug.cgi?id=239291
Bug Depends on: 236534, 237924    
Bug Blocks: 84635    
Attachments:
Description Flags
Screen capture of dialog animation bug
none
Patch
none
Patch for landing
none
Patch
none
Patch for landing
none
Patch
dino: review+, ews-feeder: commit-queue-
Patch for landing none

Description Darin Senneff 2022-02-07 17:10:10 PST
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.
Comment 1 Radar WebKit Bug Importer 2022-02-08 09:50:05 PST
<rdar://problem/88635487>
Comment 2 Antoine Quint 2022-02-08 11:15:30 PST
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.
Comment 3 Antoine Quint 2022-02-09 08:10:28 PST
Created attachment 451379 [details]
Patch
Comment 4 Antoine Quint 2022-02-09 08:12:19 PST
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/32773
Comment 5 EWS Watchlist 2022-02-09 08:13:13 PST
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
Comment 6 Tim Nguyen (:ntim) 2022-02-09 08:26:02 PST
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
Comment 7 Tim Nguyen (:ntim) 2022-02-09 08:30:22 PST
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.
Comment 8 Dean Jackson 2022-02-09 08:56:40 PST
Comment on attachment 451379 [details]
Patch

r=me based on Tim's review
Comment 9 Simon Fraser (smfr) 2022-02-09 09:22:02 PST
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.
Comment 10 Antoine Quint 2022-02-09 11:16:03 PST
Created attachment 451413 [details]
Patch for landing
Comment 11 EWS 2022-02-09 12:49:25 PST
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].
Comment 12 WebKit Commit Bot 2022-02-11 17:10:56 PST
Re-opened since this is blocked by bug 236534
Comment 13 Tim Nguyen (:ntim) 2022-02-12 11:21:55 PST
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.
Comment 14 Antoine Quint 2022-03-15 00:55:14 PDT
Created attachment 454676 [details]
Patch
Comment 15 Antoine Quint 2022-03-15 01:08:22 PDT
(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.
Comment 16 Antoine Quint 2022-03-15 03:06:28 PDT
Created attachment 454685 [details]
Patch for landing
Comment 17 EWS 2022-03-15 04:41:09 PDT
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].
Comment 18 WebKit Commit Bot 2022-03-15 15:06:19 PDT
Re-opened since this is blocked by bug 237924
Comment 19 Antoine Quint 2022-03-18 11:39:33 PDT
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.
Comment 20 Antoine Quint 2022-03-18 11:42:29 PDT
Created attachment 455117 [details]
Patch
Comment 21 Antoine Quint 2022-03-18 11:50:23 PDT
Created attachment 455119 [details]
Patch for landing
Comment 22 EWS 2022-03-19 06:25:06 PDT
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].
Comment 23 Antoine Quint 2022-04-13 12:36:23 PDT
This caused bug 239291.
Comment 24 Brent Fulgham 2022-05-26 14:50:00 PDT
This fix shipped with Safari 15.5 (all platforms).