Title: [265985] trunk
- Revision
- 265985
- Author
- wenson_hs...@apple.com
- Date
- 2020-08-20 17:12:05 -0700 (Thu, 20 Aug 2020)
Log Message
REGRESSION (r263729): Carousel freezes on "fourth page"/fourth click on right arrow on netflix.com
https://bugs.webkit.org/show_bug.cgi?id=215655
<rdar://problem/65845979>
Reviewed by Dean Jackson.
Source/WebCore:
On netflix.com, when clicking on the left and right arrows in each movie or TV show carousel, the page attempts
to animate to the next page of the carousel using a CSS transform transition. The logic applies `transform` and
`transition` CSS properties to a container `div`, and adds a `transitionend` event listener which the page
expects to be invoked when the animation is complete. While waiting for this `transitionend` event, the script
also sets a boolean flag that prevents the carousel from being advanced to any other page. However, after the
changes in r263729, the carousel gets into a state where `transition` and `transform` styles are set, but the
animation never begins, and thus, no subsequent `transitionend` event is observed. This causes the page to
believe that the carousel is indefinitely animating, so it never unsets the boolean flag, which results in the
carousel being permanently stuck.
This occurs because we now have logic in `AnimationTimeline::updateCSSTransitionsForElementAndProperty` that
moves the `CSSTransition` from the element's map of running transitions to the map of completed transitions in
the case where the corresponding `WebAnimation` is already in `Finished` state. However, consider the case where
there is no matching backing animation (i.e. `matchingBackingAnimation` is `nullptr`); for instance, this can
happen if the transition CSS property is set to none in the middle of the `transitionend` event, as demonstrated
in the new layout test. Before the change, we would've removed the `CSSTransition` from the map of running
transitions and canceled it, but now, we instead move it to the map of completed transitions, where it remains
until the next CSS transition update is triggered (which would potentially be indefinitely long!).
On netflix.com, this next CSS transition update happens the page attempts to advance the carousel. Since the old
`CSSTransition` is still in the "completed" transitions map, we end up returning `true` when checking
`propertyInStyleMatchesValueForTransitionInMap`, and consequently never attempt to create a new `CSSTransition`
and add it to the map of running transitions in step 1 of the algorithm. As described above, this causes the
carousel to get stuck in a bad state.
To fix this, we simply revert to pre-r263729 behavior in the case where the matching backing animation was
already removed, and allow step 3 of the algorithm to cancel the running animation and remove it altogether
instead of moving it into the element's completed transitions map.
Test: animations/animation-followed-by-two-transitions.html
* animation/AnimationTimeline.cpp:
(WebCore::AnimationTimeline::updateCSSTransitionsForElementAndProperty):
LayoutTests:
Adds a layout test inspired by animation logic used in the broken carousel UI on netflix.com. This test can be
manually run by opening the test in a browser and verifying that the green square quickly slides across the
screen twice, and two `transitionend` events are observed in the process.
* animations/animation-followed-by-two-transitions-expected.txt: Added.
* animations/animation-followed-by-two-transitions.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (265984 => 265985)
--- trunk/LayoutTests/ChangeLog 2020-08-20 23:53:35 UTC (rev 265984)
+++ trunk/LayoutTests/ChangeLog 2020-08-21 00:12:05 UTC (rev 265985)
@@ -1,3 +1,18 @@
+2020-08-20 Wenson Hsieh <wenson_hs...@apple.com>
+
+ REGRESSION (r263729): Carousel freezes on "fourth page"/fourth click on right arrow on netflix.com
+ https://bugs.webkit.org/show_bug.cgi?id=215655
+ <rdar://problem/65845979>
+
+ Reviewed by Dean Jackson.
+
+ Adds a layout test inspired by animation logic used in the broken carousel UI on netflix.com. This test can be
+ manually run by opening the test in a browser and verifying that the green square quickly slides across the
+ screen twice, and two `transitionend` events are observed in the process.
+
+ * animations/animation-followed-by-two-transitions-expected.txt: Added.
+ * animations/animation-followed-by-two-transitions.html: Added.
+
2020-08-20 Kenneth Russell <k...@chromium.org>
[WebGL2] Pass context-lost-restored.html test
Added: trunk/LayoutTests/animations/animation-followed-by-two-transitions-expected.txt (0 => 265985)
--- trunk/LayoutTests/animations/animation-followed-by-two-transitions-expected.txt (rev 0)
+++ trunk/LayoutTests/animations/animation-followed-by-two-transitions-expected.txt 2020-08-21 00:12:05 UTC (rev 265985)
@@ -0,0 +1,12 @@
+The test passes if the animationend event is followed by two transitionend events.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Observed 'animationend' event. Starting first transition...
+PASS Observed first 'transitionend' event. Starting second transition...
+PASS Observed second 'transitionend' event.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/animations/animation-followed-by-two-transitions.html (0 => 265985)
--- trunk/LayoutTests/animations/animation-followed-by-two-transitions.html (rev 0)
+++ trunk/LayoutTests/animations/animation-followed-by-two-transitions.html 2020-08-21 00:12:05 UTC (rev 265985)
@@ -0,0 +1,61 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <script src=""
+ <script src=""
+ <style>
+ #target {
+ position: absolute;
+ top: 100px;
+ left: 100px;
+ height: 100px;
+ width: 100px;
+ background: green;
+ animation: fadeIn 30ms;
+ }
+
+ @keyframes fadeIn {
+ 0% { opacity: 0; }
+ 100% { opacity: 1; }
+ }
+
+ .transition {
+ transition: transform 60ms;
+ transform: translateX(200%);
+ }
+
+ .no-transition {
+ transition: none;
+ transform: translateX(0);
+ }
+ </style>
+</head>
+<body>
+ <div id="target"></div>
+ <script>
+ jsTestIsAsync = true;
+ target = document.getElementById("target");
+
+ description("The test passes if the animationend event is followed by two transitionend events.");
+
+ target.addEventListener("transitionend", async () => {
+ testPassed("Observed first 'transitionend' event. Starting second transition...");
+ target.classList.replace("transition", "no-transition");
+
+ await UIHelper.renderingUpdate();
+ await UIHelper.renderingUpdate();
+
+ target.classList.replace("no-transition", "transition");
+ target.addEventListener("transitionend", () => {
+ testPassed("Observed second 'transitionend' event.");
+ finishJSTest();
+ });
+ }, { once: true });
+
+ target.addEventListener("animationend", () => {
+ testPassed("Observed 'animationend' event. Starting first transition...");
+ target.classList.add("transition");
+ });
+ </script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (265984 => 265985)
--- trunk/Source/WebCore/ChangeLog 2020-08-20 23:53:35 UTC (rev 265984)
+++ trunk/Source/WebCore/ChangeLog 2020-08-21 00:12:05 UTC (rev 265985)
@@ -1,3 +1,45 @@
+2020-08-20 Wenson Hsieh <wenson_hs...@apple.com>
+
+ REGRESSION (r263729): Carousel freezes on "fourth page"/fourth click on right arrow on netflix.com
+ https://bugs.webkit.org/show_bug.cgi?id=215655
+ <rdar://problem/65845979>
+
+ Reviewed by Dean Jackson.
+
+ On netflix.com, when clicking on the left and right arrows in each movie or TV show carousel, the page attempts
+ to animate to the next page of the carousel using a CSS transform transition. The logic applies `transform` and
+ `transition` CSS properties to a container `div`, and adds a `transitionend` event listener which the page
+ expects to be invoked when the animation is complete. While waiting for this `transitionend` event, the script
+ also sets a boolean flag that prevents the carousel from being advanced to any other page. However, after the
+ changes in r263729, the carousel gets into a state where `transition` and `transform` styles are set, but the
+ animation never begins, and thus, no subsequent `transitionend` event is observed. This causes the page to
+ believe that the carousel is indefinitely animating, so it never unsets the boolean flag, which results in the
+ carousel being permanently stuck.
+
+ This occurs because we now have logic in `AnimationTimeline::updateCSSTransitionsForElementAndProperty` that
+ moves the `CSSTransition` from the element's map of running transitions to the map of completed transitions in
+ the case where the corresponding `WebAnimation` is already in `Finished` state. However, consider the case where
+ there is no matching backing animation (i.e. `matchingBackingAnimation` is `nullptr`); for instance, this can
+ happen if the transition CSS property is set to none in the middle of the `transitionend` event, as demonstrated
+ in the new layout test. Before the change, we would've removed the `CSSTransition` from the map of running
+ transitions and canceled it, but now, we instead move it to the map of completed transitions, where it remains
+ until the next CSS transition update is triggered (which would potentially be indefinitely long!).
+
+ On netflix.com, this next CSS transition update happens the page attempts to advance the carousel. Since the old
+ `CSSTransition` is still in the "completed" transitions map, we end up returning `true` when checking
+ `propertyInStyleMatchesValueForTransitionInMap`, and consequently never attempt to create a new `CSSTransition`
+ and add it to the map of running transitions in step 1 of the algorithm. As described above, this causes the
+ carousel to get stuck in a bad state.
+
+ To fix this, we simply revert to pre-r263729 behavior in the case where the matching backing animation was
+ already removed, and allow step 3 of the algorithm to cancel the running animation and remove it altogether
+ instead of moving it into the element's completed transitions map.
+
+ Test: animations/animation-followed-by-two-transitions.html
+
+ * animation/AnimationTimeline.cpp:
+ (WebCore::AnimationTimeline::updateCSSTransitionsForElementAndProperty):
+
2020-08-20 Chris Dumez <cdu...@apple.com>
AudioBufferSourceNode should use "final" values for playbackRate and detune
Modified: trunk/Source/WebCore/animation/AnimationTimeline.cpp (265984 => 265985)
--- trunk/Source/WebCore/animation/AnimationTimeline.cpp 2020-08-20 23:53:35 UTC (rev 265984)
+++ trunk/Source/WebCore/animation/AnimationTimeline.cpp 2020-08-21 00:12:05 UTC (rev 265985)
@@ -387,7 +387,7 @@
// A CSS Transition might have completed since the last time animations were updated so we must
// update the running and completed transitions membership in that case.
- if (is<CSSTransition>(animation) && element.hasRunningTransitionsForProperty(property) && animation->playState() == WebAnimation::PlayState::Finished) {
+ if (is<CSSTransition>(animation) && matchingBackingAnimation && element.hasRunningTransitionsForProperty(property) && animation->playState() == WebAnimation::PlayState::Finished) {
element.ensureCompletedTransitionsByProperty().set(property, element.ensureRunningTransitionsByProperty().take(property));
animation = nullptr;
}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
