[css-view-transitions-2] Editorial: refactor L2 concerns out of L1 #9892

noamr · 2024-02-02T12:20:47Z

L2 redefines appropriate algorithms and additional fields instead of relying on L1
moved onReady to correct place, and renamed proceedWithNavigation.

css-view-transitions-2/Overview.bs

khushalsagar · 2024-02-07T19:07:40Z

css-view-transitions-1/Overview.bs

@@ -1955,6 +1937,7 @@ Changes from <a href="https://www.w3.org/TR/2023/WD-css-view-transitions-1-20230
 * Add note to explain paint order for entry animations. See <a href="https://github.com/w3c/csswg-drafts/issues/9672">issue 9672</a>.
 * Add note to explain how the named elements are cleaned up. See <a href="https://github.com/w3c/csswg-drafts/issues/9669">issue 9669</a>.
 * Refactor algorithm to clarify timing, especially of `updateCallbackDone. See <a href="https://github.com/w3c/csswg-drafts/issues/9762">issue 9762</a>.
+* Remove references to cross-document view-transitions, to keep the L1 spec clean.


nit: Add a link to the relevant issue.

css-view-transitions-2/Overview.bs

css-view-transitions-1/Overview.bs

khushalsagar · 2024-02-07T19:30:48Z

css-view-transitions-2/Overview.bs


-			Issue: should we check for the opt-in again, in case there was a CSSOM change in a requestAnimationFrame callback?
+		1. Run the following steps when [=Perform pending transition operations|performing pending transition operations=]:


Hmmm, I actually liked your approach of stashing the proceedWithNavigation callback on the VT object. Because we also need to invoke it when the transition is skipped by script before the next frame could've run. In that case "perform pending transition operations" will never happen.

How about setting this as follows:

L2 adds a proceedWithNavigation callback on VT that gets invoked as the last step in the skip algorithm.

We define a new algorithm in L2 for the cross-doc case which is a parallel to https://drafts.csswg.org/css-view-transitions-1/#setup-view-transition in L1. All the steps below go there.
In fact we can rename the L1 version to "setup-same-doc-view-transition" and call the L2 one "setup-cross-doc-view-transition-on-old-doc".

Redefine https://drafts.csswg.org/css-view-transitions-1/#perform-pending-transition-operations in L2 to add a step in the beginning which says, "If active view transition is not null and its cross-doc for old document, then setup-cross-doc-view-transition-on-old-doc and return". When the specs are merged we'll basically split which algorithm is invoked depending on which case you're on.

I'm reviewing the rest of the algorithm assuming |outboundTransition| is the document's active view transition referenced here.

Since we're dealing with task sources in this spec, good to document in the "setup-cross-doc-view-transition-on-old-doc" algo that it runs on the rendering task source.

It's not though... it's running on the navigation and traversal task source.
Ack regarding calling the callback when skipped.

khushalsagar · 2024-02-07T22:20:35Z

css-view-transitions-2/Overview.bs


-				Note: The ViewTransition object on the old Document should be destroyed after its state has been copied to the new Document below.
-					We explicitly clear it here since the old Document may be cached by the UA.
+			1. [=Capture the old state=] for |outboundTransition|.


We need to handle the fact that capture is async, see note here.

I think that means setting the rendering suppression bit to true here and unsetting both rendering suppression + auto-skip VT in the task queued below?

Reminder for this. We need to set rendering suppression for view transitions to true here if capture old state succeeds and post a task to run the success steps which also reset this bit. This is to allow async capture.

Why is that needed? We don't render any more frames on the old page, so supposedly nothing to suppress.

khushalsagar · 2024-02-07T22:21:40Z

css-view-transitions-2/Overview.bs


-			1. Set |oldDocument|'s [=auto-skip view transitions=] to false.
+			1. If failure is returned, call |proceedWithNavigation| and return.


Simpler to call skip the VT here? Assuming you update the skip algorithm for the case I mentioned above.

css-view-transitions-2/Overview.bs

khushalsagar · 2024-02-07T22:46:32Z

css-view-transitions-2/Overview.bs


-			1. Set |oldDocument|'s [=auto-skip view transitions=] to false.
+			1. If failure is returned, call |proceedWithNavigation| and return.


"proceedWithNavigation" will be invoked from the rendering task source if done from "perform pending transition operations". And whatever task source script runs in if done via viewTransition.skipTransition().

Also, in the case where we don't even set up an outbound transition, "proceedWithNavigation" is invoked synchronously at the html call-site. So now it will run on whatever source "setup cross-document view-transition" is called from.

It seems odd if the navigation steps are running on different task sources in the 3 cases. Can you document the API contract on where navigation steps should be run, based on what happens in the non-VT case. And ensure we consistently use the same task source for the VT case?

This is handled in the HTML spec. The callback that's passed here queues steps on the traversal queue (browser process), so it doesn't matter who calls it.

noamr · 2024-02-08T14:15:21Z

@khushalsagar I've moved things around a bit, take a look, I think it addresses your concerns.

css-view-transitions-1/Overview.bs

khushalsagar · 2024-02-08T23:13:44Z

css-view-transitions-2/Overview.bs


-		1. Let |viewTransition| be the result of running [=method steps=] for {{Document/startViewTransition(updateCallback)}} given |callbackOptions|'s {{StartViewTransitionOptions/update}}.
+			1. Let |transition| be a new {{ViewTransition}} in |this|'s [=relevant realm=].


I don't understand. We created transition, settled all its promises but then didn't do anything with it? I'm guessing you missed a return at the end. But even so, this misses dispatching the update callback.

My preference would be to set the update callback on |transition| and then invoke the skip algorithm, instead of duplicating the promise handling logic here. The only thing that needs to change in the skip algorithm for this case is to make this step conditional: "If document’s active view transition is transition, set rendering suppression for view transitions to false".

khushalsagar · 2024-02-09T02:19:41Z

css-view-transitions-2/Overview.bs


 		1. Return |viewTransition|.
 	</div>

+	A {{Document}} additionaly has:


nit: Move this to the "Data Structures" block below, similar to the L1 spec.

khushalsagar · 2024-02-09T02:47:37Z

css-view-transitions-2/Overview.bs


-			1. If |outboundTransition|'s [=ViewTransition/phase=] is "`done`", then call |onReady| and return.
+		1. Run the following steps when [=Perform pending transition operations|performing pending transition operations=]:

 			1. Assert: |outboundTransition|'s  [=ViewTransition/phase=] is "`pending-capture`".


Use the document's active view transition here instead of binding these steps to |outboundTransition|.

The assert only makes sense if it's the document's active view transition. If the author skips |outboundTransition| in script before the next frame, then it will also be cleared as the active view transition.

khushalsagar · 2024-02-09T02:57:48Z

css-view-transitions-2/Overview.bs


-			1. If |outboundTransition|'s [=ViewTransition/phase=] is "`done`", then call |onReady| and return.
+		1. Run the following steps when [=Perform pending transition operations|performing pending transition operations=]:


Please move this to a separate "Additions to perform pending transition operations" section. Makes it much easier to see the diff where we're adding to existing algorithms in L2. And also when we eventually merge.

The text can say something like:

Prepend this to the perform pending transitions algorithm:
If document’s active view transition is not null and its is outbound cross-doc is true, then:
...

css-view-transitions-2/Overview.bs

khushalsagar · 2024-02-09T03:11:18Z

css-view-transitions-2/Overview.bs


-				Note: The ViewTransition object on the old Document should be destroyed after its state has been copied to the new Document below.
-					We explicitly clear it here since the old Document may be cached by the UA.
+			1. [=Capture the old state=] for |outboundTransition|.


Reminder for this. We need to set rendering suppression for view transitions to true here if capture old state succeeds and post a task to run the success steps which also reset this bit. This is to allow async capture.

khushalsagar · 2024-02-09T03:13:31Z

css-view-transitions-2/Overview.bs

+
+				1. Let |initialSnapshotContainingBlockSize| to |outboundTransition|'s [=ViewTransition/initial snapshot containing block size=].
+
+				1. Set |newDocument|'s [=inbound view transition steps=] to the following steps:


I actually find it cleaner to stash initialSnapshotContainingBlockSize and clonedNamedElements on the Document and inline these steps into "resolve cross-document view-transition", where they run. Instead of binding them to a callback and then stashing that on the new Document.

khushalsagar · 2024-02-09T18:51:44Z

css-view-transitions-2/Overview.bs


-		1. If |callbackOptions| is an {{UpdateCallback}}, then run the [=method steps=] for {{Document/startViewTransition(updateCallback)}} given |callbackOptions| and return the result.
+			1. Let |preSkippedTransition| be a new {{ViewTransition}} in |this|'s [=relevant realm=].


We need to set up the update callback on |preSkippedTransition| before calling skip below, otherwise the update callback will never run.

khushalsagar · 2024-02-09T18:53:32Z

css-view-transitions-2/Overview.bs


-		1. Let |viewTransition| be the result of running [=method steps=] for {{Document/startViewTransition(updateCallback)}} given |callbackOptions|'s {{StartViewTransitionOptions/update}}.
+			1. [=Skip the view transition|Skip=] |preSkippedTransition| with an "{{InvalidStateError}}" {{DOMException}}.


Step 4 in "Skip the view transition" algorithm in L1 needs to be updated to: "If document’s active view transition is transition, Set rendering suppression for view transitions to false."

Otherwise this will resume rendering while the outbound transition is being captured.

I still don't understand why we need rendering suppression. Is that for that unspecified async capture?

khushalsagar · 2024-02-09T18:54:54Z

css-view-transitions-2/Overview.bs


 		: <dfn>active types</dfn>
 		:: Null or a [=list=] of strings, initially null.
 	</dl>

+### [=Captured elements=] ### {#captured-elements}


Did you mean to add anything here?

khushalsagar · 2024-02-09T19:00:04Z

css-view-transitions-2/Overview.bs

-
-			Note: |outboundTransition| is not exposed to JavaScript, it is used only for capturing
-			the state of the old document.
+			1. Call |proceedWithNavigation|.


nit: Update the note below, "The process continues in [=perform pending transition operations=]."

khushalsagar · 2024-02-09T19:01:40Z

css-view-transitions-2/Overview.bs

-			transition was started before the {{Window/pagereveal}} event was fired.
+			Note: this means that starting a same-document transition before revealing the document would cancel a pending cross-document transition.
+
+		1. Let |transition| be a new {{ViewTransition}} in |document|'s [=relevant Realm=],


nit: Should we move this after resolving the rule below? Other |transition| is unused.

css-view-transitions-2/Overview.bs

khushalsagar · 2024-02-09T19:07:40Z

css-view-transitions-2/Overview.bs

+			Note: The ViewTransition object on the old Document should be destroyed after its state has been copied to the new Document below.
+				We explicitly clear it here since the old Document may be cached by the UA.
+
+		1. Call |transition|'s [=outbound post-capture steps=] given |viewTransitionParams|.


This needs to be done in a posted task because "Capture the old state" can be async. So we set "rendering suppression" to true, post a task which resets that bit and then invoke [=outbound post-capture steps=]. In that task, also check in case the phase for the transition is done (skip might have been called).

The L1 spec does the same in the task posted here.

The L1 spec in this case is a bit dodgy... posting a task doesn't guarantee anything wrt that async capture. We need to think about this further.

css-view-transitions-1/Overview.bs

khushalsagar · 2024-02-13T17:59:13Z

css-view-transitions-2/Overview.bs


-		1. Set |oldDocument|'s [=active view transition=] to |outboundTransition|.
+		1. React to |outboundTransition|'s [=ViewTransition/finished promise=]:


I really don't like this. We're just making our spec unnecessarily complicated by relying on promises for browser steps. I also suggested adding an "addition to skip steps" style of update for this because that's what we'll do when the specs are eventually merged.

See w3c#9886 (comment)

Co-authored-by: Khushal Sagar <[email protected]>

noamr requested a review from khushalsagar February 2, 2024 12:20

khushalsagar reviewed Feb 2, 2024

View reviewed changes

css-view-transitions-2/Overview.bs Show resolved Hide resolved

noamr changed the title ~~[css-view-transitions-2] Move onReady to the correct place and rename~~ [css-view-transitions-2] Editorial: refactor L2 concerns out of L1 Feb 6, 2024

khushalsagar reviewed Feb 7, 2024

View reviewed changes

khushalsagar reviewed Feb 9, 2024

View reviewed changes

noamr force-pushed the vt2-edits-2 branch from c4ad71d to 3ec772d Compare February 9, 2024 13:42

khushalsagar reviewed Feb 9, 2024

View reviewed changes

khushalsagar reviewed Feb 13, 2024

View reviewed changes

css-view-transitions-1/Overview.bs Show resolved Hide resolved

khushalsagar reviewed Feb 13, 2024

View reviewed changes

khushalsagar approved these changes Feb 13, 2024

View reviewed changes

noamr and others added 13 commits February 13, 2024 19:26

Move onReady to the correct place and rename

4d40d9b

See w3c#9886 (comment)

Call proceed with navigation after capturing old doc

b0742a1

Move all cross-doc references to L2

c64adb0

Update css-view-transitions-2/Overview.bs

d33b40c

Co-authored-by: Khushal Sagar <[email protected]>

Restructure new document steps and auto-skip

fa716ea

Add missing piece

34925bb

Remove more references to L2

4193a78

Address PR nits

1720ab1

Address PR nits

1c621b3

Address PR nits

5bb1973

nits

6606c42

Add rendering suppression

8e14621

Move skip steps to monkey patch

d4c7d6d

noamr force-pushed the vt2-edits-2 branch from fbee935 to d4c7d6d Compare February 13, 2024 19:26

noamr merged commit 6b3259f into w3c:main Feb 13, 2024

noamr deleted the vt2-edits-2 branch February 13, 2024 19:27


		Issue: should we check for the opt-in again, in case there was a CSSOM change in a requestAnimationFrame callback?
		1. Run the following steps when [=Perform pending transition operations\|performing pending transition operations=]:


		1. Set \|oldDocument\|'s [=auto-skip view transitions=] to false.
		1. If failure is returned, call \|proceedWithNavigation\| and return.


		1. Let \|viewTransition\| be the result of running [=method steps=] for {{Document/startViewTransition(updateCallback)}} given \|callbackOptions\|'s {{StartViewTransitionOptions/update}}.
		1. Let \|transition\| be a new {{ViewTransition}} in \|this\|'s [=relevant realm=].


		1. If \|outboundTransition\|'s [=ViewTransition/phase=] is "`done`", then call \|onReady\| and return.
		1. Run the following steps when [=Perform pending transition operations\|performing pending transition operations=]:


		1. Let \|initialSnapshotContainingBlockSize\| to \|outboundTransition\|'s [=ViewTransition/initial snapshot containing block size=].

		1. Set \|newDocument\|'s [=inbound view transition steps=] to the following steps:


		1. If \|callbackOptions\| is an {{UpdateCallback}}, then run the [=method steps=] for {{Document/startViewTransition(updateCallback)}} given \|callbackOptions\| and return the result.
		1. Let \|preSkippedTransition\| be a new {{ViewTransition}} in \|this\|'s [=relevant realm=].


		1. Let \|viewTransition\| be the result of running [=method steps=] for {{Document/startViewTransition(updateCallback)}} given \|callbackOptions\|'s {{StartViewTransitionOptions/update}}.
		1. [=Skip the view transition\|Skip=] \|preSkippedTransition\| with an "{{InvalidStateError}}" {{DOMException}}.


		1. Set \|oldDocument\|'s [=active view transition=] to \|outboundTransition\|.
		1. React to \|outboundTransition\|'s [=ViewTransition/finished promise=]:

[css-view-transitions-2] Editorial: refactor L2 concerns out of L1 #9892

[css-view-transitions-2] Editorial: refactor L2 concerns out of L1 #9892

Uh oh!

Conversation

noamr commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamr commented Feb 8, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

noamr commented Feb 2, 2024 •

edited

Loading