REGRESSION(r271515): ::marker fired at wrong time

• Component: Animations
• Status: RESOLVED FIXED

[Sam Sneddon [:gsnedders]]

https://wpt.fyi/results/css/css-animations/event-order.tentative.html?q=seq%28status%3Apass%7Cstatus%3Aok%20status%3Apass%7Cstatus%3Aok%20status%3Apass%7Cstatus%3Aok%20status%3Apass%7Cstatus%3Aok%20status%3Apass%7Cstatus%3Aok%20status%3A%21pass%26status%3A%21ok%20status%3A%21pass%26status%3A%21ok%20status%3A%21pass%26status%3A%21ok%20status%3A%21pass%26status%3A%21ok%20status%3A%21pass%26status%3A%21ok%29&run_id=5703710288117760&run_id=5730791449427968&run_id=5176095785615360&run_id=5761234664161280&run_id=6289110253699072&run_id=6295692878282752&run_id=5717790667309056&run_id=6252961795670016&run_id=5112363554439168&run_id=5651017565732864

[Antoine Quint]

Probably regressed with r271515, confirming this now.

[Antoine Quint]

Indeed, it is, regressed due to r271515, the fix for bug 220652.

[Antoine Quint]

Just looking at the very simple patch, the order in which pseudo-elements are generated is different since in the PseudoId enum the relative order is Marker, Before, After. While changing the order in which we process animations likely addresses this, I don't think this should be relevant and there is some sorting code somewhere that hasn't been updated for ::marker support.

[Antoine Quint]

Indeed, this simple patch fixes the regression:

diff --git a/Source/WebCore/style/StyleTreeResolver.cpp b/Source/WebCore/style/StyleTreeResolver.cpp
index a8a107e6e6aa..6af316dadfa4 100644
--- a/Source/WebCore/style/StyleTreeResolver.cpp
+++ b/Source/WebCore/style/StyleTreeResolver.cpp
@@ -249,12 +249,12 @@ ElementUpdates TreeResolver::resolveElement(Element& element)
     }
 
     PseudoIdToElementUpdateMap pseudoUpdates;
+    if (auto markerElementUpdate = resolvePseudoStyle(element, update, PseudoId::Marker))
+        pseudoUpdates.set(PseudoId::Marker, WTFMove(*markerElementUpdate));
     if (auto beforeElementUpdate = resolvePseudoStyle(element, update, PseudoId::Before))
         pseudoUpdates.set(PseudoId::Before, WTFMove(*beforeElementUpdate));
     if (auto afterElementUpdate = resolvePseudoStyle(element, update, PseudoId::After))
         pseudoUpdates.set(PseudoId::After, WTFMove(*afterElementUpdate));
-    if (auto markerElementUpdate = resolvePseudoStyle(element, update, PseudoId::Marker))
-        pseudoUpdates.set(PseudoId::Marker, WTFMove(*markerElementUpdate));
 
 #if ENABLE(TOUCH_ACTION_REGIONS)
     // FIXME: Track this exactly.


But I think this should be handled by the compareDeclarativeAnimationOwningElementPositionsInDocumentTreeOrder() utility.

[Antoine Quint]

Oh, we simply don't invoke the composite order sorting code when dispatching events, in DocumentTimelinesController::updateAnimationsAndSendEvents():

    // 6. Perform a stable sort of the animation events in events to dispatch as follows.
    std::stable_sort(events.begin(), events.end(), [] (const Ref<AnimationEventBase>& lhs, const Ref<AnimationEventBase>& rhs) {
        // 1. Sort the events by their scheduled event time such that events that were scheduled to occur earlier, sort before events scheduled to occur later
        // and events whose scheduled event time is unresolved sort before events with a resolved scheduled event time.
        // 2. Within events with equal scheduled event times, sort by their composite order. FIXME: Need to do this.
        return lhs->timelineTime() < rhs->timelineTime();
    });

So this test progressed when we added support for ::marker, but in a very fragile way that just worked out of luck.

[Antoine Quint]

That said, touching the event sorting code is a bit risky so I'll do the first fix which will restore functionality.

[Radar WebKit Bug Importer]

<rdar://problem/74397846>

[Antoine Quint]

Created attachment 420499 [details]
Patch

[EWS]

Committed r272927: <https://commits.webkit.org/r272927>

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