Bug 185299

Summary:

REGRESSION (r230574): Interrupted hardware transitions don't behave correctly

Product:

WebKit

Reporter:

Antoine Quint <graouts>

Component:

Animations

Assignee:

Antoine Quint <graouts>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, dino, ews-watchlist, realdawei, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari Technology Preview

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

185668

Bug Blocks:

Attachments:

Description	Flags
Patch for EWS, not for review.	none
Patch	none
Patch	simon.fraser: review+

Antoine Quint

Reported 2018-05-04 06:55:20 PDT

The fix for https://bugs.webkit.org/show_bug.cgi?id=184518 broke interrupted transitions and suspended animations when moving to a different tab.

Attachments
Patch for EWS, not for review. (7.10 KB, patch) 2018-05-04 07:01 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Patch (7.22 KB, patch) 2018-05-04 12:16 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Patch (10.38 KB, patch) 2018-05-14 04:41 PDT, Antoine Quint	simon.fraser: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Antoine Quint

Comment 1 2018-05-04 06:56:10 PDT

<rdar://problem/39630230>

Antoine Quint

Comment 2 2018-05-04 07:01:28 PDT

Created attachment 339541 [details] Patch for EWS, not for review.

Simon Fraser (smfr)

Comment 3 2018-05-04 08:24:01 PDT

Comment on attachment 339541 [details] Patch for EWS, not for review. View in context: https://bugs.webkit.org/attachment.cgi?id=339541&action=review > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3118 > + if (m_animationsToProcess.contains(animation.m_name)) > + m_animationsToProcess.remove(animation.m_name); You have two hash lookups when you could use only one. Either just unconditionally remove, or find and remove using the iterator.

Antoine Quint

Comment 4 2018-05-04 09:31:47 PDT

(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 339541 [details] > Patch for EWS, not for review. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339541&action=review > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3118 > > + if (m_animationsToProcess.contains(animation.m_name)) > > + m_animationsToProcess.remove(animation.m_name); > > You have two hash lookups when you could use only one. Either just > unconditionally remove, or find and remove using the iterator. Oh right, remove returns a bool, so I guess it works whether there is a key or not. Can you think of a way to test this? I think the only way to check whether an animation is going is if we can get at the play state of CA animations. Is this possible?

Antoine Quint

Comment 5 2018-05-04 12:16:06 PDT

Created attachment 339582 [details] Patch

Simon Fraser (smfr)

Comment 6 2018-05-04 13:04:30 PDT

I think you could test this via a long-running transition with a negative start delay; hover and uncover (or changes styles) and then ref-test that the box is not in its untransformed location.

Antoine Quint

Comment 7 2018-05-14 04:41:21 PDT

Created attachment 340306 [details] Patch

Antoine Quint

Comment 8 2018-05-14 11:16:07 PDT

Committed r231765: <https://trac.webkit.org/changeset/231765>

Dawei Fenton (:realdawei)

Comment 9 2018-05-15 16:34:58 PDT

The test for this issue (transitions/interrupted-transition-hardware.html is flaky on the bots: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=transitions%2Finterrupted-transition-hardware.html

WebKit Commit Bot

Comment 10 2018-05-15 16:56:59 PDT

Re-opened since this is blocked by bug 185668