[css-view-transitions-1] Should the DOMChangeCallback be posted as a task from skip transition steps?

[vmpstr]

Step 4 in skip the page transition algorithm says that the DOM change callback has to be invoked synchronously. However, the skip the page transition algorithm sometimes runs as part of the update the rendering, and we really want to avoid running (new) script in those steps.

We should make it asynchronous. The question is should we just make it asynchronous for all places that run the skip the page transition algorithm, or should it be conditional?

I'm leaning towards just make it all asynchronous, but I'm unsure if there are cases where synchronous invocation is important.

/cc @khushalsagar @jakearchibald

[khushalsagar]

The downside of making it async is the following sequence:

• Queue a transition with createDocumentTransition.
• While snapshot is pending (so the callback hasn't run) initiate a new transition so the previous one is skipped.

It would be nice if the callback for the previous one consistently happened before or after we capture for the new one. But that depends on whether the event loop already has a task to run the next frame? I think the answer to this is a microtask which would ensure that this runs before the next frame. @flackr on that.

Microtask won't help if multiple transitions are set up within the same frame but that seems like an edge case that the developer can handle better.

[khushalsagar]

Summary from offline discussion. Microtasks need a checkpoint to ensure they run before a specific event. Since there is already a checkpoint for Web Animations, using a microtask will ensure the callback runs before the next rAF. It is brittle to rely on an unrelated checkpoint for this timing though.

Also since the callback itself is async, it's already possible that it hasn't finished before the next rAF. The domUpdated promise was added precisely so developers have a hook to know when the callback is done. That could be used to delay queueing another transition if it should deterministically start after the callback for the first one is done.

Proposed Resolution: Invoking the dom-update-callback is done by positing a task in skip the page transition.

[ydaniv]

If I'm following correctly the proposal is to always post as microtask? In any case it must be consistently one of sync/next task/next microtask to be predictable for authors and also mockable for testing.

[khushalsagar]

The proposal is to use a task instead of microtask. Microtask timing can be unpredictable since the microtask checkpoint which runs them can be triggered by any feature.

[astearns]

@ydaniv (and/or anyone else) are you OK with the clarification and proposed resolution?

[ydaniv]

@astearns SGTM!

[astearns]

OK, seeing consensus here so this should get in the spec for wider review.

The CSSWG will automatically accept this resolution one week from now if no objections are raised here. Anyone can add an emoji to this comment to express support. If you do not support this resolution, please add a new comment.

Proposed Resolution: Invoking the dom-update-callback is done by positing a task in skip the page transition.

[astearns]

RESOLVED: Invoking the dom-update-callback is done by positing a task in skip the page transition.

[noamr]

This would mess up the phases. By making update the callback async we make it happen after the ready/finish phases. Integrating it as per the resolution would assert at call the update callback step 1 (wrong phase).

Note that the change callback is called synchronously during the update phase also when it's not skiped, when preparing the transition.

IMO the callback should be called synchronouosly or the whole state machine becomes botched.

[khushalsagar]

IMO the callback should be called synchronouosly

That's not feasible because we can execute this step very late in the update the rendering steps. Specifically, step 8.7.5 :

If transition’s initial snapshot root size is not equal to the snapshot root size, then skip the view transition for transition, and return.

The "Handle transition frame" algorithm is triggered here, right before step 16. The goal behind running it this late is that its supposed to reflect DOM changes into the corresponding pseudo-elements. So we want to run it at a point when script is guaranteed to not modify the DOM. But that also means we're at a point where running script is bad. It won't be logically incorrect for this feature but adding a hook where script can run implies the DOM can be dirtied and we'll need to re-run style/layout again.

We can make the async approach by doing the following:

• In step 8.4.1 assert that phase is either before update-callback-called, which means this is running in the regular transition flow. Or assert that its done, which means this is happening because the transition was skipped before the callback was invoked.
• Make step 8.4.5 conditional to only update the phase if it was before update-callback-called.
• Add step 8.4.7 to invoke the finished promise if the phase is done. This means we delay dispatching the finished promise until the update callback is done if we're skipping the transition.
• Step 8.5.4 will post a task to call the update function.
• In step 8.5.9, don't resolve the finished promise if this task was posted.

I'll try to dig up the exact issue but IIRC we resolved on the fact that ready promise should reject immediately if you skip before call the update callback but finished should mirror the result of that callback.

[noamr]

IMO the callback should be called synchronouosly

That's not feasible because we can execute this step very late in the update the rendering steps. Specifically, step 8.7.5 :

If transition’s initial snapshot root size is not equal to the snapshot root size, then skip the view transition for transition, and return.

The "Handle transition frame" algorithm is triggered here, right before step 16. The goal behind running it this late is that its supposed to reflect DOM changes into the corresponding pseudo-elements. So we want to run it at a point when script is guaranteed to not modify the DOM. But that also means we're at a point where running script is bad. It won't be logically incorrect for this feature but adding a hook where script can run implies the DOM can be dirtied and we'll need to re-run style/layout again.

I don't get why running scripts here is bad? It's not different from running scripts in resize/intersection observers/rAF.

What adding a postTask here would cause is a potential flicker. you'd have a moment where on one hand rendering is allowed ("transition suppressing rendering" is off), and OTOH the update callback wasn't called.

In the current spec, from the moment you start a transition it is guaranteed that there's no intermediate rendering until the transition is resolved. That's great. With this change, there would be this uncanny-valley moment of "the previous transition has not resolved but rendering is allowed". Of course, we could also put step 5 (Set transition suppressing rendering to false) in that task, but what would we gain? Allowing event/timeout handling while rendering is suppressed, which has to now take the intermediate suppressed state into account?

If we suppress rendering anyway, we might as well do everything synchronously and perform an additional style and layout if needed.

[khushalsagar]

What adding a postTask here would cause is a potential flicker. you'd have a moment where on one hand rendering is allowed ("transition suppressing rendering" is off), and OTOH the update callback wasn't called.

Just reiterating the case which could potentially flicker:

function cancelTransition(transition) {
  // Author assumes that update callback was dispatched synchronously here.
  transition.skipTransition();

  // Author updates DOM assuming the update callback was already run.
  // This causes a flicker when the update callback is dispatched.
  updateDOMAfterTransition();
}

The update callback itself is async. In the case above, even if dispatched the update callback synchronously in skipTransition, the author still has to ensure its completed before running updateDOMAfterTransition. They can use updateCallbackDone promise for it.

function cancelTransition(transition) {
  transition.skipTransition();
  transition.updateCallbackDone.then(updateDOMAfterTransition);
}

Also note that we don't want to suppress rendering until the callback finishes if the transition was skipped. We don't know if the author will make the updateDOM callback a no-op in this case.

[noamr]

Thanks @khushalsagar, this gives me enough context, I'll proceed with a PR based on the resolution.