-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add browsingContext.historyUpdated event #740
base: main
Are you sure you want to change the base?
Conversation
f9bc26b
to
90d18b4
Compare
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.
Thank you Alex for getting started on this event! For now I have two questions. See inline.
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.
LGTM
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.
Thanks, I feel like we should at least have the context info in the event payload, otherwise it will be hard to use?
90d18b4
to
8d58742
Compare
thanks for reviewing, totally! It was a while since I started this PR draft 😅 |
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.
Thanks for the update, looks good to me!
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.
A couple of comments, but no blockers.
@@ -4365,6 +4366,48 @@ navigated</dfn> steps given |navigable| and |navigation status|: | |||
|
|||
</div> | |||
|
|||
#### The browsingContext.historyUpdated Event #### {#event-browsingContext-historyUpdated} | |||
|
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 know we don't have this for the other events, but it could be useful to have some informative text indicating that this is only fired for pushState
and friends, not for history updates caused by navigation.
params: browsingContext.HistoryUpdatedParameters | ||
) | ||
|
||
browsingContext.HistoryUpdatedParameters = { |
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 wonder if we should also include a string indicating the type of the history update to distinguish pushState
from replaceState
?
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 think we could add it eventually. So far we didn't expose it and didn't have a use case for it so adding it would involve extra implementation work. Also we could add the serialized history state to be able to distinguish multiple updates if there are use cases.
Thanks for reviews! I plan to publish the HTML spec change for review and start working on WPT in the next weeks. For now, I will leave the PR open to incorporate potential updates during the implementation or WPT authoring. |
Draft WPT tests: web-platform-tests/wpt#48347 I am still clarifying some details on the HTML spec PR. |
There have been additional discussions on the HTML spec PR resulting in the call being moved to a different place in the HTML spec. PTAL and let me know if you have concerns about it. |
This change adds
browsingContext.historyUpdated
event to notify the client when a history update happens.Issue: #502
HTML spec: whatwg/html#10587
Preview | Diff