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

Editorial: Extract cross-document swap algo out #10178

Merged
merged 10 commits into from
Mar 15, 2024

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Mar 5, 2024

The cross-document deactivation part of the navigation algorithm is an indentation-hell of lambdas. Refactoring it to a couple of top-level algos instead.

(See WHATWG Working Mode: Changes for more details.)


/browsing-the-web.html ( diff )
/dom.html ( diff )
/infrastructure.html ( diff )

@noamr noamr requested a review from domenic March 5, 2024 15:14
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
Comment on lines 101748 to 101753
<p>To <dfn>deactivate a document for a cross-document navigation</dfn>, run these steps given a
<code>Document</code> <var>displayedDocument</var>, a <span>user navigation involvement</span>
<var>userNavigationInvolvement</var>, a <span>source snapshot params</span>
<var>sourceSnapshotParams</var>, a <span>session history entry</span> <var>targetEntry</var>, a
<code>NavigationType</code> <var>navigationType</var>, and <var>afterPotentialUnloads</var>, which
is an algorithm that receives no argument:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p>To <dfn>deactivate a document for a cross-document navigation</dfn>, run these steps given a
<code>Document</code> <var>displayedDocument</var>, a <span>user navigation involvement</span>
<var>userNavigationInvolvement</var>, a <span>source snapshot params</span>
<var>sourceSnapshotParams</var>, a <span>session history entry</span> <var>targetEntry</var>, a
<code>NavigationType</code> <var>navigationType</var>, and <var>afterPotentialUnloads</var>, which
is an algorithm that receives no argument:
<p>To <dfn>deactivate a document for a cross-document navigation</dfn> given a
<code>Document</code> <var>displayedDocument</var>, a <span>user navigation involvement</span>
<var>userNavigationInvolvement</var>, a <span>source snapshot params</span>
<var>sourceSnapshotParams</var>, a <span>session history entry</span> <var>targetEntry</var>, a
<code>NavigationType</code> <var>navigationType</var>, and <var>afterPotentialUnloads</var>, which
is an algorithm that receives no argument:

source Show resolved Hide resolved
@domenic
Copy link
Member

domenic commented Mar 13, 2024

I forgot to say, this is a big improvement, so thank you for doing this. Removing a level of indirection around the unload steps makes it a bit hard to match up the old and new versions, but greatly improves clarity.

source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
data-x="source-snapshot-params-view-transition-opt-in">view transition opt-in</span> is true,
<li><p>Let <var>viewTransitionRule</var> be the result of calling
<span>resolve <code>@view-transition</code> rule</span> given
<var>displayedDocument</var>.</p></li>
Copy link
Member

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 can check these things from inside the session history traversal queue. Snapshotting beforehand seemed more correct to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way this works in Chromium is that we put a flag on the document that's readable from the traversal queue and change it when the css changes. So I think specing it like this is more accurate?

Also sourceSnapshotParams are about the source document, so they are the wrong concept here IIUC. They are often null.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like you need to spec that process, of updating such a flag. Right now it looks like it's crawling through the list of CSS rules and doing style computation from the traversal queue...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, see w3c/csswg-drafts#10078 and new revision

Copy link
Member

Choose a reason for hiding this comment

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

This new version looks great, thank you for doing that.

Although, I wonder if it'd be better to call into the CSS spec once instead of twice? I.e. instead of

If displayedDocument's can initiate outbound view transition is true, then set potentiallyTriggerViewTransition to the result of calling can navigation trigger a cross-document view-transition? given displayedDocument's origin...

something like

Set potentiallyTriggerViewTransition to the result of calling can navigation trigger a cross-document view transition? given displayedDocument, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea at this point this would be better, I wrote it the other way when we used the snapshot--params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See new revision.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
data-x="source-snapshot-params-view-transition-opt-in">view transition opt-in</span> is true,
<li><p>Let <var>viewTransitionRule</var> be the result of calling
<span>resolve <code>@view-transition</code> rule</span> given
<var>displayedDocument</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

This new version looks great, thank you for doing that.

Although, I wonder if it'd be better to call into the CSS spec once instead of twice? I.e. instead of

If displayedDocument's can initiate outbound view transition is true, then set potentiallyTriggerViewTransition to the result of calling can navigation trigger a cross-document view-transition? given displayedDocument's origin...

something like

Set potentiallyTriggerViewTransition to the result of calling can navigation trigger a cross-document view transition? given displayedDocument, ...

@domenic domenic merged commit f9313a2 into whatwg:main Mar 15, 2024
2 checks passed
@noamr noamr deleted the pageswap-refactor branch March 15, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants