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

Initial draft of navigate event spec #63

Merged
merged 18 commits into from
Mar 12, 2021
Merged

Initial draft of navigate event spec #63

merged 18 commits into from
Mar 12, 2021

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Mar 5, 2021

@natechapin this should hopefully help. (Check out the preview link that should be auto-generated.) It's a bit more complex than your implementation because I want to preserve any pushState() or replaceState() arguments, and it tries to incorporate all the stuff that you're deferring for now.

I don't yet have the monkeypatches that fire the event, so the "fire an event" algorithm might not be quite right. But I think I captured all the inputs. (There are a lot of them... maybe some can be collapsed, once we see the call sites.)


Preview | Diff

@domenic

This comment has been minimized.

@domenic
Copy link
Collaborator Author

domenic commented Mar 9, 2021

Note to self: I think this needs updates to handle #65

@domenic
Copy link
Collaborator Author

domenic commented Mar 10, 2021

OK, I think this is complete. @natechapin mind taking a look? You probably would most enjoy the preview link in the OP, instead of the raw diff.

@domenic
Copy link
Collaborator Author

domenic commented Mar 11, 2021

@natechapin found a bug while implementing: the "Modify the traverse the history by a delta algorithm" patch needs to pass hashChange in same-document cases where the hash differs.

Copy link
Collaborator

@natechapin natechapin left a comment

Choose a reason for hiding this comment

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

This looks very reasonable to me. I'm sure I'll find some additional things to tweak as I continue to implement.

Just one cleanup suggestion/question for now.

for: URL and history update steps
text: serializedData; url: history.html#uhus-serializeddata
text: title; url: history.html#uhus-title
text: isPush; url: history.html#uhus-ispush
Copy link
Collaborator

Choose a reason for hiding this comment

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

Semi-related: It seems strange to me that the HTML spec both has isPush and the "history handling behavior". The spec changes proposed here will greatly expand the use of isPush. Can/should we deprecate isPush in favor of the "history handling behavior" as a cleanup step?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I can see both sides. History handling behavior is very tied to the full navigate lifecycle right now (navigate / all the history algorithms). Whereas the URL and history update steps are much smaller scoped. We could reuse history handling (and assert that it's only ever "default" or "replace")... and I guess that's closer to what Chrome does. Hmm.

@domenic domenic merged commit 271f325 into main Mar 12, 2021
@domenic domenic deleted the spec branch March 12, 2021 19:50
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