Bug 257861 - REGRESSION (262875@main / iOS 17): fill: 'both' not respected with animation
Summary: REGRESSION (262875@main / iOS 17): fill: 'both' not respected with animation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari 17
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
: 258134 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-06-08 11:57 PDT by Liam DeBeasi
Modified: 2023-07-10 11:01 PDT (History)
5 users (show)

See Also:


Attachments
Code reproduction (1.67 KB, text/html)
2023-06-08 11:57 PDT, Liam DeBeasi
no flags Details
Non-accelerated demo (working as expected) (1.46 KB, text/html)
2023-06-12 05:43 PDT, Liam DeBeasi
no flags Details
Test reduction (418 bytes, text/html)
2023-06-14 11:23 PDT, zalan
no flags Details
Test reduction (495 bytes, text/html)
2023-06-14 11:26 PDT, zalan
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Liam DeBeasi 2023-06-08 11:57:35 PDT
Created attachment 466639 [details]
Code reproduction

Using fill: 'both' causes elements to disappear after their animation ends when using iOS 17 beta 1.

Steps to reproduce:

1. Open code reproduction on an iOS 17 device.
2. Tap "Open Overlay". Observe that the overlay opens.
3. Tap "Close Overlay". Observe that the overlay closes.
4. Tap "Open Overlay" again. Observe that the overlay opens and then disappears.

Expected Behavior:

I expect the overlay to remain visible every time I present it.

Actual Behavior:

The overlay disappears after subsequent presentations.

Other Information:

- This does not reproduce on Chrome 113 or Firefox 113.
- This does not reproduce on iOS 16. This appears to be a regression on iOS 17.
Comment 1 Radar WebKit Bug Importer 2023-06-09 19:03:40 PDT
<rdar://problem/110559121>
Comment 2 Antoine Quint 2023-06-11 12:05:27 PDT
I'll look into this. In the meantime, do you know whether this only apply to opacity or transform, which are accelerated properties, or also other properties such as width/height/top/left which are not accelerated?
Comment 3 Antoine Quint 2023-06-12 03:29:56 PDT
This regressed with 262875@main, the fix for bug 255338.
Comment 4 Liam DeBeasi 2023-06-12 05:43:10 PDT
Created attachment 466664 [details]
Non-accelerated demo (working as expected)

This behavior does not reproduce with non-accelerated properties. I attached a reproduction that shows the expected behavior when animating height.
Comment 5 zalan 2023-06-14 11:23:57 PDT
Created attachment 466694 [details]
Test reduction
Comment 6 zalan 2023-06-14 11:26:18 PDT
Created attachment 466695 [details]
Test reduction
Comment 7 Antoine Quint 2023-06-15 06:56:34 PDT
Not clear to me what is going wrong just yet, as far as I can tell at the GraphicsLayerCA level we are left with a single animation which is the last added animation in the test reduction – which is correct since its composite order is higher and the first animation is "replaced" per https://drafts.csswg.org/web-animations-1/#removing-replaced-animations. So I don't yet understand what the presentation value is the final value of the first animation.
Comment 8 Antoine Quint 2023-06-15 07:14:33 PDT
Actually, we end up calling GraphicsLayerCA::removeAnimation() with the last animation and not the former, so the final stack for GraphicsLayerCA is incorrect. So the presentation value makes sense, it's the content of the GraphicsLayerCA animation stack which is incorrect.
Comment 9 zalan 2023-06-15 14:12:30 PDT
<rdar://109221339>
Comment 10 Antoine Quint 2023-06-16 01:33:00 PDT
The accelerated actions we issue are correct, the first animation is played and stopped when it stops producing an interpolating value, at which points the second animation is played and stopped when itself stops producing an interpolating value.

Somehow, we fail to remove the first animation, or add it back, it's unclear yet.
Comment 11 Antoine Quint 2023-06-16 01:38:37 PDT
Oh, I think I see what's wrong, in KeyframeEffect::applyPendingAcceleratedActionsOrUpdateTimingProperties() we append AcceleratedAction::UpdateProperties but fail to record it as the last recorded action. This might just be it, there's a state disconnect here.
Comment 12 zalan 2023-06-23 10:34:48 PDT
*** Bug 258134 has been marked as a duplicate of this bug. ***
Comment 13 Antoine Quint 2023-06-24 06:08:23 PDT
Pull request: https://github.com/WebKit/WebKit/pull/15275
Comment 14 Antoine Quint 2023-06-24 06:18:40 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/40736
Comment 15 EWS 2023-06-24 11:05:40 PDT
Committed 265498@main (c35a74273103): <https://commits.webkit.org/265498@main>

Reviewed commits have been landed. Closing PR #15275 and removing active labels.
Comment 16 zalan 2023-06-27 09:12:50 PDT
While the fix addresses https://bug-257861-attachments.webkit.org/attachment.cgi?id=466695 it does not fix the original test case (and the duped bugzilla fails too)
Comment 17 zalan 2023-06-27 12:51:59 PDT
Let's use <rdar://111407099> to track this issue.
Comment 18 Antoine Quint 2023-06-28 03:45:22 PDT
Sorry for not being as thorough as I should have been. I'll take a look at the Code reproduction listed here.

Should we also de-dupe bug 258134 until we know for a fact that the complete fix for this bug actually addresses that bug as well?
Comment 19 Antoine Quint 2023-07-01 01:29:22 PDT
I have a good understand of the issue here, we'll need to send down the relative sorting order of accelerated effects to GraphicsLayerCA which should be easy enough. I'm hoping to get this done in the coming week (starting July 3).
Comment 20 Antoine Quint 2023-07-10 07:21:17 PDT
Pull request: https://github.com/WebKit/WebKit/pull/15693
Comment 21 Antoine Quint 2023-07-10 07:22:25 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/40947
Comment 22 EWS 2023-07-10 10:33:01 PDT
Committed 265909@main (6f928cc543c3): <https://commits.webkit.org/265909@main>

Reviewed commits have been landed. Closing PR #15693 and removing active labels.
Comment 23 Antoine Quint 2023-07-10 10:41:31 PDT
@Liam: does that regression break a website or application?
Comment 24 Liam DeBeasi 2023-07-10 11:01:23 PDT
This breaks Ionic Framework's sheet modal component: https://docs-demo.ionic.io/component/modal

Open on an iOS 17 device and tap "Open Sheet Modal". The animation should break after you try to swipe.

Thanks for the fix! I'll give this a try when it lands in an upcoming beta.