-
Notifications
You must be signed in to change notification settings - Fork 681
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][css-view-transitions-2] Initial draft for cross-document view transitions #9022
Conversation
css-view-transitions-2/Overview.bs
Outdated
To <dfn>reveal {{Document}}</dfn> |document|: | ||
1. Assert: |document|'s [=page showing=] is false. | ||
|
||
1. If |document| is [=render-blocked=] then return. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this check? We call this function when render blocking is finished (or timed out). So assert that we're not render-blocked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also call it on reactivation, which could happen when the document is still render blocked (e.g. you activate a prerendered page really quickly before it's unblocked). To clarify, I moved this line into the reactivation bit.
Co-authored-by: Khushal Sagar <63884798+khushalsagar@users.noreply.github.com>
Co-authored-by: Khushal Sagar <63884798+khushalsagar@users.noreply.github.com>
Co-authored-by: Khushal Sagar <63884798+khushalsagar@users.noreply.github.com>
257239a
to
e0989f4
Compare
css-view-transitions-2/Overview.bs
Outdated
|
||
1. [=reveal document|reveal=] |document|. | ||
|
||
Reword the text in the [=render-blocked=] definition, to call [=reveal document=] |document| if the [=implementation-defined=] timeout value has been exceeded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
css-view-transitions-2/Overview.bs
Outdated
|
||
1. |outboundTransition|'s [=ViewTransition/phase=] is not "`pending-capture`", then call |onReady| and return. | ||
|
||
1. If |oldDocument| does not [=opt in to cross-document view transitions|opts in to cross-document view transitions=], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a check here? Didn't we already do it before initiating the capture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case the developer changes the opt-in in a rAF callback. It's questionable though, I don't mind removing this for now, maybe replace with an inline issue?
css-view-transitions-2/Overview.bs
Outdated
<div algorithm> | ||
To get the <dfn>inbound cross-document view-transition</dfn> for a {{Document}} |document|: | ||
|
||
1. If |document| does not [=opt in to cross-document view transitions=], then return null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have a check for this here. If there is an "active view transition" whose "is cross document" is true then that's always returned. The places where the opt in is checked should be explicit and we should clear the "active view transition" at those places if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
css-view-transitions-2/Overview.bs
Outdated
|
||
1. Let |transition| be |document|'s [=active view transition=]. | ||
|
||
1. If |transition| is null or |transition|'s [=ViewTransition/is cross-document=] is false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Do you mind using "is inbound cross-document"? Just to make it explicit that the bit is only set for the VT object on the new Document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Co-authored-by: Khushal Sagar <63884798+khushalsagar@users.noreply.github.com>
Co-authored-by: Khushal Sagar <63884798+khushalsagar@users.noreply.github.com>
Co-authored-by: Khushal Sagar <63884798+khushalsagar@users.noreply.github.com>
Co-authored-by: Khushal Sagar <63884798+khushalsagar@users.noreply.github.com>
css-view-transitions-1/Overview.bs
Outdated
@@ -1565,6 +1572,8 @@ urlPrefix: https://wicg.github.io/navigation-api/; type: interface; | |||
|
|||
1. [=Assert=]: |transition|'s [=ViewTransition/phase=] is not "`done`". | |||
|
|||
1. If |transition|'s [=ViewTransition/phase=] is "`pending-capture`" and |transition|'s [=ViewTransition/process old state captured=] is not null, then call [=ViewTransition/process old state captured=] and return. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the phase value be updated before calling "process old state captured"? Otherwise that algorithm will incorrectly assume that the capture was successful.
Doing this as the last step is a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
css-view-transitions-2/Overview.bs
Outdated
|
||
1. Let |transition| be the result of getting the [=inbound cross-document view-transition=] for |document|. | ||
|
||
1. If |document| does not [=opt in to cross-document view transitions=], then [=skip the view transition|skip=] |transition| and set |transition| to null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Do we need a null check for transition
here? Also, I think you want say set document's active view transition to null. Mapping this to code, transition
is now a local variable in this algorithm so resetting it won't reset the member variable on Document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skip transition already sets the active view transition to null.
Fixed the nit.
This lays the foundations for cross-document view transitions.
Specifically:
See https://github.com/WICG/view-transitions/blob/main/cross-doc-explainer.md