Bug 221961 - REGRESSION(r271515): ::marker fired at wrong time
Summary: REGRESSION(r271515): ::marker fired at wrong time
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
Keywords: InRadar
Depends on:
Reported: 2021-02-16 06:39 PST by Sam Sneddon [:gsnedders]
Modified: 2021-02-16 14:01 PST (History)
4 users (show)

See Also:

Patch (3.70 KB, patch)
2021-02-16 10:52 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Antoine Quint 2021-02-16 07:46:26 PST
Probably regressed with r271515, confirming this now.
Comment 2 Antoine Quint 2021-02-16 08:38:21 PST
Indeed, it is, regressed due to r271515, the fix for bug 220652.
Comment 3 Antoine Quint 2021-02-16 08:39:55 PST
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.
Comment 4 Antoine Quint 2021-02-16 09:03:21 PST
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));
     // FIXME: Track this exactly.

But I think this should be handled by the compareDeclarativeAnimationOwningElementPositionsInDocumentTreeOrder() utility.
Comment 5 Antoine Quint 2021-02-16 09:10:28 PST
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.
Comment 6 Antoine Quint 2021-02-16 10:49:41 PST
That said, touching the event sorting code is a bit risky so I'll do the first fix which will restore functionality.
Comment 7 Radar WebKit Bug Importer 2021-02-16 10:50:11 PST
Comment 8 Antoine Quint 2021-02-16 10:52:21 PST
Created attachment 420499 [details]
Comment 9 EWS 2021-02-16 14:01:39 PST
Committed r272927: <https://commits.webkit.org/r272927>

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