Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-view-transitions-1] Refactor timings of updateCallbackDone and related #9774

Merged
merged 4 commits into from Jan 11, 2024

Conversation

noamr
Copy link
Collaborator

@noamr noamr commented Jan 8, 2024

Note: this change is not a behavior change, but rather a clarification of existing expected behavior.

  • Instead of reacting to the update callback's promise in several places, we react to it once with fulfill/reject steps and continue based on the state, all inside the "call the update callback" algorithm.
  • Clarify that in the successful case, updateCallbackDone is always resolved after the new state capture but before the ready promise. (This is current behavior, but not explicit in the current spec).
  • Fix a bug where we were resolving a promise with the result of reacting to a different promise, which is wrong use of those operations.
  • Use normative language for user-agent transition timeout
  • Remove redundant early returns for done phase.

Closes #9762


- If the promise was fulfilled, then return undefined.
1. If |transition|'s [=ViewTransition/phase=] is "`done`", then [=resolve=] |transition|'s [=ViewTransition/update callback done promise=] with undefined.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolving multiple times is a no-op right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would we resolve multiple times?

Copy link
Collaborator Author

@noamr noamr Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I guess if setting up the pseudo-elements fail... yea, https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-promise-resolve-functions step 5

We might not even need the if, we can simply resolve and rely on that behavior. WDYT?

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

@khushalsagar khushalsagar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the spec here!

1. [=Skip the view transition=] |transition| with |reason|.

* If the promise was fulfilled, then [=activate view transition|activate=] |transition|.
Note: A task is queued here because the texture read back in [=capturing the image=] may be async,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind restoring the position of this note? Nicer to have this next to where we post the task. Otherwise its part of this sub-point which is less clearer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

css-view-transitions-1/Overview.bs Show resolved Hide resolved
@khushalsagar khushalsagar merged commit 9d5ed6f into w3c:main Jan 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[css-view-transitions-1] Clarify timing of updateCallbackDone
2 participants