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

Fire the pageswap event when navigating away from a document #10002

Merged
merged 29 commits into from
Feb 29, 2024

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Dec 18, 2023

Together with w3c/csswg-drafts#9819.

The pageswap event is specified here to work as folows:

  • It's sent whenever a document navigates away, to a different document, but not when unloaded for other reasons, such as closing the browser types or unloading an ancestor.
  • It includes a NavigationActivation object for same-origin navigations.
  • If the navigation might trigger a cross-document view-transition (this is determined when snapshotting the source document, by reading the CSS), pageswap is fired and then unloading is deferred until the old state is captured for the view-transition or immediately if the ViewTransition is skipped.

Closes #9702

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


/browsing-the-web.html ( diff )
/document-lifecycle.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/nav-history-apis.html ( diff )
/sections.html ( diff )
/webappapis.html ( diff )

@noamr noamr closed this Dec 18, 2023
@noamr noamr reopened this Dec 18, 2023
@noamr noamr changed the title WIP: Pageconceal Fire the pageconceal event when navigating away from a document Jan 19, 2024
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

LGTM. Some minor comments about renames and clean up from removing userInitiated from the event.

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 Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Contributor

@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.

Seems much simpler now, thanks!

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
@noamr noamr changed the title Fire the pageconceal event when navigating away from a document Fire the pageswap event when navigating away from a document Feb 8, 2024
Copy link
Member

@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.

All the hard parts look good!

source Outdated 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 Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@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.

LGTM.

It occurs to me that maybe it would be cleaner to create separate algorithms instead of the many closures. (Namely the closures for firePageSwap, unloadSteps, everything inside "Otherwise, queue a global task on the navigation and traversal task source given navigable's active window to run the steps:".) That might help keep the overall complexity of reading the algorithm from top to bottom contained.

However, doing so would require adding a good amount of parameters to those separate algorithms (to correspond to the various closed-over variables). And I'm not sure the end result would be that much better, especially since the algorithm is already huge. So, definitely not a blocker for this PR. Just something I'm curious to get your impression of.

@domenic domenic merged commit a0a7218 into whatwg:main Feb 29, 2024
2 checks passed
@noamr
Copy link
Contributor Author

noamr commented Feb 29, 2024

LGTM.

It occurs to me that maybe it would be cleaner to create separate algorithms instead of the many closures. (Namely the closures for firePageSwap, unloadSteps, everything inside "Otherwise, queue a global task on the navigation and traversal task source given navigable's active window to run the steps:".) That might help keep the overall complexity of reading the algorithm from top to bottom contained.

However, doing so would require adding a good amount of parameters to those separate algorithms (to correspond to the various closed-over variables). And I'm not sure the end result would be that much better, especially since the algorithm is already huge. So, definitely not a blocker for this PR. Just something I'm curious to get your impression of.

I'm thinking that perhaps we should keep the closure style, but use something like labels or list/details to make it easier to read?

@domenic
Copy link
Member

domenic commented Feb 29, 2024

Eh, the main thing I was hoping to get was the ability to read the algorithms top to bottom. So moving from closures to standalone functions would be a form of https://refactoring.guru/extract-method

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.

[View Transition] Event on old Document to set transition state
4 participants