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] Clarify how unhandled rejections should work #8451

Closed
wants to merge 1 commit into from

Conversation

jakearchibald
Copy link
Contributor

@jakearchibald jakearchibald commented Feb 14, 2023

@domenic I'd like your feedback on this, as I'm not sure if this is the correct way to spec this.

Right now, in Chrome, this is a silent failure:

const transition = document.startViewTransition(() => {
  doesNotExist();
});

…which is bad. Whereas:

const transition = document.startViewTransition(() => {
  doesNotExist();
});

transition.updateCallbackDone;
transition.ready;
transition.finished;

…causes three unhandled rejections. That seems correct, since all three promises reject.

I want to spec it so, in the first example, there's one unhandled rejection, but without creating four unhandled rejections in the second example.

I've tried to do this by creating only one unhandled promise when the ViewTransition instance is created (transition.updateCallbackDone). The other promises are handled, and only create unhandled promises when their properties are accessed.

Am I doing this right?

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I question the desired behavior here. Accessing a property should not have the side effect of creating an unhandled rejection. It should behave as if the promise is already there, i.e., https://w3ctag.github.io/design-principles/#attributes-like-data .

There are ways to get what you want, by creating duplicate promises and side-effecting getters that generate them or return them. (I.e., basically manually doing what you though [SameObject] did). But it isn't great. What we've done in the navigation API is to nominate a single promise (committed) as not-automatically-handled, and then unconditionally mark the other one (finished) as handled. I know you aren't a big fan of that (WICG/navigation-api#255). Perhaps you should just bite the bullet and allow 3 unhandled rejections per failure.

I don't know how Chrome is behaving, by the way. If it's following the current spec (before this PR's change), there should be 3 unhandled rejections in both code snippets.

readonly attribute Promise<undefined> ready;
readonly attribute Promise<undefined> finished;
[SameObject] readonly attribute Promise<undefined> ready;
[SameObject] readonly attribute Promise<undefined> finished;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not valid Web IDL, technically. whatwg/webidl#71

@@ -1032,9 +1040,12 @@ urlPrefix: https://html.spec.whatwg.org/multipage/rendering.html; type: dfn;
[Discussion of this behavior](https://github.com/w3c/csswg-drafts/issues/8045).
</dl>

The {{ViewTransition/finished}} [=getter steps=] are to return [=this's=] [=ViewTransition/finished promise=].
The {{ViewTransition/finished}} [=getter steps=] are to return [=a promise resolved with=] [=this's=] [=ViewTransition/finished promise=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will return a new promise each time, since the getter steps are run each time. (Even if [SameObject] were allowed, it doesn't do magic caching behavior for you; it's more of a code comment. See whatwg/webidl#212.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, TIL. I could have sworn I'd seen specs written assuming SameObject had this effect.

@jakearchibald
Copy link
Contributor Author

@domenic thanks for the feedback.

If updateCallbackDone rejects, the other two promises will always reject with the same error. Would it be ok then, to mark the other two as handled, at the point of rejecting updateCallbackDone?

@domenic
Copy link
Collaborator

domenic commented Feb 15, 2023

Yeah, that's what we do for the navigation API; we mark finished as handled when we reject committed and finished at the same time.

@jakearchibald
Copy link
Contributor Author

Closing in favour of #8454

@jakearchibald jakearchibald deleted the handled-promises branch February 15, 2023 09:58
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.

2 participants